Horton

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

John Naylor <johncnaylorls@gmail.com>

View thread View on postgr.es
From: John Naylor <johncnaylorls@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org, Andres Freund <andres@anarazel.de>, Peter Eisentraut <peter_e@gmx.net>, Michael Paquier <michael.paquier@gmail.com>
Date: 2024-08-12T05:33:49Z
Lists: pgsql-hackers 
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.

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