Обсуждение: small cleanup: unify scanstr() functions

Hi,

Looking at the guc file code, GUC_scanstr() is almost the same as the
exported function scanstr(), except the latter requires finicky extra
coding around double quotes both in its callers and while creating the
input.

In the attached, the GUC_scanstr() function body is moved to scansup.c
to replace scanstr(), but with a different order of if-statements to
make the diff smaller. Since we have control over what goes in the BKI
file, we can use single-quoted escape strings there, allowing removal
of special case code for double quotes.

--
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

John Naylor <john.naylor@2ndquadrant.com> writes:
> In the attached, the GUC_scanstr() function body is moved to scansup.c
> to replace scanstr(), but with a different order of if-statements to
> make the diff smaller. Since we have control over what goes in the BKI
> file, we can use single-quoted escape strings there, allowing removal
> of special case code for double quotes.

I'm unsure this topic is worth messing with.  But if we do so, I'd kind
of like to move scanstr() out of parser/scansup.c.  Since it's not used
by the core scanner, only bootstrap, it seems out of place there; and
it's confusing because somebody coming across it for the first time
would reasonably assume that it does have something to do with how
we lex normal SQL strings.

Of course, that just begs the question of where to put it instead.
But since guc-file.l lives in utils/misc/, in or beside that file
does not seem that awful.

We might consider renaming it to something a shade less generic, too.
I have no immediate suggestions on that score.

In short: maybe instead of what you have here, leave GUC_scanstr()
alone except for a possible rename; make bootscanner.l use that;
and drop the now-unused scanstr().

            regards, tom lane

On Thu, Oct 1, 2020 at 11:19 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm unsure this topic is worth messing with.  But if we do so, I'd kind
> of like to move scanstr() out of parser/scansup.c.  Since it's not used
> by the core scanner, only bootstrap, it seems out of place there; and
> it's confusing because somebody coming across it for the first time
> would reasonably assume that it does have something to do with how
> we lex normal SQL strings.
>
> Of course, that just begs the question of where to put it instead.
> But since guc-file.l lives in utils/misc/, in or beside that file
> does not seem that awful.

A ringing endorsement if I ever heard one. ;-) Seems fine if it
remains in guc-file.l. Cosmetic considerations are

- The declaration in guc.h should fit in with code that's already
there. I put it next to the *Parse* functions.
- There seem to be a few unused headers included into bootscanner.l.
To keep "#include guc.h" from looking as strange as those, I added a
comment. It might be worth removing those extra includes, but that's
for another day.

> We might consider renaming it to something a shade less generic, too.
> I have no immediate suggestions on that score.

I've named it DeescapeQuotedString to see how that looks.

> In short: maybe instead of what you have here, leave GUC_scanstr()
> alone except for a possible rename; make bootscanner.l use that;
> and drop the now-unused scanstr().

v2 done that way.

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

John Naylor <john.naylor@2ndquadrant.com> writes:
> On Thu, Oct 1, 2020 at 11:19 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In short: maybe instead of what you have here, leave GUC_scanstr()
>> alone except for a possible rename; make bootscanner.l use that;
>> and drop the now-unused scanstr().

> v2 done that way.

Pushed with very minor adjustments --- mainly, that I rewrote the commit
message to emphasize the change in postgres.bki conventions, since that
could possibly bite someone in more-surprising ways than simple removal
of a function.

> - There seem to be a few unused headers included into bootscanner.l.
> To keep "#include guc.h" from looking as strange as those, I added a
> comment. It might be worth removing those extra includes, but that's
> for another day.

I went ahead and removed everything in bootscanner.l and bootparse.y
that could be removed without provoking a compile error.  I was slightly
astonished at how many there were.  One gets the impression that there
was once a good deal of very low-level code in the bootstrap grammar.
I didn't try very hard to trace the commit history, but I did note that
almost all of the removed #includes dated to 1996 or so.  I'm surprised
they survived Bruce's occasional attempts at removing unused #includes;
maybe his script didn't check .y/.l files.

            regards, tom lane