Horton

Thread

Commits

  1. ecpg: clean up some other assorted memory leaks.

  2. ecpg: fix some memory leakage of data-type-related structures.

  3. ecpg: put all string-valued tokens returned by pgc.l in local storage.

  4. ecpg: fix more minor mishandling of bad input in preprocessor.

  5. ecpg: fix some minor mishandling of bad input in preprocessor.

  6. ecpg: avoid breaking the IDENT precedence level in two.

  7. ecpg: improve preprocessor's memory management.

  8. ecpg: move some functions into a new file ecpg/preproc/util.c.

  9. ecpg: re-implement preprocessor's string management.

  10. ecpg: major cleanup, simplification, and documentation of parse.pl.

  11. ecpg: remove check_rules.pl.

  12. ecpg: clean up documentation of parse.pl, and add more input checking.

  13. Clean up indentation and whitespace inconsistencies in ecpg.

  14. Exclude flex-generated code from coverage testing

  1. ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-19T02:18:34Z

    Here is a patch series that aims to clean up ecpg's preprocessor
code a little and fix the compile time problems we're seeing with
late-model clang [1].  I guess whether it's a cleanup is in the eye of
the beholder, but it definitely succeeds at fixing compile time: for
me, the time needed to compile preproc.o with clang 16 drops from
104 seconds to less than 1 second.  It might be a little faster at
processing input too, though that wasn't the primary goal.

The reason that clang is having a problem seems to be the large number
of virtually-duplicate semantic actions in the generated preproc.y.
So I looked for a way to allow most productions to use the default
semantic action rather than having to write anything.  The core idea
of this patch is to stop returning <str> results from grammar
nonterminals and instead pass the strings back as Bison location data,
which we can do by redefining YYLTYPE as "char *".  Since ecpg isn't
using Bison's location logic for error reports, and seems unlikely to
do so in future, this doesn't cost us anything.  Then we can implement
a one-size-fits-most token concatenation rule in YYLLOC_DEFAULT, and
only the various handmade rules that don't want to just concatenate
their inputs need to do something different.  (Within those handmade
rules, the main notational change needed is to write "@N" not "$N"
for the string value of the N'th input token, and "@@" not "$@"
for the output string value.)  Aside from not giving clang
indigestion, this makes the compiled parser a little smaller since
there are fewer semantic actions that need code space.

As Andres remarked in the other thread, the parse.pl script that
constructs preproc.y is undocumented and unreadable, so I spent
a good deal of time reverse-engineering and cleaning that up
before I went to work on the actual problem.  Four of the six
patches in this series are in the way of cleanup and adding
documentation, with no significant behavioral changes.

The patch series comprises:

0001: pgindent the code in pgc.l and preproc.y's precursor files.
Yeah, this was my latent OCD rearing its head, but I hate looking
at or working on messy code.  It did actually pay some dividends
later on, by making it easier to make bulk edits.

0002: improve the external documentation and error checking of
parse.pl.  This was basically to convince myself that I knew
what it was supposed to do before I started changing it.
The error checks did find some errors, too: in particular,
it turns out there are two unused entries in ecpg.addons.

