Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Return yyparse() result not via global variable

  2. Remove flex version checks

  3. Drop warning-free support for Flex 2.5.35

  4. plpgsql: pure parser and reentrant scanner

  5. flex code modernization: Replace YY_EXTRA_TYPE define with flex option

  6. guc: reentrant scanner

  7. jsonpath scanner: reentrant scanner

  8. syncrep parser: pure parser and reentrant scanner

  9. replication parser: pure parser and reentrant scanner

  10. bootstrap: pure parser and reentrant scanner

  11. Small whitespace improvement

  12. Prevent redeclaration of typedef yyscan_t

  13. seg: pure parser and reentrant scanner

  14. cube: pure parser and reentrant scanner

  1. pure parsers and reentrant scanners

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-02T09:46:00Z

    This patch series changes several parsers in the backend and contrib 
modules to use bison pure parsers and flex reentrant scanners. This is 
ultimately toward thread-safety, but I think it's also just nicer in 
general, and it might also fix a few possible small memory leaks.

I organized this patch series into very small incremental changes so 
that it's easier to follow. The final commits should probably combined a 
bit more (e.g., one per module).

In this patch series I have so far dealt with

* contrib/cube/
* contrib/seg/
* src/backend/replication/repl_*
* src/backend/replication/syncrep_*

These four needed the whole treatment: pure parser, reentrant scanner, 
and updated memory handling.

Also:
* src/backend/utils/adt/jsonpath_scan.l

This one already had a pure parser and palloc-based memory handling, but 
not a reentrant scanner, so I just did that.

The above are all pretty similar, so it was relatively easy to work 
through them once I had the first one figured out.

A couple of things that are still missing in the above:

* For repl_scanner.l, I want to use yyextra to deal with the static 
variables marked /*FIXME*/, but somehow I made that buggy, I'll need to 
take another look later.
* For both the replication parser and the syncrep parser, get rid of the 
global variables to pass back the results. Again, here I need another 
look later. I confused either myself or the compiler on these.

cube, seg, and jsonpath are about as done as I would want them to be.

Not done yet:
* src/backend/utils/misc/guc-file.l
* src/pl/plpgsql/src/pl_gram.y

These have quite different structures and requirements, so I plan to 
deal with them separately.

Not relevant for backend thread-safety:
* src/backend/bootstrap/

It might make sense to eventually covert that one as well, just so that 
the APIs are kept similar. But that could be for later.

Note that the core scanner and parser are already reentrant+pure.

Also, there are various other scanners and parsers in frontends (psql, 
pgbench, ecpg) that are not relevant for this. (Again, it might make 
sense to convert some of them later, and some of them are already done.)

AFAICT, all the options and coding techniques used here are already in 
use elsewhere in the tree, so there shouldn't be any concerns about 
bison or flex version compatibility.

  2. Re: pure parsers and reentrant scanners

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-16T07:39:06Z

    On 02.12.24 10:46, Peter Eisentraut wrote:
> This patch series changes several parsers in the backend and contrib 
> modules to use bison pure parsers and flex reentrant scanners. This is 
> ultimately toward thread-safety, but I think it's also just nicer in 
> general, and it might also fix a few possible small memory leaks.

I did a bit more work on this, so here is an updated patch set.

> Not done yet:
> * src/backend/utils/misc/guc-file.l
> * src/pl/plpgsql/src/pl_gram.y

I converted both of these scanners to reentrant, but I haven't done the 
plpgsql parser yet.

> Not relevant for backend thread-safety:
> * src/backend/bootstrap/
> 
> It might make sense to eventually covert that one as well, just so that 
> the APIs are kept similar. But that could be for later.

I have done this one.

I'll leave it at this for now and wait for some reviews.

  3. Re: pure parsers and reentrant scanners

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-12-16T08:40:57Z

    On 16/12/2024 09:39, Peter Eisentraut wrote:
