Re: pure parsers and reentrant scanners

Peter Eisentraut <peter@eisentraut.org>

From: Peter Eisentraut <peter@eisentraut.org>
To: Andreas Karlsson <andreas@proxel.se>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2024-12-19T20:57:55Z
Lists: pgsql-hackers

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

Attachments

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.