(This implies that check_rules.pl is completely worthless and should
be nuked: it adds build cycles and maintenance effort while failing
to reliably accomplish its one job of detecting dead rules, because
what it is testing is not the same thing that parse.pl actually does.
I've not included that removal in this patch series, though.)

0003: clean up and simplify parse.pl, and write some internal
documentation for it.  The effort of understanding it exposed that
there was a pretty fair amount of dead or at least redundant code,
so I got rid of that.  This patch changes the output preproc.y
file only to the extent of removing some blank lines that didn't
seem very useful to preserve.

0004: this is where something useful happens, specifically where
we change <str>-returning productions to return void and instead
pass back the desired output string as location data.  In most
cases the productions now need no explicit semantic action at all,
allowing substantial simplification in parse.pl.

0005: more cleanup.  I didn't want to add more memory-management
code to preproc/type.c, where mm_alloc and mm_strdup have lived
for no explicable reason.  I pulled those and a couple of other
functions out to a new file util.c, so as to have a better home
for new utility code.

0006: the big problem with 0004 is that it can't use the trick
of freeing input substrings as soon as it's created the merged
string, as cat_str and friends have historically done.  That's
because YYLLOC_DEFAULT runs before the rule's semantic action
if any, so that if the action does need to look at the input
strings, they'd already be freed.  So 0004 is leaking memory
rather badly.  Fix that by creating a notion of "local" memory
that will be reclaimed at end of statement, analogously to
short-lived memory contexts in the backend.  All the string
concatenation work happens in short-lived storage and we don't
worry about getting rid of intermediate values immediately.
By making cat_str and friends work similarly, we can get rid
of quite a lot of explicit mm_strdup calls, although we do have
to add some at places where we're building long-lived data
structures.  This should greatly reduce the malloc/free traffic
too, at the cost of eating somewhat more space intra-statement.

In my view 0006 is about the scariest part of this, as it's
hard to be sure that there are no use-after-free problems
wherein a pointer to a short-lived string survives past end
of statement.  It gets through the ecpg regression tests
under valgrind successfully, but I don't have much faith
in the thoroughness of the code coverage of those tests.
(If our code coverage tools worked on bison/flex stuff,
maybe this'd be less scary ... but they don't.)

I'll park this in the July commitfest.

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/CAGECzQQg4qmGbqqLbK9yyReWd1g%3Dd7T07_gua%2BRKXsdsW9BG-Q%40mail.gmail.com

  2. Re: ECPG cleanup and fix for clang compile-time problem

    Andres Freund <andres@anarazel.de> — 2024-04-19T03:03:46Z

    Hi,

On 2024-04-18 22:18:34 -0400, Tom Lane wrote:
> Here is a patch series that aims to clean up ecpg's preprocessor
> code a little and fix the compile time problems we're seeing with
> late-model clang [1].  I guess whether it's a cleanup is in the eye of
> the beholder, but it definitely succeeds at fixing compile time: for
> me, the time needed to compile preproc.o with clang 16 drops from
> 104 seconds to less than 1 second.  It might be a little faster at
> processing input too, though that wasn't the primary goal.

Nice! I'll look at this more later.


For now I just wanted to point one minor detail:

> (If our code coverage tools worked on bison/flex stuff,
> maybe this'd be less scary ... but they don't.)

For bison coverage seems to work, see e.g.:

https://coverage.postgresql.org/src/interfaces/ecpg/preproc/preproc.y.gcov.html#10638

I think the only reason it doesn't work for flex is that we have
/* LCOV_EXCL_START */
/* LCOV_EXCL_STOP */

around the scanner "body".  Without that I get reasonable-looking, albeit not
very comforting, coverage for pgc.l as well.

                                               |Lines      |Functions|Branches
Filename                                       |Rate    Num|Rate  Num|Rate   Num
src/interfaces/ecpg/preproc/pgc.l              |65.9%   748|87.5%   8|    -    0
src/interfaces/ecpg/preproc/preproc.y          |29.9%  4964|66.7%  15|    -    0


This has been introduced by

commit 421167362242ce1fb46d6d720798787e7cd65aad
Author: Peter Eisentraut <peter_e@gmx.net>
Date:   2017-08-10 23:33:47 -0400

    Exclude flex-generated code from coverage testing

    Flex generates a lot of functions that are not actually used.  In order
    to avoid coverage figures being ruined by that, mark up the part of the
    .l files where the generated code appears by lcov exclusion markers.
    That way, lcov will typically only reported on coverage for the .l file,
    which is under our control, but not for the .c file.

    Reviewed-by: Michael Paquier <michael.paquier@gmail.com>


but I don't think it's working as intended, as it's also preventing coverage
for the actual scanner definition.

Greetings,

Andres Freund

  3. Re: ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-19T03:11:52Z

    Andres Freund <andres@anarazel.de> writes:
> On 2024-04-18 22:18:34 -0400, Tom Lane wrote:
>> (If our code coverage tools worked on bison/flex stuff,
>> maybe this'd be less scary ... but they don't.)

> For bison coverage seems to work, see e.g.:

Yeah, I'd just noticed that --- I had it in my head that we'd put
LCOV_EXCL_START/STOP into bison files too, but nope they are only
in flex files.  That's good for this specific problem, because the
code I'm worried about is all in the bison file.

> around the scanner "body".  Without that I get reasonable-looking, albeit not
> very comforting, coverage for pgc.l as well.

I was just looking locally at what I got by removing that, and sadly
I don't think I believe it: there are a lot of places where it claims
we hit lines we don't, and vice versa.  That might be partially
blamable on old tools on my RHEL8 workstation, but it sure seems
that flex output confuses lcov to some extent.

			regards, tom lane

  4. Re: ECPG cleanup and fix for clang compile-time problem

    Andres Freund <andres@anarazel.de> — 2024-04-19T03:40:22Z

    On 2024-04-18 23:11:52 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2024-04-18 22:18:34 -0400, Tom Lane wrote:
> >> (If our code coverage tools worked on bison/flex stuff,
> >> maybe this'd be less scary ... but they don't.)
>
> > For bison coverage seems to work, see e.g.:
>
> Yeah, I'd just noticed that --- I had it in my head that we'd put
> LCOV_EXCL_START/STOP into bison files too, but nope they are only
> in flex files.  That's good for this specific problem, because the
> code I'm worried about is all in the bison file.

At least locally the coverage seems to make sense too, both for the main
grammar and for ecpg's.


> > around the scanner "body".  Without that I get reasonable-looking, albeit not
> > very comforting, coverage for pgc.l as well.
>
> I was just looking locally at what I got by removing that, and sadly
> I don't think I believe it: there are a lot of places where it claims
> we hit lines we don't, and vice versa.  That might be partially
> blamable on old tools on my RHEL8 workstation, but it sure seems
> that flex output confuses lcov to some extent.

Hm. Here it mostly looks reasonable, except that at least things seem off by
1. And sure enough, if I look at pgc.l it has code like

case 2:
YY_RULE_SETUP
#line 465 "/home/andres/src/postgresql/src/interfaces/ecpg/preproc/pgc.l"
{
                                        token_start = yytext;
                                        state_before_str_start = YYSTATE;

However line 465 is actually the "token_start" line.

Further down this seems to get worse, by "<<EOF>>" it's off by 4 lines.


$ apt policy flex
flex:
  Installed: 2.6.4-8.2+b2
  Candidate: 2.6.4-8.2+b2
  Version table:
 *** 2.6.4-8.2+b2 500
        500 http://mirrors.ocf.berkeley.edu/debian unstable/main amd64 Packages
        100 /var/lib/dpkg/status

Greetings,

Andres Freund

  5. Re: ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-19T03:53:59Z

    Andres Freund <andres@anarazel.de> writes:
> On 2024-04-18 23:11:52 -0400, Tom Lane wrote:
>> I was just looking locally at what I got by removing that, and sadly
>> I don't think I believe it: there are a lot of places where it claims
>> we hit lines we don't, and vice versa.  That might be partially
>> blamable on old tools on my RHEL8 workstation, but it sure seems
>> that flex output confuses lcov to some extent.

> Hm. Here it mostly looks reasonable, except that at least things seem off by
> 1.

Yeah, now that you mention it what I'm seeing looks like the line
numbering might be off-by-one.  Time for a bug report?

			regards, tom lane

  6. Re: ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-19T15:21:05Z

    One other bit of randomness that I noticed: ecpg's parse.pl has
this undocumented bit of logic:

                if ($a eq 'IDENT' && $prior eq '%nonassoc')
                {

                    # add more tokens to the list
                    $str = $str . "\n%nonassoc CSTRING";
                }

The net effect of that is that, where gram.y writes

%nonassoc    UNBOUNDED NESTED /* ideally would have same precedence as IDENT */
%nonassoc    IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
             SET KEYS OBJECT_P SCALAR VALUE_P WITH WITHOUT PATH
%left        Op OPERATOR      /* multi-character ops and user-defined operators */

preproc.c has

 %nonassoc UNBOUNDED NESTED
 %nonassoc IDENT
%nonassoc CSTRING PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
 SET KEYS OBJECT_P SCALAR VALUE_P WITH WITHOUT PATH
 %left Op OPERATOR

If you don't find that scary as heck, I suggest reading the very long
comment just in front of the cited lines of gram.y.  The argument why
assigning these keywords a precedence at all is OK depends heavily
on it being the same precedence as IDENT, yet here's ECPG randomly
breaking that.

We seem to have avoided problems though, because if I fix things
by manually editing preproc.y to re-join the lines:

 %nonassoc IDENT CSTRING PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP

the generated preproc.c doesn't change at all.  Actually, I can
take CSTRING out of this list altogether and it still doesn't
change the results ... although looking at how CSTRING is used,
it looks safer to give it the same precedence as IDENT.

I think we should change parse.pl to give one or the other of these
results before something more serious breaks there.

			regards, tom lane

  7. Re: ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-05T15:56:55Z

    The cfbot noticed that this patchset had a conflict with d35cd0619,
so here's a rebase.  It's just a rebase of v1, no other changes.

			regards, tom lane

  8. Re: ECPG cleanup and fix for clang compile-time problem

    John Naylor <johncnaylorls@gmail.com> — 2024-08-12T05:33:49Z

    On Fri, Apr 19, 2024 at 10:21 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> One other bit of randomness that I noticed: ecpg's parse.pl has
> this undocumented bit of logic:
>
>                 if ($a eq 'IDENT' && $prior eq '%nonassoc')
>                 {
>
>                     # add more tokens to the list
>                     $str = $str . "\n%nonassoc CSTRING";
>                 }

> preproc.c has
>
>  %nonassoc UNBOUNDED NESTED
>  %nonassoc IDENT
> %nonassoc CSTRING PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
>  SET KEYS OBJECT_P SCALAR VALUE_P WITH WITHOUT PATH
>  %left Op OPERATOR
>
> If you don't find that scary as heck, I suggest reading the very long
> comment just in front of the cited lines of gram.y.  The argument why
> assigning these keywords a precedence at all is OK depends heavily
> on it being the same precedence as IDENT, yet here's ECPG randomly
> breaking that.

Before 7f380c59f (Reduce size of backend scanner's tables), it was
even more spread out:

# add two more tokens to the list
$str = $str . "\n%nonassoc CSTRING\n%nonassoc UIDENT";

...giving:
 %nonassoc UNBOUNDED
 %nonassoc IDENT
%nonassoc CSTRING
%nonassoc UIDENT GENERATED NULL_P PARTITION RANGE ROWS GROUPS
PRECEDING FOLLOWING CUBE ROLLUP

> We seem to have avoided problems though, because if I fix things
> by manually editing preproc.y to re-join the lines:
>
>  %nonassoc IDENT CSTRING PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
>
> the generated preproc.c doesn't change at all.

On a whim I tried rejoining on v12 and the .c doesn't change there, either.

> Actually, I can
> take CSTRING out of this list altogether and it still doesn't
> change the results ... although looking at how CSTRING is used,
> it looks safer to give it the same precedence as IDENT.

Doing that on v12 on top of rejoining results in a shift-reduce
conflict, so I imagine that's why it's there. Maybe it's outdated, but
this backs up your inclination that it's safer to keep.

  9. Re: ECPG cleanup and fix for clang compile-time problem

    John Naylor <johncnaylorls@gmail.com> — 2024-08-12T10:46:22Z

    On Fri, Jul 5, 2024 at 10:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> The cfbot noticed that this patchset had a conflict with d35cd0619,
> so here's a rebase.  It's just a rebase of v1, no other changes.

Hi Tom,

I started looking at the earlier cleanup patches.

0001 seems straightforward. Note: It doesn't apply cleanly anymore,
but does with 'patch'.
0002 LGTM, just a couple minor comments:

--- a/src/interfaces/ecpg/preproc/parse.pl
+++ b/src/interfaces/ecpg/preproc/parse.pl
@@ -1,7 +1,13 @@
 #!/usr/bin/perl
 # src/interfaces/ecpg/preproc/parse.pl
 # parser generator for ecpg version 2
-# call with backend parser as stdin
+#
+# See README.parser for some explanation of what this does.

Doesn't this patch set put us up to version 3? ;-) Looking in the
history, a very long time ago a separate "parse2.pl" was committed for
some reason, but that was reconciled some time later. This patch
doesn't need to get rid of that meaningless version number, but I find
it distracting.

+ # There may be multiple ECPG: lines and then multiple lines of code.
+ # Each block of code needs to be added to all prior ECPG records.

This took me a while to parse at first. Some places in this script put
quotes around words-with-colons, and that seems good for readability.

0003:

Looks a heck of a lot better, but I didn't try to understand
everything in the script, either before or after.

+ # Emit the target part of the rule.
+ # Note: the leading space is just to match
+ # the old, rather weird output logic.
+ $tstr = ' ' . $non_term_id . ':';
+ add_to_buffer('rules', $tstr);

Removing the leading space (or making it two spaces) has no effect on
the output -- does that get normalized elsewhere?

That's all I have for now.

  10. Re: ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-12T19:19:57Z

    John Naylor <johncnaylorls@gmail.com> writes:
> I started looking at the earlier cleanup patches.

Thanks for looking!

> 0001 seems straightforward. Note: It doesn't apply cleanly anymore,
> but does with 'patch'.

Odd, after rebasing it seems to have only line-number differences.

> + # Emit the target part of the rule.
> + # Note: the leading space is just to match
> + # the old, rather weird output logic.
> + $tstr = ' ' . $non_term_id . ':';
> + add_to_buffer('rules', $tstr);

> Removing the leading space (or making it two spaces) has no effect on
> the output -- does that get normalized elsewhere?

It does affect horizontal space in the generated preproc.y file,
which'd have no effect on the derived preproc.c file.  I tweaked
the commit message to clarify that.

I adopted your other suggestions, no need to rehash them.

Here's a rebased but otherwise identical patchset.  I also added
an 0007 that removes check_rules.pl as threatened.

			regards, tom lane

  11. Re: ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-15T00:43:36Z

    I wrote:
> Here's a rebased but otherwise identical patchset.  I also added
> an 0007 that removes check_rules.pl as threatened.

I've done some more work on this and hope to post an updated patchset
tomorrow.  Before that though, is there any objection to going ahead
with pushing the 0001 patch (pgindent'ing ecpg's lexer and parser
files)?  It's pretty bulky yet of no intellectual interest, so I'd
like to stop carrying it forward.

			regards, tom lane

  12. Re: ECPG cleanup and fix for clang compile-time problem

    Peter Eisentraut <peter@eisentraut.org> — 2024-08-15T07:20:13Z

    On 15.08.24 02:43, Tom Lane wrote:
> I wrote:
>> Here's a rebased but otherwise identical patchset.  I also added
>> an 0007 that removes check_rules.pl as threatened.
> 
> I've done some more work on this and hope to post an updated patchset
> tomorrow.  Before that though, is there any objection to going ahead
> with pushing the 0001 patch (pgindent'ing ecpg's lexer and parser
> files)?  It's pretty bulky yet of no intellectual interest, so I'd
> like to stop carrying it forward.

The indentation patch looks good to me and it would be good to get it 
out of the way.

  13. Re: ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-15T19:58:35Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> On 15.08.24 02:43, Tom Lane wrote:
>> I've done some more work on this and hope to post an updated patchset
>> tomorrow.  Before that though, is there any objection to going ahead
>> with pushing the 0001 patch (pgindent'ing ecpg's lexer and parser
>> files)?  It's pretty bulky yet of no intellectual interest, so I'd
>> like to stop carrying it forward.

> The indentation patch looks good to me and it would be good to get it 
> out of the way.

Thanks, done.  Here's a revised patchset.

0001-0003 are substantially identical to the previous 0002-0004.
Likewise 0005 is basically the same as previous 0006,
and 0009 is identical to the previous 0007.

0004 differs from the previous 0005 in also moving the
cat_str/make_str functions into util.c, because I found that
at least the make_str functions could be useful in pgc.l.

The new stuff is in 0006-0008, and what it basically does is
clean up all remaining memory leakage in ecpg --- or at least,
all that valgrind can find while running ecpg's regression tests.
(I'm not fool enough to think that there might not be some in
unexercised code paths.)  It's fairly straightforward attention
to detail in data structure management.

I discovered the need for more effort on memory leakage by
doing some simple performance testing (basically, running ecpg
on a big file made by pasting together lots of copies of some
of the regression test inputs).  v3 was slower and consumed more
memory than HEAD :-(.  HEAD does already leak quite a bit of
memory, but v3 was worse, mainly because the string tokens
returned by pgc.l weren't being reclaimed.  I hadn't really
set out to drive the leakage to zero, but it turned out to not
be that hard, so I did it.

With those fixes, I see v4 running maybe 10% faster than HEAD,
rather than a similar amount slower.  I'm content with that
result, and feel that this may now be commit-quality.

			regards, tom lane

  14. Re: ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-15T20:21:42Z

    I wrote:
> Thanks, done.  Here's a revised patchset.

The cfbot points out that I should probably have marked progname
as "static" in 0008.  I'm not going to repost the patchset just for
that, though.

			regards, tom lane

  15. Re: ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-04T20:46:44Z

    I wrote:
> The cfbot points out that I should probably have marked progname
> as "static" in 0008.  I'm not going to repost the patchset just for
> that, though.

Rebase needed after f22e84df1, so here's an update that rebases
up to HEAD and adds the missing "static".  No other changes.

(Anybody want to review this?  I'm getting tired of rebasing it,
and we're missing out on the clang build time savings.)

			regards, tom lane

  16. Re: ECPG cleanup and fix for clang compile-time problem

    John Naylor <johncnaylorls@gmail.com> — 2024-10-07T09:52:04Z

    On Saturday, October 5, 2024, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>
> Rebase needed after f22e84df1, so here's an update that rebases
> up to HEAD and adds the missing "static".  No other changes.
>
> (Anybody want to review this?  I'm getting tired of rebasing it,
> and we're missing out on the clang build time savings.)


 Sorry for the delay, I'll respond in a couple days.

  17. Re: ECPG cleanup and fix for clang compile-time problem

    John Naylor <johncnaylorls@gmail.com> — 2024-10-11T11:43:22Z

    > [v5]

0001 - LGTM, maybe can be squashed with 0009?

0002 - I went through this again and don't see anything that should
raise eyebrows.

+ # HACK: insert our own %nonassoc line after IDENT.
+ # XXX: this seems pretty wrong, IDENT is not last on its line!

We can come back to this afterwards, as mentioned elsewhere in the thread.

0003

Clang is what motivated this, but gcc also shows a speedup -- from
5.9s to 1.6s on this old machine, which is great. The giant switch
statement in preproc.c has about 1/10 the labels as before.

+In the original implementation of ecpg, the strings constructed
+by grammar rules were returned as the Bison result of each rule.
+This led to a large number of effectively-identical rule actions,
+which caused compilation-time problems with some versions of clang.
+Now, rules that need to return a string are declared as having

"Original" is going to be a mystery in a few years -- I'd describe
this in terms of "as of PG18" or some such.

+ * is producing uniformly-cased output of keywords.  (That's mostly
+ * cosmetic, but there are places in ecpglib that expect to receive
+ * downcased keywords, plus it keeps us regression-test-compatible
+ * with the old implementation of ecpg.)

Ditto with "old".

+ /* List a token here if pgc.l assigns to base_yylval.str for it */

Does pgc.l need to have a similar comment?

0004 seems like a sensible reorg.

0005 - I wondered if the change of YYLTYPE to "const char *" can be
done in a separate commit to make the other changes more legible, but
that might not be worth the effort.

+ * "Local" (or "location"?) memory management support

"Local" seems to fit well enough. Tying the arena to the statement
level seems sound.

I haven't looked closely at 0006 through 0009. One possible concern is
that the regression tests might not cover very well, but if you can
get valgrind silent for memory leaks for what they do cover, that's
certainly a good step.

  18. Re: ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-14T18:25:10Z

    John Naylor <johncnaylorls@gmail.com> writes:
>> [v5]

Thanks for reviewing!  I pushed 0001-0005 and 0009, adopting
your suggestions except for

> + /* List a token here if pgc.l assigns to base_yylval.str for it */
> Does pgc.l need to have a similar comment?

That's not a bad suggestion, but I couldn't see any very useful place
to put such a comment.

> I haven't looked closely at 0006 through 0009. One possible concern is
> that the regression tests might not cover very well, but if you can
> get valgrind silent for memory leaks for what they do cover, that's
> certainly a good step.

Attached are rebased and renumbered 0006-0008, mostly to keep the
cfbot happy.  We could actually stop here, if we were feeling lazy,
but now that I've done the work I'm inclined to push forward with
the rest.

The rest is just memory leak removal, and I suspect that nobody really
cares that much about small leakage in the preprocessor: you'd have to
be running some darn big files through it to notice.  FTR, here are
the total leaks reported by valgrind for running the ecpg regression
tests, using code like

$ grep lost: *log | tr -d ',' | awk '{sum += $5} 
END {print sum}'

Before these patches:	25743
after 0003:		59049363
after 0005:		141556		(this is master now)
after 0006(0001):	132633
after 0007(0002):	9087
after 0008(0003):	0

So clearly, 0003 by itself wasn't good enough, but arguably no
real users will notice the extra inefficiency as of HEAD.
Still, I'd kind of like to get 0007 (now 0002) in there, and
I believe 0006 (0001) is a necessary prerequisite to that.

			regards, tom lane

  19. Re: ECPG cleanup and fix for clang compile-time problem

    Alexander Law <exclusion@gmail.com> — 2024-10-16T03:00:00Z

    Hello Tom,

14.10.2024 21:25, Tom Lane wrote:
> Attached are rebased and renumbered 0006-0008, mostly to keep the
> cfbot happy.  We could actually stop here, if we were feeling lazy,
> but now that I've done the work I'm inclined to push forward with
> the rest.
>
> The rest is just memory leak removal, and I suspect that nobody really
> cares that much about small leakage in the preprocessor: you'd have to
> be running some darn big files through it to notice.  FTR, here are
> the total leaks reported by valgrind for running the ecpg regression
> tests, using code like

Maybe you would like to fix in passing several (not new) defects, I've
found while playing with ecpg under Valgrind:
echo "
   EXEC SQL DECLARE cur1 CURSOR FOR stmt1;
" > t.pgc

valgrind  .../preproc/ecpg ... t.pgc

==831888== Conditional jump or move depends on uninitialised value(s)
==831888==    at 0x10C7B0: main (ecpg.c:490)
==831888==
char_array.pgc:2: WARNING: cursor "cur1" has been declared but not opened

Another case:
   EXEC SQL DECLARE cur_1 CURSOR FOR stmt_1;
   EXEC SQL FETCH cur_1 INTO :f1[[i];

==1335775==
==1335775== Conditional jump or move depends on uninitialised value(s)
==1335775==    at 0x121294: find_variable (variable.c:211)
==1335775==    by 0x11D661: base_yyparse (preproc.y:9749)
==1335775==    by 0x10C78F: main (ecpg.c:483)
==1335775==
==1335775== Conditional jump or move depends on uninitialised value(s)
==1335775==    at 0x121299: find_variable (variable.c:211)
==1335775==    by 0x11D661: base_yyparse (preproc.y:9749)
==1335775==    by 0x10C78F: main (ecpg.c:483)
==1335775==
==1335775== Invalid read of size 1
==1335775==    at 0x12128B: find_variable (variable.c:211)
==1335775==    by 0x11D661: base_yyparse (preproc.y:9749)
==1335775==    by 0x10C78F: main (ecpg.c:483)
==1335775==  Address 0x4e3bc80 is 0 bytes after a block of size 8,208 alloc'd
==1335775==    at 0x4848899: malloc (vg_replace_malloc.c:381)
==1335775==    by 0x120585: mm_alloc (util.c:87)
==1335775==    by 0x12065A: loc_alloc (util.c:151)
==1335775==    by 0x120701: loc_strdup (util.c:172)
==1335775==    by 0x10D9EC: base_yylex_location (parser.c:261)
==1335775==    by 0x10D4A1: filtered_base_yylex (parser.c:75)
==1335775==    by 0x114CA4: base_yyparse (preproc.c:39316)
==1335775==    by 0x10C78F: main (ecpg.c:483)
==1335775==
declare.pgc:2: ERROR: variable "f1" is not declared

One more case:
   EXEC SQL BEGIN DECLARE SECTION;
     int i;
   EXEC SQL END DECLARE SECTION;

   EXEC SQL DECLARE C CURSOR FOR SELECT 1;
   {
     EXEC SQL FETCH 1 IN C INTO :i;
   }
   EXEC SQL MOVE BACKWARD 1 IN C;

==961441== Invalid read of size 1
==961441==    at 0x484FBD7: strcmp (vg_replace_strmem.c:924)
==961441==    by 0x11442F: add_additional_variables (preproc.y:470)
==961441==    by 0x117DEF: base_yyparse (preproc.y:3548)
==961441==    by 0x10C78F: main (ecpg.c:483)
==961441==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

Best regards,
Alexander

  20. Re: ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-16T16:26:48Z

    Alexander Lakhin <exclusion@gmail.com> writes:
> Maybe you would like to fix in passing several (not new) defects, I've
> found while playing with ecpg under Valgrind:

Done.  After evaluation I concluded that none of these were worth the
trouble to back-patch, but by all means let's fix such things in HEAD.

			regards, tom lane

  21. Re: ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-16T17:54:21Z

    I wrote:
> Attached are rebased and renumbered 0006-0008, mostly to keep the
> cfbot happy.

For some reason I thought the stuff I pushed later on Monday
didn't interact with these patches, but the cfbot disabused me
of that folly.  Here's a rebased v7 --- no substantive change.

			regards, tom lane

  22. Re: ECPG cleanup and fix for clang compile-time problem

    Alexander Law <exclusion@gmail.com> — 2024-10-17T18:00:00Z

    Hello Tom,

16.10.2024 19:26, Tom Lane wrote:
> Alexander Lakhin <exclusion@gmail.com> writes:
>> Maybe you would like to fix in passing several (not new) defects, I've
>> found while playing with ecpg under Valgrind:
> Done.  After evaluation I concluded that none of these were worth the
> trouble to back-patch, but by all means let's fix such things in HEAD.

Thank you for fixing these defects!

I've spent a day testing ecpg preprocessor and found another couple of
bugs:
1)
EXEC SQL BEGIN DECLARE SECTION;
int i = 1;
EXEC SQL END DECLARE SECTION;

EXEC SQL DECLARE c CURSOR FOR SELECT :i;

{;}
}

EXEC SQL OPEN c;

==1247560==
==1247560== Invalid read of size 4
==1247560==    at 0x121C13: dump_variables (variable.c:462)
==1247560==    by 0x10CF75: output_statement (output.c:157)
==1247560==    by 0x116B6B: base_yyparse (preproc.y:1233)
==1247560==    by 0x10C78F: main (ecpg.c:483)
==1247560==  Address 0x4e39bc0 is 16 bytes inside a block of size 32 free'd
==1247560==    at 0x484B27F: free (vg_replace_malloc.c:872)
==1247560==    by 0x1219AE: remove_variables (variable.c:351)
==1247560==    by 0x11899F: base_yyparse (preproc.y:7853)
==1247560==    by 0x10C78F: main (ecpg.c:483)
==1247560==  Block was alloc'd at
==1247560==    at 0x4848899: malloc (vg_replace_malloc.c:381)
==1247560==    by 0x120703: mm_alloc (util.c:87)
==1247560==    by 0x120C3C: new_variable (variable.c:12)
==1247560==    by 0x11C27C: base_yyparse (preproc.y:8984)
==1247560==    by 0x10C78F: main (ecpg.c:483)
...

---
2)
EXEC SQL BEGIN DECLARE SECTION;
char s[100];
EXEC SQL END DECLARE SECTION;

EXEC SQL DECLARE cur_1 CURSOR FOR SELECT 1;
EXEC SQL FETCH cur_1 INTO :s[0];

==1247848== Invalid read of size 4
==1247848==    at 0x121388: find_variable (variable.c:238)
==1247848==    by 0x11D684: base_yyparse (preproc.y:9751)
==1247848==    by 0x10C78F: main (ecpg.c:483)
==1247848==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

---
Also, processing of .../ecpg/test/sql/include.pgc, containing only:
EXEC SQL INCLUDE ../sql;

emits merely:
input in flex scanner failed

I think that's all that can be found here without extra efforts.

Best regards,
Alexander

  23. Re: ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-17T19:42:41Z

    Alexander Lakhin <exclusion@gmail.com> writes:
> I've spent a day testing ecpg preprocessor and found another couple of
> bugs:

Thanks for the report!  The first couple of these seem simple
enough to fix, so I've done so.  As for

> Also, processing of .../ecpg/test/sql/include.pgc, containing only:
> EXEC SQL INCLUDE ../sql;
> emits merely:
> input in flex scanner failed

what we have here is an attempt to read a directory.  On Linux
it seems that fopen() is okay with that but then fread() fails
with EISDIR.  The fread() occurs in code emitted by flex that
is totally failing to produce a useful error report:

        while ( (result = (int) fread(buf, 1, (yy_size_t) max_size, base_yyin)) == 0 && ferror(base_yyin)) \
            { \
            if( errno != EINTR) \
                { \
                YY_FATAL_ERROR( "input in flex scanner failed" ); \
                break; \
                } \

It looks like YY_FATAL_ERROR can only accept a literal string,
so I can see why this isn't including strerror(errno), but still
I'd say that this is their poor error reporting not ours.  The
only thing we could really do about it is fstat the fopen's FD
and see if it's a directory, but that would only improve matters
for EISDIR not any other error cause.  I don't believe we've done
that anywhere else we use flex, so I'm not inclined to do it here.

> I think that's all that can be found here without extra efforts.

Thanks for poking at it!  I don't feel a huge need to search out
ecpg bugs in advance of field reports, but we might as well fix
things we do stumble across.

			regards, tom lane

  24. Re: ECPG cleanup and fix for clang compile-time problem

    John Naylor <johncnaylorls@gmail.com> — 2024-11-27T07:02:23Z

    On Tue, Oct 15, 2024 at 1:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> The rest is just memory leak removal, and I suspect that nobody really
> cares that much about small leakage in the preprocessor: you'd have to
> be running some darn big files through it to notice.  FTR, here are
> the total leaks reported by valgrind for running the ecpg regression
> tests, using code like
>
> $ grep lost: *log | tr -d ',' | awk '{sum += $5}
> END {print sum}'
>
> Before these patches:   25743
> after 0003:             59049363
> after 0005:             141556          (this is master now)
> after 0006(0001):       132633
> after 0007(0002):       9087
> after 0008(0003):       0
>
> So clearly, 0003 by itself wasn't good enough, but arguably no
> real users will notice the extra inefficiency as of HEAD.
> Still, I'd kind of like to get 0007 (now 0002) in there, and
> I believe 0006 (0001) is a necessary prerequisite to that.

Hi Tom,

I think you can go ahead and commit 0001-0003. For 0003 I do admit
being confused why valgrind had a problem with progname...

FWIW, I also took a quick eyeball check of the coverage output for
preproc.y looking for untested non-error branches that do things that
are not covered elsewhere, and I didn't notice any. Some places
allocating new connections are not covered, which should be handled by
0003.

-- 
John Naylor
Amazon Web Services

  25. Re: ECPG cleanup and fix for clang compile-time problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-11-27T17:59:38Z

    John Naylor <johncnaylorls@gmail.com> writes:
> I think you can go ahead and commit 0001-0003. For 0003 I do admit
> being confused why valgrind had a problem with progname...

Pushed, thanks for looking at it!

As for progname, valgrind was unhappy because the local in main()
went out of scope at return, but the malloc block was still there.
But this was weird coding anyway: pretty much everywhere else,
we store progname in a globally-accessible variable so that it can
be used in error messages.  I figured making it static was a down
payment on someday making ecpg's messages honor that convention.

> FWIW, I also took a quick eyeball check of the coverage output for
> preproc.y looking for untested non-error branches that do things that
> are not covered elsewhere, and I didn't notice any. Some places
> allocating new connections are not covered, which should be handled by
> 0003.

Yeah, ecpg's coverage report is fairly sad overall, but a lot of the
uncovered stuff is autogenerated grammar productions, which I think
we can have high confidence in.  I'm not currently feeling motivated
to try to improve that number.

			regards, tom lane