> On 02.12.24 10:46, Peter Eisentraut wrote:
>> This patch series changes several parsers in the backend and contrib 
>> modules to use bison pure parsers and flex reentrant scanners. This is 
>> ultimately toward thread-safety, but I think it's also just nicer in 
>> general, and it might also fix a few possible small memory leaks.
> 
> I did a bit more work on this, so here is an updated patch set.

Looks good to me. There's more work to be done, but this is all good 
steps in the right direction.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  4. Re: pure parsers and reentrant scanners

    Andreas Karlsson <andreas@proxel.se> — 2024-12-17T00:46:18Z

    On 12/16/24 8:39 AM, Peter Eisentraut wrote:
> I'll leave it at this for now and wait for some reviews.

I really like this work since it makes the code cleaner to read on top 
of paving the way for threading.

Reviewed the patches and found a couple of issues.

- Shouldn't yyext in syncrep_scanner_init() be allocated on the heap? Or 
at least on the stack but by the caller?

- I think you have flipped the parameters of replication_yyerror(), see 
attached fixup patch.

- Some white space issues fixed in an attached fixup patch.

- Also fixed the static remaining variables in the replication parser in 
an attached patch.

- There seems to be a lot left to do to make the plpgsql scanner 
actually re-entrant so I do not think it would makes sense to commit the 
patch which sets the re-entrant option before that is done.

Andreas

  5. Re: pure parsers and reentrant scanners

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-18T09:42:37Z

    I started committing the cube and seg pieces.  There were a couple of 
complaints from the buildfarm, like

ccache clang -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith 
-Wdeclaration-after-statement -Werror=vla 
-Werror=unguarded-availability-new -Wendif-labels 
-Wmissing-format-attribute -Wcast-function-type -Wformat-security 
-Wmissing-variable-declarations -fno-strict-aliasing -fwrapv 
-Wno-unused-command-line-argument -Wno-compound-token-split-by-macro -g 
-O2 -fno-common -fsanitize=alignment -fsanitize-trap=alignment 
-Wno-deprecated-declarations -Werror  -fvisibility=hidden -I. -I. 
-I../../src/include  -isysroot 
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk 
-DWRITE_READ_PARSE_PLAN_TREES -DSTRESS_SORT_INT_MIN 
-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS 
-I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include 
-I/usr/local/include  -I/usr/local/ssl/include  -c -o segscan.o segscan.c
segscan.c:348:15: error: redefinition of typedef 'yyscan_t' is a C11 
feature [-Werror,-Wtypedef-redefinition]
typedef void* yyscan_t;
               ^
./segdata.h:19:15: note: previous definition is here
typedef void *yyscan_t;
               ^

I can fix that with the attached patch.

The symbol YY_TYPEDEF_YY_SCANNER_T isn't documented, but we already use 
it elsewhere in the code.

Note that in replication/syncrep.h and replication/walsender_private.h 
we have to have an #ifndef wrapper because there are files that end up 
including both headers.  Maybe we should put that #ifndef wrapper 
everywhere for consistency?

Any thoughts?

(Also, we should probably figure out a way to get these warnings before 
things hit the buildfarm.)

  6. Re: pure parsers and reentrant scanners

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-18T17:43:31Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> I started committing the cube and seg pieces.  There were a couple of 
> complaints from the buildfarm, like
> segscan.c:348:15: error: redefinition of typedef 'yyscan_t' is a C11 
> feature [-Werror,-Wtypedef-redefinition]
> typedef void* yyscan_t;
> ...
> (Also, we should probably figure out a way to get these warnings before 
> things hit the buildfarm.)

Interestingly, while sifaka shows that, its sibling indri doesn't.
Same compiler, same CFLAGS.  I think the relevant difference must
be that sifaka is using a much older Bison version (the Apple-supplied
2.3, versus MacPorts' up-to-the-minute version).  I think that sort of
thing is exactly why we have the buildfarm.  It would not be
reasonable to expect CI to cover that many cases.  Trying to do so
would just make CI slow enough that we'd start looking for a new test
phase to put in front of it.

			regards, tom lane

  7. Re: pure parsers and reentrant scanners

    Andreas Karlsson <andreas@proxel.se> — 2024-12-18T18:44:47Z

    On 12/18/24 10:42 AM, Peter Eisentraut wrote:
> I can fix that with the attached patch.
> 
> The symbol YY_TYPEDEF_YY_SCANNER_T isn't documented, but we already use 
> it elsewhere in the code.
> 
> Note that in replication/syncrep.h and replication/walsender_private.h 
> we have to have an #ifndef wrapper because there are files that end up 
> including both headers.  Maybe we should put that #ifndef wrapper 
> everywhere for consistency?
> 
> Any thoughts?

Seems like a sane fix to me and as for the ifndef I have no strong 
opinion either way but I would personally probably have added it 
everywhere for consistency.

Andreas

  8. Re: pure parsers and reentrant scanners

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-19T12:48:24Z

    On 17.12.24 01:46, Andreas Karlsson wrote:
> On 12/16/24 8:39 AM, Peter Eisentraut wrote:
>> I'll leave it at this for now and wait for some reviews.
> 
> I really like this work since it makes the code cleaner to read on top 
> of paving the way for threading.
> 
> Reviewed the patches and found a couple of issues.
> 
> - Shouldn't yyext in syncrep_scanner_init() be allocated on the heap? Or 
> at least on the stack but by the caller?

I think it's correct the way it is.  It's only a temporary space for the 
scanner, so we can allocate it in the innermost scope.

> - I think you have flipped the parameters of replication_yyerror(), see 
> attached fixup patch.

Good catch.  There was also a similar issue with syncrep_yyerror().

> - Some white space issues fixed in an attached fixup patch.

committed

> - Also fixed the static remaining variables in the replication parser in 
> an attached patch.

Thanks, I'll take a look at that.

> - There seems to be a lot left to do to make the plpgsql scanner 
> actually re-entrant so I do not think it would makes sense to commit the 
> patch which sets the re-entrant option before that is done.

Ok, we can hold that one back until the full stack including the parser 
is done.

  9. Re: pure parsers and reentrant scanners

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-19T12:51:13Z

    On 18.12.24 18:43, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> I started committing the cube and seg pieces.  There were a couple of
>> complaints from the buildfarm, like
>> segscan.c:348:15: error: redefinition of typedef 'yyscan_t' is a C11
>> feature [-Werror,-Wtypedef-redefinition]
>> typedef void* yyscan_t;
>> ...
>> (Also, we should probably figure out a way to get these warnings before
>> things hit the buildfarm.)
> 
> Interestingly, while sifaka shows that, its sibling indri doesn't.
> Same compiler, same CFLAGS.  I think the relevant difference must
> be that sifaka is using a much older Bison version (the Apple-supplied
> 2.3, versus MacPorts' up-to-the-minute version).  I think that sort of
> thing is exactly why we have the buildfarm.  It would not be
> reasonable to expect CI to cover that many cases.  Trying to do so
> would just make CI slow enough that we'd start looking for a new test
> phase to put in front of it.

The situation is that most current compilers default to some newer C 
standard version.  And so they won't complain about use of C11 features. 
  But the affected buildfarm members for whatever reason run with 
CC='clang -std=gnu99', and so they correctly reject C11 features.  We 
could do something similar in the Cirrus configuration.  I'll start a 
separate thread about that.

  10. Re: pure parsers and reentrant scanners

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-19T20:57:55Z

    On 19.12.24 13:48, Peter Eisentraut wrote:
> On 17.12.24 01:46, Andreas Karlsson wrote:
>> On 12/16/24 8:39 AM, Peter Eisentraut wrote:
>>> I'll leave it at this for now and wait for some reviews.
>>
>> I really like this work since it makes the code cleaner to read on top 
>> of paving the way for threading.
>>
>> Reviewed the patches and found a couple of issues.
>>
>> - Shouldn't yyext in syncrep_scanner_init() be allocated on the heap? 
>> Or at least on the stack but by the caller?
> 
> I think it's correct the way it is.  It's only a temporary space for the 
> scanner, so we can allocate it in the innermost scope.

>> - Also fixed the static remaining variables in the replication parser 
>> in an attached patch.
> 
> Thanks, I'll take a look at that.

I see what was going on here.  I was allocating yyext as a local 
variable in the init function and then it would go out of scope while 
the scanner is still in use.  That's why this didn't work for me.  I had 
written essentially the same patch as you for the replication scanner 
yyextra but with a local variable, and it was "mysteriously" failing the 
tests for me.  Your solution is better.  (For the jsonpath scanner, the 
local variable works because the scanner init and shutdown are called 
from the same function.)

Here is an updated patch set on top of what has been committed so far, 
with all the issues you pointed out addressed.

  11. Re: pure parsers and reentrant scanners

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-20T01:07:49Z

    I noticed that lapwing is bleating about

ccache gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 -fPIC -fvisibility=hidden -I. -I. -I../../src/include  -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS -D_GNU_SOURCE -I/usr/include/libxml2  -I/usr/include/et  -c -o cubescan.o cubescan.c
cubescan.c:1689:5: warning: no previous prototype for 'cube_yyget_column' [-Wmissing-prototypes]
cubescan.c:1765:6: warning: no previous prototype for 'cube_yyset_column' [-Wmissing-prototypes]

and likewise in segscan.c.  lapwing is using flex 2.5.35, so probably
this is the same bug worked around in parser/scan.l:

/*
 * Work around a bug in flex 2.5.35: it emits a couple of functions that
 * it forgets to emit declarations for.  Since we use -Wmissing-prototypes,
 * this would cause warnings.  Providing our own declarations should be
 * harmless even when the bug gets fixed.
 */
extern int	core_yyget_column(yyscan_t yyscanner);
extern void core_yyset_column(int column_no, yyscan_t yyscanner);

			regards, tom lane

  12. Re: pure parsers and reentrant scanners

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-20T14:29:40Z

    On 20.12.24 02:07, Tom Lane wrote:
> I noticed that lapwing is bleating about
> 
> ccache gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 -fPIC -fvisibility=hidden -I. -I. -I../../src/include  -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS -D_GNU_SOURCE -I/usr/include/libxml2  -I/usr/include/et  -c -o cubescan.o cubescan.c
> cubescan.c:1689:5: warning: no previous prototype for 'cube_yyget_column' [-Wmissing-prototypes]
> cubescan.c:1765:6: warning: no previous prototype for 'cube_yyset_column' [-Wmissing-prototypes]
> 
> and likewise in segscan.c.  lapwing is using flex 2.5.35, so probably
> this is the same bug worked around in parser/scan.l:

Ok, we can fix that, but maybe this is also a good moment to think about 
whether that is useful.  I could not reproduce the issue with flex 
2.5.39.  I could find no download of flex 2.5.35.  The github site only 
offers back to 2.5.39, the sourceforce site back to 2.5.36.  lapwing 
says it's Debian 7.0, which went out of support in 2016 and out of 
super-duper-extended support in 2020.  It also doesn't have a supported 
OpenSSL version anymore, and IIRC, it has a weird old compiler that 
occasionally gives bogus warnings.  I think it's time to stop supporting 
this.

  13. Re: pure parsers and reentrant scanners

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-20T15:23:33Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> On 20.12.24 02:07, Tom Lane wrote:
>> I noticed that lapwing is bleating about
>> cubescan.c:1689:5: warning: no previous prototype for 'cube_yyget_column' [-Wmissing-prototypes]
>> cubescan.c:1765:6: warning: no previous prototype for 'cube_yyset_column' [-Wmissing-prototypes]
>> and likewise in segscan.c.  lapwing is using flex 2.5.35, so probably
>> this is the same bug worked around in parser/scan.l:

> Ok, we can fix that, but maybe this is also a good moment to think about 
> whether that is useful.  I could not reproduce the issue with flex 
> 2.5.39.  I could find no download of flex 2.5.35.  The github site only 
> offers back to 2.5.39, the sourceforce site back to 2.5.36.  lapwing 
> says it's Debian 7.0, which went out of support in 2016 and out of 
> super-duper-extended support in 2020.  It also doesn't have a supported 
> OpenSSL version anymore, and IIRC, it has a weird old compiler that 
> occasionally gives bogus warnings.  I think it's time to stop supporting 
> this.

OK, that's fair.  I do see lapwing called out a lot in the commit log,
though it's not clear how much of that is about 32-bitness and how
much about old tools.  It's surely still valuable to have i386
machines in the buildfarm, but I agree that supporting unobtainable
tool versions is a bit much.  Could we get that animal updated to
some newer OS version?

Presumably, we should also rip out the existing yyget_column and
yyset_column kluges in

src/backend/parser/scan.l: extern int      core_yyget_column(yyscan_t yyscanner);
src/bin/psql/psqlscanslash.l: extern int   slash_yyget_column(yyscan_t yyscanner);
src/bin/pgbench/exprscan.l: extern int     expr_yyget_column(yyscan_t yyscanner);
src/fe_utils/psqlscan.l: extern int        psql_yyget_column(yyscan_t yyscanner);

			regards, tom lane

  14. Re: pure parsers and reentrant scanners

    Julien Rouhaud <rjuju123@gmail.com> — 2024-12-20T15:30:17Z

    On Fri, Dec 20, 2024 at 11:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Could we get that animal updated to
> some newer OS version?

There is already adder animal that is running debian sid on i386.  The
only remaining interest in lapwing is to have older versions of
everything, so if that's useless I can just trash that vm.

  15. Re: pure parsers and reentrant scanners

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-20T15:35:01Z

    Julien Rouhaud <rjuju123@gmail.com> writes:
> On Fri, Dec 20, 2024 at 11:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Could we get that animal updated to
>> some newer OS version?

> There is already adder animal that is running debian sid on i386.  The
> only remaining interest in lapwing is to have older versions of
> everything, so if that's useless I can just trash that vm.

Hmm, sid is the opposite extreme no?  Maybe switching lapwing to
whatever is currently the oldest supported Debian release would
be a good answer.

			regards, tom lane

  16. Re: pure parsers and reentrant scanners

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-21T10:47:06Z

    On 20.12.24 16:35, Tom Lane wrote:
> Julien Rouhaud <rjuju123@gmail.com> writes:
>> On Fri, Dec 20, 2024 at 11:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Could we get that animal updated to
>>> some newer OS version?
> 
>> There is already adder animal that is running debian sid on i386.  The
>> only remaining interest in lapwing is to have older versions of
>> everything, so if that's useless I can just trash that vm.
> 
> Hmm, sid is the opposite extreme no?  Maybe switching lapwing to
> whatever is currently the oldest supported Debian release would
> be a good answer.

Yeah, Debian stable or oldstable on i386 could certainly be useful.

  17. Re: pure parsers and reentrant scanners

    Andreas Karlsson <andreas@proxel.se> — 2024-12-22T21:43:38Z

    On 12/19/24 9:57 PM, Peter Eisentraut wrote:
> Here is an updated patch set on top of what has been committed so far, 
> with all the issues you pointed out addressed.

Other than the discussion of how old versions of flex we should support 
I think this set of patches is ready to be committed. I looked at it 
again and everything looks good.

Andreas

  18. Re: pure parsers and reentrant scanners

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-26T18:27:26Z

    On 22.12.24 22:43, Andreas Karlsson wrote:
> On 12/19/24 9:57 PM, Peter Eisentraut wrote:
>> Here is an updated patch set on top of what has been committed so far, 
>> with all the issues you pointed out addressed.
> 
> Other than the discussion of how old versions of flex we should support 
> I think this set of patches is ready to be committed. I looked at it 
> again and everything looks good.

I have committed these except the plpgsql one, which was still work in 
progress.  But I have progressed on this now and also converted the 
parser and put the local state into yyextra.  This gets rid of all 
internal global state now.  The patches for this are attached.  It's a 
lot of churn, but otherwise pretty standard stuff.

Along the way I noticed that the flex documentation now recommends a 
different way to set the yyextra type.  So I have changed the ones we 
already have to that newer style.  I inspected the generated C code and 
there wasn't any significant difference, so I'm not sure, but I figure 
if we're making changes in this area we might as well use the modern style.

  19. Re: pure parsers and reentrant scanners

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-12-27T09:19:27Z

    On 26/12/2024 20:27, Peter Eisentraut wrote:
> On 22.12.24 22:43, Andreas Karlsson wrote:
>> On 12/19/24 9:57 PM, Peter Eisentraut wrote:
>>> Here is an updated patch set on top of what has been committed so 
>>> far, with all the issues you pointed out addressed.
>>
>> Other than the discussion of how old versions of flex we should 
>> support I think this set of patches is ready to be committed. I looked 
>> at it again and everything looks good.
> 
> I have committed these except the plpgsql one, which was still work in 
> progress.  But I have progressed on this now and also converted the 
> parser and put the local state into yyextra.  This gets rid of all 
> internal global state now.  The patches for this are attached.  It's a 
> lot of churn, but otherwise pretty standard stuff.

Looks good to me.

> Along the way I noticed that the flex documentation now recommends a 
> different way to set the yyextra type.  So I have changed the ones we 
> already have to that newer style.  I inspected the generated C code and 
> there wasn't any significant difference, so I'm not sure, but I figure 
> if we're making changes in this area we might as well use the modern style.

+1. According to the flex NEWS file, this syntax was added in flex 
2.5.34, and we already require 2.5.35.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  20. Re: pure parsers and reentrant scanners

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-08T09:53:57Z

    On 27.12.24 10:19, Heikki Linnakangas wrote:
> On 26/12/2024 20:27, Peter Eisentraut wrote:
>> On 22.12.24 22:43, Andreas Karlsson wrote:
>>> On 12/19/24 9:57 PM, Peter Eisentraut wrote:
>>>> Here is an updated patch set on top of what has been committed so 
>>>> far, with all the issues you pointed out addressed.
>>>
>>> Other than the discussion of how old versions of flex we should 
>>> support I think this set of patches is ready to be committed. I 
>>> looked at it again and everything looks good.
>>
>> I have committed these except the plpgsql one, which was still work in 
>> progress.  But I have progressed on this now and also converted the 
>> parser and put the local state into yyextra.  This gets rid of all 
>> internal global state now.  The patches for this are attached.  It's a 
>> lot of churn, but otherwise pretty standard stuff.
> 
> Looks good to me.
> 
>> Along the way I noticed that the flex documentation now recommends a 
>> different way to set the yyextra type.  So I have changed the ones we 
>> already have to that newer style.  I inspected the generated C code 
>> and there wasn't any significant difference, so I'm not sure, but I 
>> figure if we're making changes in this area we might as well use the 
>> modern style.
> 
> +1. According to the flex NEWS file, this syntax was added in flex 
> 2.5.34, and we already require 2.5.35.

These have been committed.  This concludes the main body of this work.

I'll let it take a lap around the buildfarm and will follow up in a few 
days on what if anything lapwing has to say about it in terms of 
warnings and what we want to do about it.

  21. Re: pure parsers and reentrant scanners

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-09T07:55:29Z

    On 20.12.24 16:23, Tom Lane wrote:
>> Ok, we can fix that, but maybe this is also a good moment to think about
>> whether that is useful.  I could not reproduce the issue with flex
>> 2.5.39.  I could find no download of flex 2.5.35.  The github site only
>> offers back to 2.5.39, the sourceforce site back to 2.5.36.  lapwing
>> says it's Debian 7.0, which went out of support in 2016 and out of
>> super-duper-extended support in 2020.  It also doesn't have a supported
>> OpenSSL version anymore, and IIRC, it has a weird old compiler that
>> occasionally gives bogus warnings.  I think it's time to stop supporting
>> this.
> 
> OK, that's fair.  I do see lapwing called out a lot in the commit log,
> though it's not clear how much of that is about 32-bitness and how
> much about old tools.  It's surely still valuable to have i386
> machines in the buildfarm, but I agree that supporting unobtainable
> tool versions is a bit much.  Could we get that animal updated to
> some newer OS version?
> 
> Presumably, we should also rip out the existing yyget_column and
> yyset_column kluges in
> 
> src/backend/parser/scan.l: extern int      core_yyget_column(yyscan_t yyscanner);
> src/bin/psql/psqlscanslash.l: extern int   slash_yyget_column(yyscan_t yyscanner);
> src/bin/pgbench/exprscan.l: extern int     expr_yyget_column(yyscan_t yyscanner);
> src/fe_utils/psqlscan.l: extern int        psql_yyget_column(yyscan_t yyscanner);

All my flex-related patches are in now.

Here is a patch that removes the workarounds for compiler warnings with 
flex 2.5.35.  This ended up being a whole lot, including the whole 
fix-old-flex-code.pl script.

The second patch contemplates raising the minimum required flex version, 
but what to?

The most recent incrementing was exactly because 2.5.35 was the oldest 
in the buildfarm.  The previous incrementings were apparently because 
certain features were required or some bugs had to be avoided.

Options:

- Leave at 2.5.35 as long as it's present in the buildfarm.

- Set to 2.5.36 because it's the oldest that compiles without warnings. 
Also, the oldest you can still download from the flex sourceforge site.

- Set to 2.5.37 because that's the next oldest in the buildfarm (for 
CentOS/RHEL 7, so it will stay around for a while).

- Set to 2.5.34 because that's the oldest we actually require as of 
commit b1ef48980dd.

- Remove version check, because these are all so old that no one cares 
anymore.

  22. Re: pure parsers and reentrant scanners

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-09T14:53:03Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> The second patch contemplates raising the minimum required flex version, 
> but what to?

Meh, let's just rip out the version check.  It's no longer very
relevant.  Nobody is going to be using anything older than 2.5.35.
While 2.5.35 produces compile warnings, it does still work, so
rejecting it with a changed version check seems unnecessary.

			regards, tom lane

  23. Re: pure parsers and reentrant scanners

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-17T15:35:55Z

    On 09.01.25 15:53, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> The second patch contemplates raising the minimum required flex version,
>> but what to?
> 
> Meh, let's just rip out the version check.  It's no longer very
> relevant.  Nobody is going to be using anything older than 2.5.35.
> While 2.5.35 produces compile warnings, it does still work, so
> rejecting it with a changed version check seems unnecessary.

This has been done.

  24. Re: pure parsers and reentrant scanners

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-17T15:40:54Z

    Here are some more patches.  This should cover the last sub-topic of 
this topic: not passing the yyparse() result via global variables.  This 
uses techniques that are already in use in some parsers in the tree, for 
example cube and jsonpath.  The syncrep parser was a bit trickier, 
because there we need to pass the syncrep_parse_error variable all the 
way down to the scanner (not just the parser), but overall it's all 
still pretty compact and standard.

  25. Re: pure parsers and reentrant scanners

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-24T06:46:48Z

    On 17.01.25 16:40, Peter Eisentraut wrote:
> Here are some more patches.  This should cover the last sub-topic of 
> this topic: not passing the yyparse() result via global variables.  This 
> uses techniques that are already in use in some parsers in the tree, for 
> example cube and jsonpath.  The syncrep parser was a bit trickier, 
> because there we need to pass the syncrep_parse_error variable all the 
> way down to the scanner (not just the parser), but overall it's all 
> still pretty compact and standard.

This has been committed.

I think this concludes this topic.

For those who haven't seen it, I wrote a blog post about this: 
https://peter.eisentraut.org/blog/2025/01/21/implementing-thread-safe-scanners-and-parsers-in-postgresql