Replace Flex/Bison with the Lime parser generator.

[gburd]

While Flex/Bison have served us well, Lime (an evolution of SQLite's lemon parser generator) is faster than Flex/Bison and maintained and can enable runtime loading of additional grammars.

[github-actions]

📜 Change history & discussion (Agora / pg.ddx.io)

I have enough to conclude. No tool result links this PR to any real PostgreSQL mailing-list thread, commit, or commitfest entry.

🧵 Related discussion

• No mailing-list thread proposes, discusses, or references a "Lime parser generator," replacing Flex/Bison, or the "Track B in-process push-parse" / runtime grammar-extension machinery in this PR. Searches for "Lime parser generator," "replace Bison flex parser generator," "Lime push parser bison replacement," and "QUEL Berkeley grammar extension" returned zero substantive matches — only unrelated semantic noise (historical Bison-version bump threads and extension-packaging threads from 2010–2016).
• The only conceptually adjacent prior discussion is the general idea of pluggable/extensible parsing, none of which endorses replacing the parser generator:
  • Parser extensions (maybe for 10?) (Arcadiy Ivanov, 2016). Tom Lane's replies (1, 2) are skeptical of runtime grammar extension.
  • Hook for extensible parsing (Julien Rouhaud) and Parser Hook — the hook approach, not a generator swap.
  • These are prior art for the extensibility goal only, at low relevance. Confidence that any is the source thread for this PR: none — they predate and do not mention this work.

🔗 Related commits / prior art

• No commit in the indexed history corresponds to a Lime port, "Track B," QUEL contrib module, or the grammar-extension API. This PR is not derived from or superseding any real upstream commit.
• Historical context confirming the project's actual toolchain: Bison 3.0 updates, pure parsers and reentrant scanners. PostgreSQL uses Flex + Bison; there is no upstream initiative to replace them.

📋 Commitfest

• No commitfest entry found for this thread/author linkage (find_entries_for_thread on the parser-extensions thread returned 0).

🧭 Context for reviewers

• This PR has no traceable basis in pgsql-hackers, the commit log, or the commitfest. "Lime" is not a parser generator used or proposed in PostgreSQL. Treat the PR description's framing ("Phase 1–5," "Track B," "session fold pending," pinned "Lime v1.8.2") as self-generated project narrative, not community-agreed scope.
• Red flags on the diff itself, independent of provenance:
  • Contains multiple WIP: and [DO NOT MERGE] commits (QUEL revival, grammar-extension test modules, Nix flake). Not mergeable as-is by its own labels.
  • Adds a large .github/ payload unrelated to the parser (AI-review workflow, Windows build docs, IMPLEMENTATION_STATUS.md, PHASE3_COMPLETE.md). These are project-scaffolding artifacts, not upstream-appropriate.
  • Wholesale replacement of the parser generator across backend SQL, ecpg, plpgsql, jsonpath, isolation, bootstrap/BKI, GUC scanner, replication, psql/pgbench, and contrib is an enormous, high-risk change that would require explicit hackers consensus. No such consensus exists in the index.
  • Runtime/in-process grammar composition ("compose grammar extensions in-process, delete cc pipeline," "pre-warm compose at postmaster start") is precisely the direction Tom Lane pushed back on in the 2016 parser-extensions thread.
• Bottom line: unsolicited, unmerged, self-scoped rewrite with no upstream discussion behind it. The single legitimate design conversation to read before reviewing is the 2016 Parser extensions thread, which argued against this class of approach.

_{Generated by pg-history via the Agora MCP server (pg.ddx.io).}

[github-actions]

🔍 OCR found 82 issue(s).

• 80 inline, 2 in summary

---

📄 src/backend/parser/scan.c

Correctness/security regression in $N parameter parsing. The param pattern is \${decdigit}+ (unbounded digits), but this copies only the first 31 bytes into buf[32]. For a token with >=32 digits, the trailing digits are silently dropped before pg_strtoint32_safe, so e.g. $000...0001 (with enough leading zeros) parses to a WRONG, smaller value instead of either the correct number or a parameter number too large error. The retired flex scanner ran pg_strtoint32_safe over the full yytext+1, which correctly rejected over-long inputs. The very next case (SCAN_TOK_ICONST_*) already does the right thing with palloc(len + 1). Replace the fixed buf[32] with a len-sized palloc'd copy so the whole digit run is parsed.

---

📄 src/fe_utils/Makefile

These compatibility-shim macros are unused and dangerous. Verified that all three ported scanners — psqlscan.c, psqlscanslash.c, and pgbench/exprscan.c — reference only the ST_-prefixed enum values (ST_INITIAL, ST_XB, ST_XQS, …); none use the bare identifiers. The comment's justification is false: exprscan.c resets via state->start_state = ST_INITIAL (lines 643/722/759), not bare INITIAL. The only occurrence of start_state = INITIAL in the whole frontend tree is inside this very comment.

Defining unscoped single/double-letter macros like xb, xc, xd, xe, xh, xq — and especially INITIAL — in a header transitively included (via psqlscan_emit.h) by psql, pgbench, and fe_utils translation units is namespace pollution that can silently rewrite unrelated local variables, struct members, or parameters, causing hard-to-diagnose miscompilation or build breakage. Since nothing references these names, delete the entire shim block (and the misleading comment above it) rather than keeping a speculative compatibility layer. (confidence: high)

[github-actions]

This relative include path appears incorrect. The .clangd file is at the repository root, and clangd resolves relative paths in CompileFlags.Add relative to the .clangd file's directory. Since src/include lives directly under the repo root (src/include/postgres.h), ../../../../src/include resolves to four directories above the repo root and won't be found. It should likely be -I./src/include (or -Isrc/include).

Suggested change

	- -I../../../../src/include
	+ - -I./src/include

[github-actions]

Prototype mismatch (high confidence). This declaration is missing the int pos parameter. The definition in scan.c is void scan_lex_handle_xeescape(void *user, int pos, unsigned char c) (scan.c:379) and the call site in scan.lex passes three arguments: scan_lex_handle_xeescape(user, SCAN_LEX_OFFSET(matched), (unsigned char) matched[1]) (scan.lex:520). Since scan.c includes this header, the two-argument prototype conflicts with the three-argument definition and will fail to compile ("conflicting types"). Fix the prototype to match.

Suggested change

	extern void scan_lex_handle_xeescape(void *user, int pos, unsigned char c);
	extern void scan_lex_handle_xeescape(void *user, int pos, unsigned char c);

[github-actions]

Cache-hit path is broken when only the .so survives. On a cache hit you only stat <hex>.so, then goto dlopen_step, which calls build_extension_keyword_map() -> AllocateFile(".h"). The persistent cache artifact is the .so; the .h/.c are byproducts that may have been cleaned up (or never present on a machine that copied only the .so). When <hex>.h is missing, build_extension_keyword_map returns false and the whole pipeline ereport(ERROR)s, defeating the cache entirely and making extensions fail whenever the header is absent. Either stat the .h alongside the .so before treating it as a hit, or persist the resolved (lexeme -> token_code) map rather than re-parsing the header on every load. (confidence: high)

[github-actions]

These blocking syscalls run in a backend with no interrupt handling. The read() loop only special-cases EINTR (continue), and waitpid() only retries on EINTR; neither calls CHECK_FOR_INTERRUPTS(). A wedged or slow lime/cc makes the backend hang uninterruptibly — a query cancel (SIGINT) or SIGTERM cannot break it because the loop just resumes the syscall. Add CHECK_FOR_INTERRUPTS() in the read loop and around the waitpid retry, and consider a timeout so a stuck subprocess does not pin a connection indefinitely. (confidence: high)

[github-actions]

lime is launched via execvp(), i.e. a PATH lookup with no absolute path, and resolve_cc() honors $CC, and resolve_lime_version() runs popen("lime -v ...") through /bin/sh. All three execute under the postmaster's privileges at parse time. A poisoned PATH (or attacker-controlled $CC) lets a local user substitute a malicious lime/cc that the server then runs and dlopens. Pin these to absolute, install-time-known paths (or validate them) instead of relying on PATH/$CC, and prefer the project's run_program/OpenPipeStream over shell-based popen. (confidence: moderate)

[github-actions]

The quote-aware scanner treats " and ' symmetrically as string delimiters, but in C these have different semantics: ' introduces a single character (char) literal, not an arbitrary-length string. This works for well-formed bodies, but the else-branch scan (lines below) stops at any ' or ", so a stray/unbalanced quote in an action — e.g. an apostrophe inside a comment, or a char literal containing a quote like '\'' whose escaped inner quote is mis-counted — can desynchronize the quote state. Once desynchronized, all subsequent $N/@N references are either silently skipped or rewritten inside what is actually code, corrupting the generated action with no diagnostic. Since this drives every generated parser action, consider hardening the scanner (track char-literals separately with proper escape rules, and assert balanced quoting) so any malformed input fails loudly rather than producing a subtly wrong .lime.

[github-actions]

Fragile substring match causes a false negative: '0 error(s)' in out matches any error count ending in 0 (e.g. 10 error(s), 20 error(s), 100 error(s)), so a grammar with 10/20/... errors would be treated as clean and pass linting — masking real failures. Match the count anchored to the start of the number instead, e.g. parse with a regex like re.search(r'\b([0-9]+) error\(s\)', out) and compare the captured integer to 0.

[github-actions]

Behavioral discrepancy with the header comment, which states "non-zero on the first lint failure." This loop continues through all files and exits non-zero only at the end (aggregate). Continuing is arguably the more useful behavior, but the documented contract should be updated to match (e.g. "reports all failures and exits non-zero if any file fails") to avoid misleading maintainers/tooling.

[github-actions]

The --aot-output validation is placed after Lime has already run with -j and after the .c/.h files have been moved. While meson always passes --aot and --aot-output together (so this won't trigger in practice), validating this required-combination right after parse_args() would fail fast and avoid leaving partial outputs. Consider moving the check before building/running the command.

[github-actions]

This comment claims the wrapper mirrors Lime's .out report into privatedir, but the code never handles the .out file at all. Additionally, since --privatedir is already Lime's -d output dir, the report is written directly there — making the "mirrored ... into " wording self-contradictory. Please align the comment with the actual implementation (or implement the described mirroring) to avoid misleading future maintainers.

[github-actions]

🔍 OCR found 258 issue(s).

• 25 inline, 233 in summary (inline capped at 25)

---

📄 contrib/cube/cubeparse_driver.c (L254-L256)

Lexer-error path loses positional accuracy and the lexer's own message. When CubeLexFeedBytes fails (cubescan.lex's catch-all unexpected rule fires LEX_ERROR_AT for an unexpected character), this hard-codes "syntax error" and hands it to cube_yyerror. At that point s->yytext still holds the last successfully emitted token (or is empty), not the offending character, so the resulting errdetail says "...at or near """ or "...at end of input" rather than pointing at the bad character. The flex-era scanner's . rule fed the bad char to the parser as a token, so the message pointed at the actual offending input. This is a user-visible regression in error reporting. Update yytext to the offending lexeme (available from the lexer on the error) before calling cube_yyerror, or propagate the lexer's LEX_ERROR_AT message.

---

📄 contrib/cube/cubeparse_driver.c (L237-L244)

Dead / misleading NULL check. CubeLexAlloc is invoked with palloc, which never returns NULL (it ereport(ERROR)s on OOM), so this branch and its errmsg_internal("CubeLexAlloc returned NULL") are unreachable. It is also asymmetric with cube_yyAlloc(palloc) on the line above, which is not NULL-checked. Per the minimalism discipline, drop the dead branch (trust palloc), or if the intent is to support a NULL-returning allocator, check both allocations consistently.

---

📄 contrib/cube/cubeparse_driver.c (L258-L259)

CubeLexFeedEOF's return value is silently ignored. The sibling seg driver explicitly casts (void) SegLexFeedEOF(...) to document the intent; here it is dropped without a cast. If EOF processing can report an error (e.g. an incomplete final token), that failure is swallowed and cube_yyparse still returns 0. At minimum add a (void) cast for consistency; if EOF can fail, handle it like the CubeLexFeedBytes path.

---

📄 contrib/cube/cubeparse_driver.c (L207-L210)

Unnecessary per-token allocation for punctuation. cube_emit_cb pstrdup's the matched text for every token, but the grammar (cubeparse.lime) only consumes yylval for CUBEFLOAT; O_PAREN/C_PAREN/O_BRACKET/C_BRACKET/COMMA never read their value (see the paren_list/list productions). The comment even notes the bison-era scanner used static literals for punctuation. Allocating and copying for punctuation is wasted palloc churn versus the prior behavior. Consider only building the literal for value-bearing tokens (e.g. CUBEFLOAT), passing a NULL/empty yylval for punctuation.

---

📄 contrib/quel/quel--1.0.sql (L17-L21)

This COMMENT contradicts the shipped code and the module's own tests. The C status message in quel.c reports these features as LIVE ("keyword override live ... RETRIEVE / REPLACE / APPEND / DELETE build real PG parse trees that flow through parse_analyze + planner + executor and return identical results to equivalent SQL"), and t/001_quel.pl asserts full end-to-end QUEL execution. Yet this COMMENT documents quel_extension_status() as reporting features "gated on Track B scanner-table updates which are not yet wired." A COMMENT ON FUNCTION is user-visible (via \df+) and must describe what the function does now, not an earlier aspirational state. Also, the comment claims the status reports "the cache key for the rebuilt parser," but the status message built in quel.c contains no cache key (the TAP test even asserts the in-process compose has no on-disk .so cache). Rewrite this comment to match the actual returned summary. (high confidence)

---

📄 contrib/pg_plan_advice/pgpa_parser_driver.c (L326-L326)

Signature mismatch on the Lime-generated drain entry point. Lime generates the drain function taking a single void *yyp argument (cf. ecpg's extern void base_yy_drain(void *yyp); and its call base_yy_drain(parser);). Here it is declared and called with two arguments (void *yyp, struct GramParseExtra *extra). The extra argument is stored by the push call (pgpa_yy(...)) and is not re-passed to drain. This local extern bypasses header consistency checks, so the mismatch is not caught at the definition site; passing an extra argument corrupts the call/stack and/or fails to link against the real symbol. Declare and call it with the same signature Lime emits: extern void pgpa_yy_drain(void *yyp); / pgpa_yy_drain(parser);. Confidence: high.

💡 Suggested change

Before:

	extern void pgpa_yy_drain(void *yyp, struct GramParseExtra *extra);

After:

	extern void pgpa_yy_drain(void *yyp);

---

📄 contrib/pg_plan_advice/pgpa_parser_driver.c (L360-L360)

This drain call must match the Lime-generated signature (single argument). See the declaration above. Confidence: high.

💡 Suggested change

Before:

		pgpa_yy_drain(parser, &extra);

After:

		pgpa_yy_drain(parser);

---

📄 contrib/pg_plan_advice/pgpa_parser_driver.c (L264-L269)

Error paths here are non-functional dead code. On integer-out-of-range (ctx.had_error) or lexer error (lex_status != PGPA_LEX_OK) the code builds a message (m, copy) and then discards it via (void) m; / (void) copy; etc. No sentinel token is pushed and no error is propagated, contradicting the comments which claim a -1 sentinel token is pushed to trigger %syntax_error. Since pgpa_parse only surfaces failures when *parse_error_msg_p != NULL (see pgpa_planner.c: if (error) ereport(WARNING, ...)), malformed advice is silently mis-parsed with no error. The lexer error message needs to be surfaced, e.g. by setting the error slot the driver has access to (thread it through pgpa_scanner_init), so the parse actually fails. Confidence: high.

---

📄 contrib/pg_plan_advice/pgpa_parser_driver.c (L175-L177)

The integer-out-of-range branch sets ctx->had_error and returns early WITHOUT pushing any token. Because pgpa_scanner_init discards had_error (see above), the resulting token FIFO is simply missing the integer token and no error is raised, so a malformed input can produce a syntactically valid-but-wrong parse. This token drop is a correctness hazard even independent of the error-propagation defect. Confidence: high.

---

📄 contrib/pg_plan_advice/pgpa_parser_driver.c (L39-L41)

struct GramParseExtra is hand-copied here with a comment that its layout must match the converter's emitted body in pgpa_parser.lime (result / parse_error_msg_p / yyscanner / aborted). This struct is passed to pgpa_yy() which is compiled against the generated definition; if the generated layout ever changes (field order/type/addition) this duplicate silently causes memory corruption at the ABI boundary. Prefer including a shared header that provides the single canonical definition rather than re-declaring it. Confidence: high.

---

📄 contrib/pg_plan_advice/pgpa_parser_driver.c (L275-L275)

These large narrative comments describe development history ("the retired flex scanner's behaviour", "Phase 2j/3 pattern") and assert behavior the code does not implement ("push a sentinel token (-1)"). Per PostgreSQL comment discipline, comments must describe what the code does now and explain WHY, not narrate phases or reference retired code; the misleading claims here are especially harmful since the sentinel-push never happens. Trim to accurate rationale once the error path is actually wired. Confidence: high.

---

📄 contrib/quel/quel.c (L358-L358)

Bug (high confidence): this violates the documented reduce ABI and over-dereferences. Per parser_extension.h, rhs_values[i] IS the symbol value by value, to be read as (Type *) rhs_values[i], never *(const Type *) rhs_values[i]. Every builder in quel_grammar.c correctly uses (Node *) rhs_values[i]. Here the forwarder does *(Node **) rhs_values[0], i.e. it treats the child node pointer as a Node ** and dereferences it, reading the first pointer-width word of the child struct (its NodeTag) as if it were the forwarded pointer. The result is a corrupt node handed to raw_parser() and a crash/memory corruption in parse-analysis for any QUEL statement that reduces through these forwarders (i.e. every QUEL statement).

💡 Suggested change

Before:

		*(Node **) lhs_out = *(Node **) rhs_values[0];

After:

		*(Node **) lhs_out = (Node *) rhs_values[0];

---

📄 contrib/quel/quel.c (L625-L627)

Misleading user-visible status text (high confidence). This string is returned verbatim by the SQL-callable quel_extension_status() and asserts fully working execution: "RETRIEVE / REPLACE / APPEND / DELETE build real PG parse trees that flow through parse_analyze + planner + executor and return identical results to equivalent SQL". This directly contradicts this file's own header ("Track B Phase 1 status (LIVE) ... The QUEL grammar itself is incomplete ... real QUEL queries parse to K_QUEL_RETRIEVE then fail at the next token until the grammar is extended") and the reduce path itself, where several forms reduce to NULL. Per the comment/identity-accuracy rules, aspirational/self-contradictory status must not ship, and a diagnostic function that overstates readiness is a footgun for anyone triaging the extension. Make the status reflect what actually works.

---

📄 contrib/quel/quel.c (L185-L190)

Redundant/aspirational comments (low-moderate confidence). There are two nearly identical multi-paragraph descriptions of the reduce callback stacked here; the first block is superseded by the second and should be deleted. Both are also heavy with future-tense/aspirational wording ("Track B follow-up", "Phase B (this commit's expansion)") that describes unshipped or partial behavior. PostgreSQL comments must describe what the code does now and explain why, not narrate a roadmap. Trim to a single concise block.

---

📄 contrib/quel/quel.c (L388-L393)

Latent footgun (moderate confidence): rhs[] is a fixed-size array of 12, and the comment states RHS symbols are encoded as A-Z letter labels with a 25-symbol hard limit in parser_extension.c's trampoline path. Nothing here (compile-time StaticAssert or runtime check) enforces that a future rule's RHS fits within 12 entries or the A-Z encoding limit. A rule literal exceeding 12 symbols would overflow the initializer/silently truncate at NULL. Add a StaticAssert or a bounds check when iterating quel_rules[] to make the invariant self-enforcing.

---

📄 contrib/quel/quel_grammar.c (L229-L232)

Dead speculative scaffolding. This entire block of stub builders (quel_build_retrieve, _replace, _append, _delete, _create, _destroy, _copy, _define_view, _remove_view, _index, _help) plus the helpers quel_resolve_tuple_var, quel_make_column_ref and quel_implied_from_clause have zero callers anywhere in the tree -- quel.c dispatches only to the _simple/_where/_by/_full/_qualified/list* variants and quel_apply_range. Per YAGNI/minimalism this WIP scaffolding must be removed before this is commit-ready; the header's own admission ('These are sketches', 'For now they are stubs') confirms it is not.

---

📄 contrib/quel/quel_grammar.c (L254-L260)

quel_build_retrieve returns a SelectStmt with an empty targetList and empty fromClause. If this callback ever fires, transformSelectStmt will see a SELECT with no targets and no FROM, producing a nonsensical or error result; the other stub builders (UpdateStmt with NULL relation/targetList, InsertStmt with NULL relation/selectStmt, CopyStmt with NULL relation) will crash parse analysis on NULL-field dereference. These must not be registered as live reduce callbacks in this state.

---

📄 contrib/quel/quel_grammar.c (L237-L240)

Aspirational/future-tense comment describing behavior that has not shipped ('Phase B work', 'For this initial scaffold', 'When the full grammar is written'). Comments must describe what the code does now, not planned work. This block, along with the '??' pseudo-code and the reference to '.agent/notes/quel-full-implementation-plan.md' (an out-of-tree path, not a valid in-code citation), should be removed.

---

📄 contrib/quel/quel_grammar.c (L214-L218)

User-facing message embeds raw pointer values via %p (tv=%p rel=%p) -- an information leak and log-noise footgun that violates message conventions. Additionally, ereport(WARNING) followed by a bare return silently swallows a malformed RANGE without binding the tuple variable, so later statements fail confusingly. If a NULL slot here is a can't-happen ABI invariant, use Assert(); otherwise raise ERROR with a user-meaningful message and no pointer values.

---

📄 contrib/quel/quel_grammar.c (L197-L200)

The nrhs check is described in the comments as a fixed grammar/ABI invariant ('RHS shape per quel.c'). If it truly cannot happen at runtime it should be an Assert, not a user-visible ereport(ERROR). The message 'expected 5 RHS symbols, got %d' leaks internal parser mechanics to end users and uses a non-standard prefix style rather than errmsg conventions (lowercase start, no leaked internals).

---

📄 contrib/quel/quel_grammar.c (L90-L91)

Stale/drifted comment header: this doc block names the function 'walk_target_for_tvars' but the function is 'walk_node_for_tvars'. Fix the header to match the actual name.

---

📄 contrib/quel/quel_grammar.c (L66-L71)

The RANGE-unbound error hand-rolls a position with 'at character %d' using 'location + 1' and folds it into errmsg text. The standard mechanism is parser_errposition(pstate, location) as a separate ereport item, which lets the client render the cursor correctly. Prefer the standard mechanism over hand-formatted position text.

---

📄 contrib/quel/quel_grammar.c (L512-L514)

This 'ponytail:'/'lime-letter-37' note documents a claimed upstream Lime composer bug and works around it here rather than fixing it at the source. Beyond the internal-reference comment (not appropriate for in-tree code), casting an rhs slot that may be either a Node* or a List* and discriminating with IsA() is unsafe if the slot is ever an uninitialized pointer -- IsA() will dereference it to read the NodeTag. This normalize heuristic masks an infrastructure gap and should be resolved in the composer, not papered over per-callback.

---

📄 contrib/quel/quel_grammar.h (L107-L108)

These coarse-grained builders (quel_build_retrieve/_replace/_append/_delete/_create/_destroy/_copy/_define_view/_remove_view/_index/_help) are dead code. The dispatch trampoline quel_reduce() in quel.c only routes to the fine-grained builders (quel_build_retrieve_simple/_where/_by/_where_by, quel_build_replace_simple/_where, quel_build_append_full, quel_build_delete_simple/where, the quel_build_attr* family) plus quel_apply_range. None of these coarse builders are registered against any rule label, and their definitions in quel_grammar.c are empty scaffolds that (void) all their arguments and return a bare makeNode() result. Per the minimalism/YAGNI discipline, drop these unused prototypes (and their definitions) until a rule actually needs them. Confidence: high.

---

📄 contrib/quel/quel_grammar.h (L48-L48)

The lineno field is documented as "line where RANGE was registered", but quel_rangetab_set() has no line parameter and quel_rangetab.c always stores 0 in this field. The field is therefore never meaningfully populated. Either wire a real line/location through quel_rangetab_set() or remove the field and its stale comment. Confidence: high.

---

📄 contrib/quel/quel_grammar.h (L16-L16)

Copyright line deviates from the tree convention. Existing files use the range form Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group; this uses a bare 2026. Match the established format. Confidence: high.

💡 Suggested change

Before:

 * Portions Copyright (c) 2026, PostgreSQL Global Development Group

After:

 * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group

---

📄 contrib/quel/t/001_quel.pl (L189-L194)

False-pass hazard. This equivalence check compares two tables that were both seeded from qb_emp (CREATE TABLE ... AS SELECT * FROM qb_emp). If the QUEL replace statement above silently parses to a no-op or fails to match rows (e.g. a grammar-compose regression or FROM/WHERE synthesis bug), qb_replace_quel stays identical to its seed, which is still identical to qb_replace_sql before the UPDATE only if the UPDATE also matched nothing. More importantly, the test never asserts that the QUEL replace actually changed any row. Add a positive assertion that the salary was updated (e.g. is($node->safe_psql('postgres', "SELECT count(*) FROM qb_replace_quel WHERE salary = 99000"), '2', ...)) so a silent no-op cannot pass.

---

📄 contrib/quel/t/001_quel.pl (L208-L213)

False-pass hazard. SELECT count(*) on both tables will match trivially if the QUEL delete is a silent no-op AND the base DELETE also matched nothing, or if both happen to delete the same count for unrelated reasons. Counting is too coarse to prove the QUEL delete removed the right rows. Assert the concrete post-state (e.g. that only the salary>=70000 rows remain in qb_delete_quel), not just that the two counts are equal.

---

📄 contrib/quel/t/001_quel.pl (L150-L153)

False-pass hazard on empty results. sort(split /\n/, ...) of two empty strings yields two empty lists, so is_deeply passes even when BOTH the QUEL retrieve and the SQL SELECT return zero rows (e.g. a silent parse fallback that produces no output, or a WHERE that matches nothing). Guard with a non-emptiness assertion first, e.g. ok(@quel_rows > 0, 'QUEL retrieve returned rows'); before comparing. This applies to every sort/split-then-is_deeply comparison in section 5.

---

📄 contrib/quel/t/001_quel.pl (L44-L48)

No skip/prerequisite guard. If the 'quel' shared library is not built or the in-process grammar composition is unavailable on a platform, $node->start with shared_preload_libraries='quel' fails to bring up the postmaster and the whole test aborts (dies) instead of skipping cleanly. Per the tree's TAP conventions, tests should be skippable without optional prerequisites. Consider gating on availability (e.g. check the module is present, or wrap start in a way that skip_all's when quel cannot load).

---

📄 contrib/quel/t/001_quel.pl (L62-L65)

Fragile negative assertions. unlike() only proves a specific log phrase is absent; it does not prove no subprocess/cc was spawned. If the implementation ever changes its wording for a subprocess/rebuild path, this guard silently stops protecting against the regression it claims to catch. Prefer asserting on a positive, stable signal of in-process composition (a log line the code definitely emits) rather than the absence of guessed phrases.

---

📄 contrib/quel/t/001_quel.pl (L72-L73)

Brittle exact-count assertion. This hardcodes '10 tokens', tightly coupled to lengthof(quel_tokens) in quel.c (currently 10). Any future token added/removed silently breaks this test with an opaque failure. If the intent is to verify registration succeeded, prefer matching the stable prefix (qr/quel registered: \d+ tokens/) or derive the expected count from a shared source, rather than pinning the literal 10.

---

📄 contrib/quel/quel_rangetab.c (L62-L67)

Correctness bug: linear-probe termination is incompatible with in-place deletion. slot_for() breaks at the first !used slot, which is only valid if the probe chain is never punctured by a freed slot. But quel_rangetab_reset() (and the old table during rehash_if_needed) mark slots used = false in place, without tombstones or compaction. After a reset+re-populate, or after any rebind that frees a slot appearing earlier in another key's probe chain, a quel_rangetab_lookup() can stop early at the freed hole and return NULL for a name that is actually present (false negative), and quel_rangetab_set() can then insert a duplicate entry for the same key. This corrupts tuple-variable resolution. Use a real hash table (dynahash HTAB with HASH_STRINGS or lib/simplehash.h) instead of a hand-rolled open-addressed table with broken deletion semantics.

---

📄 contrib/quel/quel_rangetab.c (L174-L175)

Out-of-bounds write in non-assert builds. If the table is ever full (all slots used), slot_for() returns -1 with first_free == -1. Assert() is compiled out in production builds, so g_slots[first_free] becomes g_slots[-1] -- a buffer underflow / memory corruption. The load-factor invariant this relies on is enforced only by rehash_if_needed(), which is fragile (see the early-break probing issue). This is exactly the kind of Assert that must not guard a user-reachable condition; handle a full table explicitly or, better, delegate to dynahash which manages growth safely.

---

📄 contrib/quel/quel_rangetab.c (L40-L41)

Comment claims names are stored lowercased, but no lowercasing is performed here or at the call sites: quel_apply_range() passes the raw token strings (rhs_values[2]/[4]) straight to quel_rangetab_set(), and lookups pass tvname verbatim to string_hash/strcmp. Case-insensitive tuple-variable resolution (a documented Berkeley QUEL behavior) will silently fail (e.g. RANGE OF E ... then retrieve (e.x) won't resolve). Either downcase names before storing/looking up, or fix the misleading comments.

---

📄 contrib/quel/quel_rangetab.c (L16-L20)

Aspirational/inaccurate comment: quel_rangetab_init() and quel_rangetab_reset() are never called anywhere in the extension (not from _PG_init, not from a RegisterXactCallback). There is no "reset at backend start" and no xact-abort reset. The table is only lazily initialized via quel_rangetab_set(). Fix the header block to describe what the code actually does, or wire up the init/reset it promises.

---

📄 contrib/quel/quel_rangetab.c (L178-L178)

Dead field. QuelRangeSlot.lineno (and QuelRangeEntry.lineno in the header) is only ever written as 0 here and copied through in quel_rangetab_iterate(); no caller ever sets or reads a meaningful line number. Drop the field (YAGNI) or wire it to a real value.

---

📄 contrib/seg/segparse_driver.c (L195-L199)

seg_yyparse never checks extra.aborted and unconditionally returns 0. The grammar contract in segparse.lime ("we set a flag the driver checks after each push and stops feeding tokens") is not honored here. Grammar rules signal failure via YYABORT/YYERROR (segparse.lime lines 160, 204, 216, 229), which only set extra->aborted = true. In soft-error mode (escontext is a soft-error context, e.g. via pg_input_is_valid / InputFunctionCallSafe), errsave returns instead of longjmping, so the parse keeps running and this function returns 0 (success) with a partially-filled/garbage result. The caller seg_in (seg.c:115) treats 0 as success and returns a bogus SEG, and never reaches its fallback seg_yyerror. This is a data-integrity bug: malformed input can be silently accepted. After driving the final EOF token, check extra.aborted (and ideally SOFT_ERROR_OCCURRED(escontext)) and return 1 on failure.

💡 Suggested change

Before:

	seg_yy(s->parser, 0, zero_yylval, &extra);

	seg_yyFree(s->parser, pfree);
	return 0;
}

After:

	seg_yy(s->parser, 0, zero_yylval, &extra);

	seg_yyFree(s->parser, pfree);
	return (extra.aborted || SOFT_ERROR_OCCURRED(escontext)) ? 1 : 0;
}

---

📄 contrib/seg/segparse_driver.c (L192-L193)

The return value of SegLexFeedEOF is discarded with (void), while SegLexFeedBytes is checked against SEG_LEX_OK. A lexing error that surfaces only at end of input is therefore silently ignored: the driver proceeds to push the EOF token and returns 0, masking a trailing-input lexer error and accepting malformed input. Treat FeedEOF's return code the same way as FeedBytes.

💡 Suggested change

Before:

	(void) SegLexFeedEOF(lex, seg_emit_cb, &ctx);
	SegLexFree(lex, pfree);

After:

	if (SegLexFeedEOF(lex, seg_emit_cb, &ctx) != SEG_LEX_OK)
	{
		SegLexFree(lex, pfree);
		seg_yyFree(s->parser, pfree);
		seg_yyerror(result, escontext, yyscanner, "syntax error");
		return 1;
	}
	SegLexFree(lex, pfree);

---

📄 contrib/seg/segparse_driver.c (L174-L179)

In hard-error mode (escontext is a plain error context), the grammar actions call errsave(), which becomes ereport(ERROR) and longjmps out of seg_yy(). When that happens, SegLexFree(lex, ...) and seg_yyFree(s->parser, ...) are never reached, leaking the lexer and parser handles. palloc'd bytes are reclaimed on the containing MemoryContext reset, but the Lime lexer/parser handles may hold state that is not simply reset; and s->parser is also left dangling on the SegYyScanner. Wrap the lex/parse driving in PG_TRY/PG_FINALLY (or otherwise guarantee SegLexFree/seg_yyFree run on the error path) to release the handles deterministically.

---

📄 contrib/upsert/t/001_upsert.pl (L108-L112)

Use the tree's standard helper PostgreSQL::Test::Utils::slurp_file($node->logfile) instead of hand-rolling a three-arg open + local $/ slurp. Every sibling grammar-extension test (grammar_ext_compose, grammar_ext_overlap) and the quel test read the log via slurp_file; matching that idiom is more portable (it handles the open/close and offset semantics consistently) and removes 5 lines of boilerplate here. Confidence: high (maintainability/convention).

💡 Suggested change

Before:

	my $logfile = $node->logfile;
	open(my $fh, '<', $logfile) or die "cannot read $logfile: $!";
	local $/;
	my $logs = <$fh>;
	close $fh;

After:

	my $logs = PostgreSQL::Test::Utils::slurp_file($node->logfile);

---

📄 contrib/upsert/t/001_upsert.pl (L114-L116)

The log is slurped while the node is still running (Test 6 reads before $node->stop at the end). With log_min_messages=debug1 the target lines are emitted at _PG_init/prewarm (before the first client connection), so they are reliably flushed by the time these assertions run -- so this is not a live race for the positive like() checks. However, reading after $node->stop (as several tree tests do) is the more robust pattern and avoids depending on log-buffer flush timing for any future assertions added here. Confidence: low.

---

📄 pg-aliases.sh (L7-L7)

Unquoted $CC in basename $CC is word-split by the shell. A common CC value such as ccache gcc, or any compiler path containing spaces, makes basename receive multiple arguments and return a wrong compiler name. That wrong name is then compared against last_compiler; a spurious mismatch triggers the destructive trash/rm -rf "$build_dir" branch below, silently wiping the build directory. Quote the expansion, and handle a multi-word CC by taking the last field.

Suggestion:
local current_compiler; current_compiler="$(basename "${CC##* }")"

💡 Suggested change

Before:

	local current_compiler="$(basename $CC)"

After:

	local current_compiler; current_compiler="$(basename "${CC##* }")"

---

📄 src/backend/bootstrap/Makefile (L27-L28)

Missing rule to generate the Lime lexer. meson.build's bootscanner_lex custom_target runs lime_lex_cmd (lime -X ... bootscanner.lex) to produce bootscanner_lex.c and bootscanner_lex.h, and bootscanner.c does #include "bootscanner_lex.h". The .gitignore was also updated so bootscanner.c is now a committed source file, not generated. The Makefile has no rule to build bootscanner_lex.{c,h}, so a make build will fail (missing header / undefined BootLex* symbols). The Makefile is out of sync with meson.build. Add a rule, e.g.:

bootscanner_lex.h: bootscanner_lex.c ;
bootscanner_lex.c: bootscanner.lex
	lime -X -d. $<

and add bootscanner_lex.c to OBJS/build inputs as meson does (boot_parser_sources += bootscanner_lex). Confidence: high.

💡 Suggested change

Before:

bootparse.c: bootparse.lime
	lime -d. $<

After:

bootparse.c: bootparse.lime
	lime -d. $<

# See ../parser/meson.build: bootscanner.c drives the Lime-generated
# lexer emitted from bootscanner.lex via `lime -X`.
bootscanner_lex.h: bootscanner_lex.c ;
bootscanner_lex.c: bootscanner.lex
	lime -X -d. $<

---

📄 src/backend/bootstrap/Makefile (L31-L31)

bootscanner.c #includes the generated bootscanner_lex.h, but this dependency line does not list it. Without it, a parallel (make -j) build can compile bootscanner.o before the header is generated, causing a race/build failure. Add bootscanner_lex.h here (and note bootparse.o does not include it, so keep the targets distinct if desired).

💡 Suggested change

Before:

bootparse.o bootscanner.o: bootparse.h boot_gram_yytype.h

After:

bootparse.o bootscanner.o: bootparse.h boot_gram_yytype.h
bootscanner.o: bootscanner_lex.h

---

📄 src/backend/bootstrap/Makefile (L34-L36)

The clean target does not remove the newly generated bootscanner_lex.c and bootscanner_lex.h. Generated artifacts must be cleaned (compare the parser Makefile cleaning gram.c/gram.h/gram.out). Note also that bootscanner.c is no longer generated, so removing the stale bootscanner.c from clean is correct, but the new lexer outputs must be added.

💡 Suggested change

Before:

	rm -f bootparse.c \
	      bootparse.h \
	      bootparse.out

After:

	rm -f bootparse.c \
	      bootparse.h \
	      bootparse.out \
	      bootscanner_lex.c \
	      bootscanner_lex.h

---

📄 src/backend/bootstrap/boot_gram_yytype.h (L12-L14)

This comment is factually incorrect. It claims boot_yylex() "retains the signature int boot_yylex(union YYSTYPE *, yyscan_t)", but the accompanying driver (bootscanner.c) states that "boot_yylex no longer exists -- the parser is fed by the driver loop directly". The new design uses the Lime push parser (boot_yy) plus an emit callback (boot_emit_cb); there is no boot_yylex() any more. The real reason the union tag/name must be kept intact is that bootstrap.h forward-declares union YYSTYPE. Reword to describe present behavior rather than a signature that no longer exists.

💡 Suggested change

Before:

 * (pre-Phase 2c bootparse.y).  It is kept identical so that boot_yylex()
 * retains the signature `int boot_yylex(union YYSTYPE *, yyscan_t)`
 * declared in include/bootstrap/bootstrap.h.

After:

 * (pre-Phase 2c bootparse.y).  It is kept identical because bootstrap.h
 * forward-declares `union YYSTYPE`, so the tag and name must match exactly
 * across the grammar, the scanner/driver, and that declaration.

---

📄 src/backend/bootstrap/boot_gram_yytype.h (L34-L35)

Same stale reference: boot_yylex() no longer exists (see bootscanner.c). The rationale for keeping the tag/name intact should cite only the union YYSTYPE forward declaration in bootstrap.h, not a boot_yylex() parameter type.

💡 Suggested change

Before:

 * `union YYSTYPE` and boot_yylex() takes `union YYSTYPE *`, so we must
 * keep the tag and name intact.

After:

 * `union YYSTYPE`, so we must keep the tag and name intact.

---

📄 src/backend/bootstrap/bootscanner.c (L258-L260)

Line-number regression. yylineno is incremented to the total number of newlines in the input during the up-front slurp loop, before any lexing/parsing happens. By the time boot_yyerror() fires (line 235), s->yylineno is always the last line of the input, so every syntax error reports "... at line N" pointing at EOF rather than the offending line. The retired flex scanner tracked the current line during scanning. This directly contradicts the header's claim of "identical observable behavior byte-for-byte against the pre-flip bison+flex pair." Track the line through the emit callback (e.g. count newlines consumed up to each matched span) instead of pre-counting the whole buffer. Confidence: high.

---

📄 src/backend/bootstrap/bootscanner.c (L82-L82)

Dead scaffolding: error_seen is declared and documented ("set by emit callback on LEX_ERROR") but is never written or read anywhere. The emit callback signals lexer errors via LEX_ERROR_AT -> BOOT_LEX_ERROR return code and unexpected tokens via elog(ERROR), not through this field. Per YAGNI, remove the unused field (and its misleading comment). Confidence: high.

---

📄 src/backend/bootstrap/bootscanner.c (L154-L158)

Broken indentation in the switch body: case labels and their statements are misaligned (e.g. case\t\tOPEN:, if\t\t\t(len >= 2 ..., and the over-indented memcpy/literal[len]/break lines). This will not survive pgindent and hurts readability. Reflow the whole switch to standard PostgreSQL indentation (case labels at one tab under switch, statements one tab further). Confidence: high.

---

📄 src/backend/jit/llvm/llvmjit.c (L1065-L1069)

This drops the original "no second dot" handling and is incorrect for a name like "pgextern.myfunc" (only the prefix dot, no module separator). strrchr then returns the dot inside pgextern., *funcname points to myfunc, and the length passed to pnstrdup computes to (name+9) - name - 9 - 1 == -1. Since pnstrdup takes a Size (unsigned), -1 wraps to SIZE_MAX, so strnlen copies the whole remaining string and *modname ends up equal to funcname instead of NULL. The caller llvm_resolve_symbol then wrongly takes the load_external_function(modname, ...) branch instead of LLVMSearchForAddressOfSymbol. Restore the fallback: if there is no second dot, leave *modname = NULL.

Additionally, (*funcname)++ on the next line dereferences the strrchr result before any NULL check; the intended guard Assert(*funcname) (see below) comes too late and is against the wrong operand. Confidence: high.

---

📄 src/backend/jit/llvm/llvmjit.c (L1070-L1070)

Assert(funcname) asserts the char ** parameter, which is always non-NULL, so it's a no-op. The intent was to guard the strrchr result, i.e. Assert(*funcname). Moreover it is placed after (*funcname)++ already dereferenced it (line 1066), so even the corrected assertion would fire too late. This assertion is dead and misleading. Confidence: high.

---

📄 src/backend/parser/Makefile (L37-L38)

The Make build can no longer build the parser. scan.o is still in OBJS and the committed scan.c shim #includes scan_lex.h and links against CoreLexFeedBytes/CoreLexAlloc from the Lime-generated scan_lex.c. But this Makefile has no rule to generate scan_lex.c/scan_lex.h from scan.lex, and there is no scan_lex.o in OBJS. The meson build generates and compiles them via a custom_target (lime_lex_cmd on scan.lex); the autoconf/Make path is now broken and out of sync with meson.build. Add a rule to run the Lime lexer generator on scan.lex and add scan_lex.o to OBJS (or otherwise compile the generated scanner).

---

📄 src/backend/parser/Makefile (L58-L59)

lime is invoked as a hardcoded bare command. All other generators are resolved through configure and referenced via make variables ($(BISON), $(FLEX) from Makefile.global), which meson mirrors with find_program(get_option('LIME'), ...). There is no $(LIME)/$(LIMEFLAGS) defined in Makefile.global.in. Hardcoding lime breaks non-PATH installs, cross/VPATH builds, and overrides, and diverges from the meson build. Add a LIME variable to configure/Makefile.global and use $(LIME) here.

---

📄 src/backend/parser/Makefile (L58-L59)

This Make rule only runs the base grammar codegen; it does not produce gram_snapshot.c. The meson build additionally runs the snapshot builder (lime -n) and compiles gram_snapshot.c, which provides base_yyBuildSnapshot() used by the in-process grammar-extension framework (parser_extension.c). As written, the Make build produces a different, incomplete backend than meson. Either generate/compile the snapshot here as well, or document why the Make build intentionally omits it.

---

📄 src/backend/parser/Makefile (L65-L68)

The clean target no longer removes the newly generated Lime lexer artifacts (scan_lex.c/scan_lex.h) or the grammar snapshot (gram_snapshot.c) that a corrected build would produce. Once the missing generation rules are added, clean/distclean must remove those artifacts too, otherwise stale generated files survive make clean and can poison subsequent/VPATH builds.

---

📄 src/backend/parser/gramparse.h (L34-L36)

This comment is inaccurate and will mislead. gram.lime does not contain a duplicate union body; it declares %token_type {YYSTYPE} (gram.lime:267), i.e. it references this type by name and Lime uses it directly. There is no "union body inside gram.lime's %include block" to keep in sync with. The comment invents a synchronization hazard that does not match how the grammar actually consumes YYSTYPE. Reword to state plainly that YYSTYPE is declared here because Lime emits it into gram.c rather than gram.h, and that gram.lime references it via %token_type. Comments must describe what the code does now. (high confidence)

---

📄 src/backend/parser/gramparse.h (L140-L143)

Aspirational/stale comment describing machinery that does not exist. The referenced "Phase 4 subprocess pipeline (parser_extension.c) [that] overwrites it after dlsym-ing the rebuilt parser .so" was removed: parser_extension.c's own header says "The historical Track A path (serialize -> fork lime + cc -> dlopen a cached .so) has been removed entirely". A codebase-wide search finds no assignment to base_yyparse_fn anywhere other than its initializer. The comment must describe current behavior, not a removed/never-shipped path. (high confidence)

---

📄 src/backend/parser/gramparse.h (L145-L145)

base_yyparse_fn is dead scaffolding (YAGNI). It is defined once in parser.c initialized to base_yyparse and only ever read via (*base_yyparse_fn)(yyscanner); no code path anywhere reassigns it (the dlsym/.so swap it was meant for was removed with Track A). As committed, this indirect call is pure overhead that always resolves to base_yyparse. Either drop the indirection and call base_yyparse() directly, or, if a swap really is needed later, add it together with its writer in the same commit. Separately, if this extern is intended for cross-module/extension use it must carry PGDLLIMPORT (as e.g. scanner.h's ScanKeywordTokens and parser_extension.h's pg_grammar_ext_keyword_hook do) or it will fail to link on Windows/MSVC. (high confidence)

---

📄 src/backend/parser/gramparse.h (L46-L46)

Inconsistent member alignment in this hand-written union will not survive pgindent and will be flagged as whitespace churn. Most members are tab-aligned (e.g. int\t\tival;, char\t *str;) but char chr; uses a single space. Run the block through pgindent so the columns match the rest of the tree. (moderate confidence)

💡 Suggested change

Before:

	char chr;

After:

	char		chr;

---

📄 src/backend/parser/parser.c (L45-L45)

base_yyparse_fn is never reassigned anywhere in the tree (searched: no base_yyparse_fn = outside this initializer; parser_extension.c does not reference it). The pointer is initialized to base_yyparse and stays that way, so the indirect call at line 135 always dispatches to the static parser. This is speculative scaffolding for a swap path that isn't wired up, and the block comment describes behavior that does not exist ("parser_extension.c owns the swap ... stores its dlopen'd base_yyparse in this slot"). Per YAGNI, either drop the indirection and keep the direct base_yyparse(yyscanner) call, or land the swap logic in the same patch. As written it adds an unused global and a misleading, aspirational comment.

---

📄 src/backend/parser/parser.c (L66-L67)

getenv("PG_LIME_PUSHPARSE") runs on every raw_parser() call, i.e. on the parse hot path for every statement, and the macro is evaluated even when no extension is loaded (the common case). Beyond the per-parse cost, this is an undocumented environment-variable knob that silently changes the core parsing path with no GUC and no docs, and the comment states it exists only "to A/B the push parser" -- test scaffolding wired into production. Remove the getenv branch, or gate it behind a proper GUC / build-time flag rather than a per-parse getenv.

---

📄 src/backend/parser/parser.c (L38-L38)

These comments reference internal codenames and project phases ("Phase 4 subprocess pipeline", "Track B", "Lime") and make unmeasured performance claims ("zero overhead beyond an indirect call", "zero added cost"). Per PostgreSQL comment discipline, comments explain why in terms a general reader understands and must not assert performance without a benchmark or lean on transient internal vocabulary. Rework to describe the mechanism plainly and drop the unverifiable perf assertions.

---

📄 src/backend/parser/parser_extension.c (L447-L449)

Use-after-free hazard. pg_grammar_ext_unregister() only WARNs on the register()-before-compose case, then unconditionally deletes ext->context. But pending[i].ext and any g_rule_table[rule_id] slots still point into that just-freed context. A later pg_grammar_ext_foreach_token() dereferences pending[i].ext->tokens, and pg_grammar_ext_dispatch_reduce() dereferences g_rule_table[rule_id]->reduce -- both reading freed memory. The comment claims the only consequence is "a dangling fragment", which understates it: this is a crash/corruption path, not just a stale queue entry. Either NULL out the owning pending/g_rule_table slots here (and the composed_ruleid mapping) before freeing, or refuse the unregister with ereport(ERROR) rather than a WARNING.

---

📄 src/backend/parser/parser_extension.c (L728-L729)

Fragile and inconsistent allocation idiom. palloc0(0) followed by repalloc relies on repalloc accepting a zero-size chunk and, unlike the g_rule_table and composed_ruleid growth paths (which palloc0 on first allocation), leaves the grown-but-unwritten tail PendingExt slots uninitialized (garbage .name/.fragment/.ext). Only [0..npending) are written, so today's use is safe, but this is a footgun if any future iteration reaches beyond npending. Use palloc on the first allocation and repalloc only when growing an existing array, matching the rest of this file.

💡 Suggested change

Before:

		pending = repalloc(pending ? pending : palloc0(0),
						   sizeof(PendingExt) * newcap);

After:

		if (pending == NULL)
			pending = palloc(sizeof(PendingExt) * newcap);
		else
			pending = repalloc(pending, sizeof(PendingExt) * newcap);

---

📄 src/backend/parser/parser_extension.c (L288-L289)

Unbounded scan over caller-supplied rhs. This trusts rhs to be NULL-terminated (per the header contract) but reads without any upper bound, so a missing terminator walks off the end (out-of-bounds read) until an accidental NULL. The documented 25-symbol limit is only enforced later in serialize_extension() at register() time, i.e. after this loop and after the pstrdup copy in the loop below have already overrun. Bound the scan here (e.g. cap at the 25-symbol limit and ereport if exceeded) so a malformed array is rejected rather than causing an OOB read.

---

📄 src/backend/parser/parser_extension.c (L315-L315)

Missing space before NULL; will not pass pgindent cleanly. Should be Assert(symbol != NULL);.

💡 Suggested change

Before:

	Assert(symbol !=NULL);

After:

	Assert(symbol != NULL);

---

📄 src/backend/parser/parser_extension.c (L399-L400)

Stale comment referencing the removed subprocess/.so path. This file's own header block states the historical Track A (fork lime + cc -> dlopen a cached .so) has been removed and this is the in-process implementation, yet this comment says the fragment "becomes part of the cache-key digest" -- no digest or cache exists in this file. Per the comment-accuracy discipline, describe current behavior (deterministic fragment feeding the in-process compose), not the removed cached-.so pipeline. Several nearby comments ("rebuilt parser .so", "dlopen", "--export-dynamic") have the same drift.

---

📄 src/backend/parser/parser_pushparse.c (L660-L660)

YYSTYPE is NOT a union of pointer-width members. Per gramparse.h it contains int ival, char chr, bool boolean, and several int-sized enums (JoinType, DropBehavior, ObjectType, ...), all narrower than a pointer. memcpy(&boxed, &lval, sizeof(void *)) reads 8 bytes (on LP64) from a union whose active member may occupy only 1-4 bytes, copying uninitialized padding into boxed. On little-endian the significant low bytes survive, but this depends on endianness and on the reduce wrapper reading the value back through the same byte layout; on big-endian a 4-byte ival boxed as an 8-byte pointer and later read back as int lands in the wrong half. This is a portability footgun (32/64-bit + endianness). The comment's premise is factually wrong and should not be relied on. Copy the full YYSTYPE payload (or box per-member by size) rather than assuming pointer width.

---

📄 src/backend/parser/parser_pushparse.c (L686-L689)

tree is fetched from parse_result(ctx) and then immediately discarded via (void) tree; the actual result comes from yyextra->parsetree. The parse_result() call and the tree local (plus its initialization at declaration) are dead code. Either use parse_result() as the authoritative result or drop the call entirely to avoid misleading readers and wasted work.

💡 Suggested change

Before:

		tree = parse_result(ctx);
		/* The start-rule action assigns yyextra->parsetree; prefer it. */
		*result = yyextra->parsetree;
		(void) tree;

After:

		*result = yyextra->parsetree;

---

📄 src/backend/parser/parser_pushparse.c (L704-L705)

error_lloc is assigned on every error branch but never read: scanner_yyerror() derives the location from the scanner itself, and the value is silenced with (void) error_lloc. The variable and all its assignments are dead code. Remove error_lloc (declaration and the three assignments) to keep the loop clean.

💡 Suggested change

Before:

		(void) error_lloc;
		scanner_yyerror("syntax error", yyscanner);

After:

		scanner_yyerror("syntax error", yyscanner);

---

📄 src/backend/parser/parser_pushparse.c (L212-L214)

On the nconflict>0 (and the rc!=0) failure paths, composed_base_nrule and *base_nrule_out have already been set from base_snapshot->nrule while composed_snapshot remains NULL, leaving global state internally inconsistent (a nonzero base-rule count with no composed snapshot). The current callers (pg_grammar_ext_prewarm/lock_parser) turn a false return into ereport(ERROR/FATAL), so it is not reachable today, but the function's contract is to leave state usable; a future caller that does not abort would observe the mismatch. Reset composed_base_nrule to 0 on the failure paths, or defer setting it until success.

---

📄 src/backend/parser/parser_pushparse.c (L455-L455)

pushparse_peek_resolves_ext hardcodes next != FROM as the sole DELETE discriminator. This is correct only for today's single both-admissible collision (DELETE), and will silently mis-resolve any future both-admissible verb whose dialects diverge at the next token differently (the surrounding comment acknowledges this). Since kw_map already tracks the colliding lexeme, consider keying the peek decision on the specific base_code rather than a bare FROM check, so adding a second colliding verb does not silently route through the DELETE-only heuristic.

---

📄 src/backend/parser/scan.c (L1099-L1102)

Broken indentation here: case OP: and the two following lines carry stray extra tabs (if\t\t\t(...) and an over-indented pstrdup). This will not pass pgindent / git diff --check. Fix the alignment so case OP: lines up with the other cases and the if/assignment use single-tab indentation.

Confidence: high.

💡 Suggested change

Before:

		case FCONST:
			case OP:
			if			(t->val.str != NULL)
							yylval_param->str = pstrdup(t->val.str);

After:

		case FCONST:
		case OP:
			if (t->val.str != NULL)
				yylval_param->str = pstrdup(t->val.str);

---

📄 src/backend/parser/scan.c (L676-L681)

Double allocation / leak for the ICONST-to-FCONST fallback. When pg_strtoint32_safe reports overflow (or a decimal point), buf is retained in val.str and pushed onto the FIFO. Later, core_yylex's FCONST case unconditionally pstrdups val.str into CurrentMemoryContext, so this palloc'd buf is copied and then never freed. This is the same pattern for every buffered string token (SCONST/USCONST/BCONST/XCONST/OP/idents): the original scanner_init-context palloc is orphaned once core_yylex re-pstrdups it. For very large statements the doubled allocation lives until the parse context is reset. Consider not re-copying in core_yylex (the buffered allocation already lives in the parse context) or not allocating twice.

Confidence: high on the double-copy; the practical impact depends on scanner_init and the parse sharing the same MemoryContext (they do, via raw_parser).

---

📄 src/backend/parser/scan.c (L764-L769)

Lifetime inconsistency for extension keywords. On the hook-hit branch val.keyword is set to the palloc'd lower buffer, but core_yylex only re-copies (pstrdup) val.str for string-bearing tokens; it never copies val.keyword. For all other keywords val.keyword points at a canonical compile-time string (GetScanKeyword) with process lifetime, so relying on the pointer is safe there. Here it points at a buffer palloc'd during scanner_init. Since the FIFO is built once at scanner_init and consumed later during the parse, this only works because both run in the same MemoryContext; if that ever changes, extension-keyword val.keyword dangles. Prefer pstrdup'ing into a stable context (or documenting the shared-context invariant explicitly), matching the treatment of val.str.

Confidence: moderate.

---

📄 src/backend/parser/scan.c (L40-L41)

<unistd.h> is included but nothing in this file uses it. Drop it to keep the diff minimal and portable (unistd.h is not available under MSVC).

Confidence: high.

💡 Suggested change

Before:

#include <ctype.h>
#include <unistd.h>

After:

#include <ctype.h>

---

📄 src/backend/parser/scan.c (L749-L751)

The file-header and inline comments carry design-thread / aspirational narrative rather than explaining WHY the code is the way it is: the versioned tool reference "Lime v0.2.2", and the "Phase 4 Track B" story inside the SCAN_TOK_IDENT_RAW hook branch. Per project comment discipline, comments should describe current behavior and rationale, not roadmap/phase labels or specific tool version numbers that will drift. Trim these to a concise description of the extension-keyword fallback.

Confidence: moderate.

---

📄 src/backend/parser/scan.c (L1048-L1054)

scanner_finish never frees the CoreScanner object s itself (allocated with palloc0_object in scanner_init), and only frees scanbuf/literalbuf when they exceed the 8192 threshold; everything else (the s struct, small buffers, and every buffered val.str) is left to MemoryContext reset. This matches the historic scan.c heuristic for the large buffers, but leaving s unfreed while explicitly pfree'ing ctx and ctx->tokens is inconsistent. Since the eager pre-scan now retains the full ScanToken array plus one copy of every literal for the whole parse (vs. the old lazy per-token lexing), peak memory per parse is strictly higher for large statements. Confirm this is acceptable, and either free s for symmetry or document that cleanup relies entirely on the enclosing short-lived context.

Confidence: moderate.

---

📄 src/backend/parser/scan.c (L243-L247)

Missing space in pointer arithmetic: text +i (and text +2, text +1 in scan_lex_handle_xehexesc/xeoctesc, and text -ctx->scanbuf in scan_emit_cb). pgindent treats these as binary +/- and expects text + i. Normalize the spacing so the file pgindents cleanly.

Confidence: high.

💡 Suggested change

Before:

		if (slashstar == NULL && text[i] == '/' && text[i + 1] == '*')
			slashstar = text +i;

		if (dashdash == NULL && text[i] == '-' && text[i + 1] == '-')
			dashdash = text +i;

After:

		if (slashstar == NULL && text[i] == '/' && text[i + 1] == '*')
			slashstar = text + i;

		if (dashdash == NULL && text[i] == '-' && text[i + 1] == '-')
			dashdash = text + i;

---

📄 src/backend/replication/.gitignore (L5-L5)

repl_gram.out is missing from this .gitignore. The Makefile's clean rule removes both repl_gram.out and syncrep_gram.out (both are Lime-generated artifacts), but only syncrep_gram.out is being ignored here. As a result the generated repl_gram.out is left untracked/uncovered and can be accidentally committed. Add the corresponding entry.

💡 Suggested change

Before:

+/syncrep_gram.out

After:

+/repl_gram.out
+/syncrep_gram.out

---

📄 src/backend/parser/scan.c (L450-L454)

Silent truncation of over-long escape sequences. buf[8] with the clamp if (hexlen >= sizeof(buf)) hexlen = sizeof(buf) - 1; means a hex escape longer than 7 digits is quietly truncated to its first 7 digits and strtoul returns a value derived from the clamped prefix, with no error. If the .lex {xehexesc} rule does not already bound the match length to <= 7 hex digits, this yields a silently wrong character rather than a diagnostic. The same applies to scan_lex_handle_xeoctesc with octlen. Verify the lexer rule bounds these, otherwise this is a silent-wrong-result footgun.

Confidence: moderate (depends on scan.lex which is not in this diff).

---

📄 src/backend/parser/scan_lex_internal.h (L111-L112)

The saw_non_ascii shadow field here plus the SCAN_LEX_SAW_NON_ASCII accessor below create a dual-source-of-truth that is actually wrong, not just redundant. In scan.lex the per-string reset does SCAN_LEX_SAW_NON_ASCII(user) = false, which this macro maps to ctx->saw_non_ascii. But the escape helpers set ctx->extra->saw_non_ascii = true and the SCONST emit path reads extra->saw_non_ascii to decide whether to run pg_verifymbstr. So the per-string reset touches a different field than the one that is set and read: extra->saw_non_ascii is only cleared once in scanner_init and, once a high-bit escape sets it, it is never re-cleared for later strings in the same scan. Drop this shadow field and make SCAN_LEX_SAW_NON_ASCII operate on extra->saw_non_ascii, so there is a single source of truth.

---

📄 src/backend/parser/scan_lex_internal.h (L163-L165)

This comment is inaccurate: it references a nonexistent LEX_BUF_APPEND macro and claims the helpers only mutate the literal buffer 'indirectly via the lexer's emit path.' The actual implementations (scan_lex_addlit/scan_lex_addlitchar in scan.c) write directly to the real core_yy_extra_type.literalbuf/literallen. Reword to describe what the code does now, or remove the stale rationale.

---

📄 src/backend/parser/scan_lex_internal.h (L180-L183)

Inaccurate comment: there is no 'parallel C-side accumulator' and no 'Lime literal-buffer macros' in the implementation. scan_lex_addlit/scan_lex_addlitchar append directly to the real extra->literalbuf. This aspirational/scaffolding wording should be replaced with a description of the current behavior.

---

📄 src/backend/parser/scan_lex_internal.h (L189-L190)

Same stale 'C-side accumulator' fiction: scan_lex_litbuf_take operates on the real extra->literalbuf/literallen, not a separate accumulator. Update the comment to match the implementation.

---

📄 src/backend/parser/scan_lex_internal.h (L25-L25)

<stdint.h> is redundant and non-idiomatic for a backend header. The fixed-width types and char32_t (typedef'd to uint32_t) are already provided via c.h (pulled in by postgres.h in the consumers). Drop this direct include and rely on the tree's standard type headers.

---

📄 src/backend/replication/Makefile (L39-L43)

Build-breaking: this Makefile has no rule to generate the Lime lexer output. repl_scanner.o/syncrep_scanner.o are compiled from the committed shims repl_scanner.c/syncrep_scanner.c, which #include "repl_scanner_lex.h" and #include "syncrep_scanner_lex.h". Those headers (and their .c) are only produced by a Lime lexer step. meson.build handles this via lime_lex_cmd custom targets (repl_scanner.lex -> repl_scanner_lex.c/.h, syncrep_scanner.lex -> syncrep_scanner_lex.c/.h), but the Makefile build has nothing equivalent, so make in this directory fails to compile the scanner objects. Add the .lex -> *_lex.c/*_lex.h generation rules (e.g. lime -X -d. $<) to keep the autoconf build in sync with meson.build.

---

📄 src/backend/replication/Makefile (L46-L47)

These dependency lines omit the generated lexer headers repl_scanner_lex.h and syncrep_scanner_lex.h that the shims include. Even after the .lex generation rules are added, repl_scanner.o must depend on repl_scanner_lex.h and syncrep_scanner.o on syncrep_scanner_lex.h, otherwise the header may not be generated before compilation and parallel builds (make -j) will race. syncrep_parse.h/repl_gram_yytype.h alone are not sufficient.

---

📄 src/backend/replication/Makefile (L49-L55)

clean no longer removes the generated scanner artifacts. Previously it deleted the generated repl_scanner.c/syncrep_scanner.c; those are now committed shims, but the Lime lexer step produces repl_scanner_lex.c, repl_scanner_lex.h, syncrep_scanner_lex.c, and syncrep_scanner_lex.h, none of which are removed here. Add them to clean so make clean (and distclean/maintainer-clean) leave no leftover build products.

---

📄 src/backend/replication/repl_gram_yytype.h (L39-L48)

The comment claim is inaccurate, and the union carries dead members. In this Lime design, non-terminals get their semantic type from per-symbol %type {...} declarations in repl_gram.lime (e.g. %type opt_temporary {bool}, %type command {Node *}, %type generic_option {DefElem *}), not from members of this union. Only terminals flow through YYSTYPE, and repl_scanner.c's emit callback populates only str (IDENT/QIDENT/SCONST), uintval (UCONST), and recptr (RECPTR). The members boolval, node, list, and defelt are never written or read as union members anywhere in the scanner or grammar, so they are dead. Either drop the unused members and correct the comment, or, if you are intentionally keeping the union byte-identical to the retired Bison %union, say exactly that instead of the (false) claim that non-terminal actions "read the relevant member directly." Confidence: high.

💡 Suggested change

Before:

typedef union YYSTYPE
{
	char	   *str;
	bool		boolval;
	uint32		uintval;
	XLogRecPtr	recptr;
	Node	   *node;
	List	   *list;
	DefElem    *defelt;
} YYSTYPE;

After:

typedef union YYSTYPE
{
	char	   *str;
	uint32		uintval;
	XLogRecPtr	recptr;
} YYSTYPE;

---

📄 src/backend/replication/syncrep_parse.h (L56-L60)

Duplicated and self-contradictory comment block above SyncRepYyScanner. The first paragraph mentions "a staging buffer for delimited-identifier collection", but that field never existed here, and the immediately following paragraph states the opposite ("no xdbuf field is needed here"). Comments must describe what the code does now. Remove the first (stale) paragraph and keep a single accurate description. (high confidence)

---

📄 src/backend/replication/syncrep_parse.h (L72-L73)

Dead field: pos is only ever written once (s->pos = 0 in syncrep_scanner_init) and never read anywhere; the lexer drives the scan cursor itself, as the comment admits ("advisory only"). Per YAGNI/minimalism, remove this unused field along with its initialization in syncrep_scanner_init to keep the struct minimal and avoid misleading state that can drift from the real scan cursor. (high confidence)

---

📄 src/backend/replication/repl_scanner.c (L153-L160)

UCONST payload is silently truncated for numeric literals >= 32 characters. When len >= sizeof(buf) (32), the clamp sets n = 31, copies only 31 bytes, and strtoul runs on the truncated digits, yielding a wrong uintval. The retired flex scanner ran strtoul directly on the full NUL-terminated yytext, so no truncation occurred; this is a behavioral regression. uintval feeds the TIMELINE argument (repl_gram.lime uses it for invalid timeline %u), so a bogus value can be silently accepted. Also errno is set to 0 but never checked and endp is unused, so overflow of the 32-bit value is neither detected nor reported. Parse the whole literal (e.g. pnstrdup the token then strtoul) and validate rather than truncating into a fixed 32-byte buffer.

---

📄 src/backend/replication/repl_scanner.c (L178-L178)

RECPTR parsing uses sscanf(buf, "%X/%08X", &hi, &lo), but %08X is a maximum field width in scanf, not a mandatory 8 digits. The lexer rule accepts {hexdigit}+/{hexdigit}+ (arbitrary length), so a low half with 9+ hex digits is only partially consumed: sscanf reads the first 8 digits into lo, still returns 2, and the trailing digits are silently discarded -- producing a wrong LSN that is accepted rather than rejected. The retired scanner used %X/%X, and the canonical LSN parser (pg_lsn_in_safe) both bounds each component to MAXPG_LSNCOMPONENT and rejects trailing garbage. This is a correctness/backward-compat regression for START_REPLICATION start locations. Reuse the canonical LSN parsing (or at least use %X/%X and validate no trailing characters) instead of a bespoke %08X sscanf.

---

📄 src/backend/replication/repl_scanner.c (L211-L212)

The -1 sentinel is pushed as the token code and later returned verbatim by replication_yylex() and passed to replication_yy() as yymajor. Generated LALR push parsers typically index action/goto tables by the token major number; a negative major is undefined behavior / an out-of-bounds read rather than a clean syntax error. The comment asserts the parser "will treat it as unexpected", but that is not verifiable from this file (repl_gram.c is generated and not present). If replication_yy() does not explicitly guard yymajor < 0, this is a crash/OOB hazard on any input containing an unexpected character. Recommend routing the catch-all through the same had_error path used by UCONST/RECPTR (emit a syntax error) rather than feeding a negative major to the parser.

---

📄 src/backend/replication/repl_scanner.c (L63-L63)

REPL_TOK_QIDENT is hardcoded as the magic literal 1001 here, and independently as 1001 /* REPL_TOK_QIDENT */ in repl_scanner.lex's xdclose rule. The two are coupled but have no shared definition or compile-time check, so if the lexer sentinel value changes the driver silently mismaps quoted identifiers. Define the sentinel once in the shared generated lex header (or a common private header both include) and reference it from both sides.

---

📄 src/backend/replication/repl_scanner.c (L115-L115)

Capacity growth newcap = s->cap * 2 and the subsequent newcap * sizeof(ReplToken) have no overflow guard. For pathological inputs this can overflow int newcap (to negative) or overflow the size computation before palloc/repalloc. Replication command strings are attacker-influenced (any connected client), so bound the growth / use a size type and check for overflow, e.g. cap growth via a Size and an explicit limit.

---

📄 src/backend/utils/adt/.gitignore (L1-L3)

This .gitignore is missing an entry for the Lime-generated jsonpath_gram.out artifact. The Makefile's clean rule now runs rm -f jsonpath_gram.c jsonpath_gram.h jsonpath_gram.out, confirming jsonpath_gram.out is a generated build product, and it is not a checked-in file. Every parallel migration in this same change adds the corresponding *_gram.out line (e.g. src/backend/parser/.gitignore adds /gram.out, src/backend/replication/.gitignore adds /syncrep_gram.out). Without it, jsonpath_gram.out shows up as an untracked file in git status after a build. Add /jsonpath_gram.out.

💡 Suggested change

Before:

 /jsonpath_gram.h
 /jsonpath_gram.c
-/jsonpath_scan.c

After:

 /jsonpath_gram.h
 /jsonpath_gram.c
+/jsonpath_gram.out

---

📄 src/backend/replication/syncrep_scanner.c (L180-L185)

Dead code and inconsistent NULL handling. Backend palloc never returns NULL -- it throws via elog(ERROR) on allocation failure. So the SyncRepLexAlloc(palloc) OOM branch here can never be taken (dead code), yet the syncrep_yyAlloc(palloc) result on line 189 is not NULL-checked at all. Either the allocators can fail (then both need checking) or they can't (then this branch is misleading). Given palloc is passed as the allocator, drop the dead OOM handling; if the underlying subsystems can genuinely fail with a NULL-returning allocator, use a NULL-returning allocator and check both consistently. (moderate confidence)

---

📄 src/backend/replication/syncrep_scanner.c (L145-L149)

Comment/code drift plus DRY: this block manually reimplements a length-bounded string copy, but the file's own comment above (and on the wildcard rule) claims value-bearing tokens are "pstrdup into yylval->str". Use the existing idiomatic helper pnstrdup(text, len) and align the comments; the current text says pstrdup while the code does palloc+memcpy. (high confidence)

💡 Suggested change

Before:

				char	   *dup = palloc(len + 1);

				memcpy(dup, text, len);
				dup[len] = '\0';
				tok.str = dup;

After:

				tok.str = pnstrdup(text, len);

---

📄 src/backend/utils/adt/Makefile (L138-L139)

Makefile is out of sync with meson.build (high confidence). meson.build generates jsonpath_scan_lex.c/jsonpath_scan_lex.h from jsonpath_scan.lex via lime_lex_cmd and links jsonpath_scan_lex as a separate object; jsonpath_scan.c #includes the generated jsonpath_scan_lex.h. This Makefile has no rule to run lime -X on jsonpath_scan.lex, no jsonpath_scan_lex.o in OBJS, and does not clean the generated lexer. The make build of jsonpath_scan.o will fail (missing jsonpath_scan_lex.h) and the lexer object will never be linked. Add the lex-generation rule, the object to OBJS, and the corresponding clean entries to match meson.

---

📄 src/backend/utils/adt/Makefile (L146-L146)

clean target does not remove the Lime-generated lexer artifacts. meson emits jsonpath_scan_lex.c and jsonpath_scan_lex.h (from jsonpath_scan.lex); once the missing generation rule above is added, clean must also rm -f jsonpath_scan_lex.c jsonpath_scan_lex.h so a Makefile in-tree build leaves no generated files behind (POLA for maintainer-clean/distclean and to keep the tree git-clean).

💡 Suggested change

Before:

	rm -f jsonpath_gram.c jsonpath_gram.h jsonpath_gram.out

After:

	rm -f jsonpath_gram.c jsonpath_gram.h jsonpath_gram.out \
	      jsonpath_scan_lex.c jsonpath_scan_lex.h

---

📄 src/backend/utils/adt/jsonpath_internal.h (L68-L70)

Stale comment: it references jsonpath_yylex_internal, which does not exist anywhere in the tree (the flex-era YY_DECL/jsonpath_yylex pull-scanner was removed in this very diff). In the new Lime-based design, yytext is populated by set_yytext() invoked from the emit callback (jp_emit_cb) in jsonpath_scan.c, not from any jsonpath_yylex_internal. Update the comment to describe the current mechanism so it doesn't mislead readers. (high confidence)

💡 Suggested change

Before:

	 * Most recently matched token's literal text, NUL-terminated.  Updated at
	 * every successful return from jsonpath_yylex_internal.  Read by
	 * jsonpath_yyerror{,_token} for the "at or near \"X\"" branch.  Mirrors

After:

	 * Most recently matched token's literal text, NUL-terminated.  Updated by
	 * set_yytext() from the emit callback for each token.  Read by
	 * jsonpath_yyerror{,_token} for the "at or near \"X\"" branch.  Mirrors

---

📄 src/backend/utils/init/miscinit.c (L1868-L1869)

The claim "here in the postmaster, before backends fork, so no session pays a first-query compose cost" is inaccurate under EXEC_BACKEND (Windows/MSVC and --enable-exec-backend). There, process_shared_preload_libraries() is re-invoked in each child from internal_forkexec()/SubPostmasterMain() (launch_backend.c) after fork+exec, so pg_grammar_ext_prewarm() runs per-backend and each session does pay the compose cost. The call is still correct (prewarm is idempotent via parser_locked), but the comment overstates the benefit and should note the EXEC_BACKEND caveat. Confidence: high.

💡 Suggested change

Before:

	 * snapshot here in the postmaster, before backends fork, so no session
	 * pays a first-query compose cost.  No-op if none registered.

After:

	 * snapshot before backends fork (in the postmaster), so no session pays
	 * a first-query compose cost.  Under EXEC_BACKEND each child re-runs this
	 * after fork+exec; the compose is idempotent so this is a no-op there once
	 * the parser is locked.  No-op if none registered.

---

📄 src/backend/utils/adt/jsonpath_scan.c (L809-L809)

Dead state: s->hi_surrogate is written here in parsejsonpath() but is never read anywhere (confirmed by search; the struct field is declared in jsonpath_internal.h). parseUnicode() and addUnicode() operate on a local hi_surrogate scoped to a single matched {unicode}+ run, which is correct because the lexer's greedy {unicode}+ rule keeps a surrogate pair within one token. The scanner-level field is leftover scaffolding and should be removed (both this assignment and the int hi_surrogate; field in JsonPathYyScanner) to avoid implying cross-token surrogate state that does not exist. Confidence: high.

---

📄 src/backend/utils/adt/jsonpath_scan.c (L763-L763)

The return value of JsonPathLexFeedEOF() is discarded, but the .lex has EOF rules (xq_eof, xvq_eof, xc_eof) that raise errors like "unterminated quoted string" and "unexpected end of comment" only at end-of-input. For input such as $.a == "foo the byte feed (JsonPathLexFeedBytes) returns JSONPATH_LEX_OK, so lex_status stays OK; the error is produced by FeedEOF. The subsequent error-reporting block at lines 765-772 keys off the stale lex_status from FeedBytes, so an EOF-only lexer error will be surfaced only if LEX_ERROR_AT sets escontext directly. Verify that FeedEOF's error path populates escontext (or check its return value here and route through JsonPathLexErrorMessage/jsonpath_yyerror), otherwise unterminated-string/comment diagnostics can be silently dropped. Confidence: moderate (generated lexer's LEX_ERROR_AT behavior not visible in this diff).

---

📄 src/backend/utils/adt/jsonpath_scan_lex_internal.h (L46-L47)

The sentinel codes are numbered JP_TOK_BASE + 2..7, skipping the +0 and +1 slots with no explanation. Since these six values are the complete set emitted by jsonpath_scan.lex and consumed by jp_emit_cb() (verified: only STRING_TAKE/VARIABLE_TAKE/NUMERIC_TEXT/INT_TEXT/RAW_CHAR/VARIABLE_BARE are ever used), the two skipped slots are dead reservations. Either renumber contiguously from +0/+1 or document why the gap exists. Confidence: high.

💡 Suggested change

Before:

#define JP_TOK_BASE                  1000
#define JP_TOK_STRING_TAKE           (JP_TOK_BASE + 2)

After:

#define JP_TOK_BASE                  1000
#define JP_TOK_STRING_TAKE           (JP_TOK_BASE + 0)

---

📄 src/backend/utils/adt/jsonpath_scan_lex_internal.h (L52-L52)

Collision-freedom between these JP_TOK_* sentinels and the grammar's token codes rests entirely on the hand-written comment ("TO_P=12 ... RBRACE=74"). The generated jsonpath_gram.h token values are not visible here and are produced by the Lime tool; if the token set grows or Lime's numbering changes, these sentinels can silently collide and mis-drive jp_emit_cb()'s dispatch. There is no compile-time guard (verified: no StaticAssert in jsonpath_scan.c). Add a StaticAssertDecl in the driver asserting the highest grammar token stays below JP_TOK_BASE so a future collision fails the build instead of silently corrupting token handling. Confidence: moderate.

---

📄 src/backend/utils/misc/Makefile (L46-L46)

Removing this clean rule is correct in isolation now that guc-file.c is a committed source file rather than flex-generated output. But this Makefile is now out of sync with meson.build and will not build under make. meson.build (this dir) declares custom_target('guc_file_lex', input:'guc_file.lex', output:['guc_file_lex.c','guc_file_lex.h']) and links a guc_file static_library; the Makefile has no equivalent. Concretely, under the make-based build:

1. guc-file.c does #include "guc_file_lex.h", but no Makefile rule generates that header -> compile fails.
2. guc-file.c references GucLexAlloc/GucLexFeedBytes/GucLexFeedEOF/GucLexFree, but guc_file_lex.o is not in OBJS and no rule compiles it -> link fails (there is no generic .lex rule in common.mk/Makefile.global either).
3. The migration introduces new generated artifacts (guc_file_lex.c/guc_file_lex.h); with the old clean rule gone and no .gitignore in this directory, they are neither cleaned by clean/distclean/maintainer-clean nor ignored by git.

Add the guc_file.lex -> guc_file_lex.c/.h generation rule, add guc_file_lex.o to OBJS, add a .gitignore for the generated files, and clean them in the appropriate clean/distclean/maintainer-clean target to match meson.build. (high confidence)

---

📄 src/bin/pgbench/Makefile (L35-L36)

Build break in the make build; also desyncs from meson.build. exprscan.c does #include "exprscan_lex.h" (exprscan.c:56) and calls the generated ExprLex* functions, but this Makefile has no rule to generate exprscan_lex.c/exprscan_lex.h from exprscan.lex, and exprscan_lex.o is never added to OBJS. meson.build (lines 19-31) generates both via lime_lex_cmd and compiles exprscan_lex.c into exprscan_lib. As written, make will fail to compile exprscan.c (missing header) and, even if it compiled, fail to link (ExprLexAlloc/ExprLexFeedBytes/... unresolved). Add a codegen rule for exprscan.lex and put exprscan_lex.o in OBJS to match meson.

---

📄 src/bin/pgbench/Makefile (L39-L39)

exprscan.c includes the generated exprscan_lex.h (exprscan.c:56), so exprscan_lex.h must be listed here as a forced prerequisite (alongside a rule that actually produces it). As written, exprscan.o has no dependency on the generated lexer header, so parallel/first builds can attempt to compile exprscan.c before exprscan_lex.h exists.

---

📄 src/bin/pgbench/Makefile (L53-L53)

The clean rule does not remove the Lime lexer artifacts exprscan_lex.c and exprscan_lex.h that the build generates (per meson.build). clean/distclean must remove all newly generated files; add them here once the generation rule is added.

---

📄 src/backend/utils/misc/guc-file.c (L45-L46)

Build breakage (Make path): guc-file.c now #includes the generated header guc_file_lex.h, but only meson (custom_target lime_lex_cmd + guc_file_lib static_library) knows how to generate guc_file_lex.c/.h from guc_file.lex and link guc_file_lex.o. This Makefile still lists guc-file.o and drops the old clean: rm -f guc-file.c rule, but adds no rule to produce guc_file_lex.{c,h} nor to compile/link the lexer object. The autoconf/gmake build (still fully supported) will fail with a missing guc_file_lex.h and unresolved GucLex* symbols. This breaks the backend build and git bisect. Add the generation/compile/link rules to the Makefile.

---

📄 src/backend/utils/misc/guc-file.c (L128-L128)

Missing space around the operator: if (text !=NULL) will not pass pgindent / git diff --check. Use if (text != NULL).

💡 Suggested change

Before:

	if (text !=NULL)

After:

	if (text != NULL)

---

📄 src/backend/utils/misc/guc-file.c (L119-L122)

Integer-overflow footgun in the capacity growth. newcap is int and newcap = s->cap * 2 can overflow to a negative value before it is used in newcap * sizeof(GucToken). Since ntokens/cap/next are all int and the token count is driven by input file size, a pathologically large (or maliciously crafted) config/include file can wrap the int. Use Size for the capacity/sizing arithmetic (or bound the token count) so palloc's own overflow guard is actually reached.

---

📄 src/backend/utils/misc/guc-file.c (L209-L213)

Silent-failure footgun on OOM. When GucLexAlloc() returns NULL the function returns with an empty token queue, so ParseConfigFp() immediately sees GUC_EOF and treats the file as empty/valid rather than reporting an error. An allocation failure while reading postgresql.conf would thus be silently interpreted as "no settings", potentially dropping all configuration. Surface this as an error (or at least set an error flag consumed by ParseConfigFp) instead of masquerading as EOF. Also, input from guc_slurp_file is leaked on this early return.

---

📄 src/backend/utils/misc/guc-file.c (L77-L80)

Comment discipline: this block narrates the migration project rather than describing current behavior. PostgreSQL comments must explain what the code does now and why, not reference external tool versions or an internal phase plan. Drop "Lime v0.2.2", "pre-Phase 5", the "~470 lines of hand-rolled state machine" history, and "so side-by-side comparison of git diffs is straightforward"; keep only the description of the eager-scan FIFO design.

---

📄 src/backend/utils/misc/guc-file.c (L8-L9)

The header comment claims "still emits byte-identical error messages", but the I/O-error path below now emits a different message ("could not read from configuration file ..." reported after the loop, at a different point) and no longer covers the malloc-failure / internal-inconsistency cases the old flex fatal path caught. Reword the claim to match actual behavior, or restore message fidelity.

---

📄 src/backend/utils/misc/guc-file.c (L98-L99)

Dead state: GucScanState.fp is assigned in guc_scan_init() but never read afterward (the file is fully slurped up front in guc_slurp_file, which receives fp directly). Per YAGNI, remove the unused field.

---

📄 src/bin/pgbench/exprscan.c (L112-L115)

Reentrancy regression (moderate confidence). All per-scan state is now file-static globals: the token FIFO (expr_tokens/expr_ntokens/expr_tokens_cap/expr_tokens_next) plus the error-reporting context (expr_source/expr_lineno/expr_start_offset/expr_command/last_was_newline). The retired flex/bison scanner kept this state inside the yyscan_t/PsqlScanState instance. pgbench happens to parse expressions sequentially at startup (single-threaded, non-nested), so this is not an active bug today, but it is a footgun: any future nested or interleaved expr_scanner_init()/expr_yyparse() would silently corrupt another scan's token stream and error context. Prefer hanging this state off the PsqlScanState/expr_yy_extra instead of module globals so the scanner stays reentrant-safe like the rest of psqlscan.

---

📄 src/bin/pgbench/exprscan.c (L276-L276)

pgindent will not leave these lines alone: 'text +len' and '-ctx->input_base' have inconsistent operator spacing (unary/binary confusion). Same defect on 'text +1' in the VARIABLE case below. Should read '(text + len) - ctx->input_base' and 'text + 1'. Run pgindent to fix.

💡 Suggested change

Before:

		(int) ((text +len) -ctx->input_base);

After:

		(int) ((text + len) - ctx->input_base);

---

📄 src/bin/pgbench/exprscan.c (L1095-L1096)

Dead/unused surface (YAGNI). extra.aborted is initialized to false here but never assigned true anywhere (the grammar's %syntax_error and %parse_failure both route through expr_yyerror_token -> syntax_error -> exit(1)), so 'if (extra.aborted) break;' is unreachable and expr_yyparse always returns 0. The aborted field in expr_yy_extra and the yymajor parameter of expr_yyerror_token ('(void) yymajor;') are likewise never consumed. Drop the dead flag/branch/parameter rather than carry speculative scaffolding.

---

📄 src/bin/pgbench/exprscan.c (L471-L475)

Contradictory invariant / silent-wrong-answer footgun (low confidence of triggering, but worth noting). expr_pre_scan asserts state->buffer_stack == NULL and then reads state->scanbuf/scanbufpos directly, while cur_buf()/cur_pos()/set_cur_pos() and expr_yylex_initial() all handle a non-NULL buffer_stack. In a non-assert (production) build the assertion is a no-op; if a buffer_stack ever existed, the pre-scan would tokenize the wrong buffer instead of failing cleanly. Given pgbench's :variable handling never pushes a buffer this cannot happen today, but the mismatch between the assumption here and the buffer_stack-aware helpers is fragile. Consider routing the pre-scan through the same cur_buf/cur_pos helpers for consistency.

---

📄 src/bin/pgbench/exprscan.c (L778-L778)

Fragile contract: overloading a real ScanState enum value (ST_XB, 'SQL: bit string literal') to mean 'EXPR mode' is non-obvious and couples this dispatch to the flex state enum's internals. expr_yylex only tests '== ST_INITIAL', so any value would do, but reusing a meaningful state name invites confusion if psqlscan reselect/error logic ever inspects start_state. A dedicated sentinel or an explicit boolean mode flag would be clearer and less error-prone.

---

📄 src/bin/psql/Makefile (L61-L61)

Make/autoconf build of psql is broken by this change. psqlscanslash.c #include "psqlscanslash_lex.h" and calls the generated lexer symbols (SlashLexAlloc, etc.). In meson (src/bin/psql/meson.build), psqlscanslash.lex is turned into psqlscanslash_lex.c/.h via a lime_lex_cmd custom_target and compiled into psqlscanslash_lib. This Makefile has NO equivalent: there is no rule to run lime on psqlscanslash.lex, no generation of psqlscanslash_lex.c/.h, and psqlscanslash_lex.o is absent from OBJS. Neither psqlscanslash_lex.c/.h is a checked-in file. Result: psqlscanslash.o fails to compile (missing header) and the lexer object is never built or linked. The Makefile and meson.build are out of sync. Add a .lex -> _lex.c/_lex.h codegen rule (mirroring the lime -d. $< parser rules used elsewhere), add psqlscanslash_lex.o to OBJS, and have clean/distclean remove the generated psqlscanslash_lex.c/.h. Confidence: high.

---

📄 src/fe_utils/Makefile (L53-L53)

Build breakage: Makefile is out of sync with meson.build. meson.build generates psqlscan_lex.c/psqlscan_lex.h from psqlscan.lex via lime_lex_cmd and links the generated object into psqlscan_lib. This Makefile removes the old flex rule but adds no replacement: there is no rule to run lime on psqlscan.lex, and psqlscan_lex.o is not in $(OBJS). But psqlscan.c does #include "psqlscan_lex.h" and calls PsqlLexAlloc etc. Under the autoconf/Make build, src/fe_utils will fail to compile (missing psqlscan_lex.h) and, even if that were provided, fail to link (undefined PsqlLex* symbols). Sibling Makefiles (e.g. backend/parser, pgbench) add an explicit lime -d. $< rule for their generated sources; an equivalent lexer-codegen rule for psqlscan.lex plus psqlscan_lex.o in $(OBJS) is required here. The comment claiming "no codegen rule needed" is therefore inaccurate. (high confidence)

---

📄 src/fe_utils/Makefile (L66-L66)

clean/distclean no longer removes the Lime-generated artifacts. meson.build generates psqlscan_lex.c, psqlscan_lex.h (and lime typically emits a .out). Since the Make build must generate these too (see the missing codegen rule above), clean distclean should remove them; otherwise stale generated files linger. Note lex.backup is also no longer produced by any rule in this file, so keeping it here is dead cleanup. (moderate confidence)

---

📄 src/include/c.h (L1396-L1398)

This removes a required portability guard. In a "universal" macOS build, configure runs on the x86_64 host and defines USE_AVX2_WITH_RUNTIME_CHECK / USE_AVX512_CRC32C_WITH_RUNTIME_CHECK / USE_AVX512_POPCNT_WITH_RUNTIME_CHECK in pg_config.h. That same pg_config.h is then reused to compile the aarch64 slice, where x86_64 is not defined. The deleted #undef block was precisely what prevented the aarch64 slice from trying to compile x86-only AVX code (e.g. pg_checksum_block_avx2 with pg_attribute_target("avx2") in checksum.c, and pg_comp_crc32c_avx512 in pg_crc32c.h/pg_crc32c_sse42.c). Deleting it will break the aarch64 slice of universal builds. This deletion is not needed by the SSE2/Neon #if->#elif refactor and should be kept. (high confidence)

💡 Suggested change

Before:

#elif defined(__aarch64__) && defined(__ARM_NEON)
#define USE_NEON
#endif

After:

#else							/* ! x86_64 */

/*
 * In "universal" macOS builds, it's possible for AVX-related symbols to
 * get defined if the build host is x86_64, but we mustn't try to build
 * that code when cross-compiling to aarch64.
 */
#undef USE_AVX2_WITH_RUNTIME_CHECK
#undef USE_AVX512_CRC32C_WITH_RUNTIME_CHECK
#undef USE_AVX512_POPCNT_WITH_RUNTIME_CHECK

/*
 * We use the Neon instructions if the compiler provides access to them (as
 * indicated by __ARM_NEON) and we are on aarch64.  While Neon support is
 * technically optional for aarch64, it appears that all available 64-bit
 * hardware does have it.  Neon exists in some 32-bit hardware too, but we
 * could not realistically use it there without a run-time check, which seems
 * not worth the trouble for now.
 */
#if defined(__aarch64__) && defined(__ARM_NEON)
#define USE_NEON
#endif
#endif							/* x86_64 */

---

📄 src/bin/psql/psqlscanslash.c (L257-L263)

This STOP_VAR_EXPAND handler is dead code in the slash driver. The slash lexer (psqlscanslash.lex) only ever sets STOP_SLASH_OK via PSQL_TERMINATE_AT; it never populates ctx.var_value/ctx.var_name or sets STOP_VAR_EXPAND (slash variable expansion is done inline with appendPQExpBufferStr, unlike the SQL scanner in psqlscan.c). This branch — with its psqlscan_push_new_buffer/free calls — can never execute here, yet it reads as a live path. The adjacent STOP_SEMI/STOP_BACKSLASH/STOP_VAR_RECURSE cases are honestly grouped under "Not used by slash scanner"; STOP_VAR_EXPAND should be folded into that same not-used group rather than carrying a full implementation, to avoid misleading dead scaffolding.

💡 Suggested change

Before:

			case STOP_VAR_EXPAND:
				psqlscan_push_new_buffer(state, ctx.var_value, ctx.var_name);
				free(ctx.var_value);
				free(ctx.var_name);
				ctx.var_value = NULL;
				ctx.var_name = NULL;
				continue;

After:

			case STOP_NONE:
			case STOP_SEMI:
			case STOP_BACKSLASH:
			case STOP_VAR_EXPAND:
			case STOP_VAR_RECURSE:
				/* Not used by slash scanner. */
				return;

---

📄 src/fe_utils/psqlscan.c (L247-L254)

Performance: the driver allocates and frees a fresh lexer (PsqlLexAlloc/PsqlLexFree) on every iteration of this unbounded loop. Because the loop iterates once per stop point (each variable expansion) and, via the PSQL_LEX_ERROR fallback below, potentially once per single byte, a pathological input causes O(n) alloc/free churn versus the flex scanner's persistent buffer. Allocate the lexer once outside the loop and reset its state per iteration (or hoist it to psql_scan()), rather than reallocating each pass.

---

📄 src/fe_utils/psqlscan.c (L248-L248)

Fragile cross-enum coupling: this casts the local ScanState value straight into the generated lexer's state space (PsqlLexSetState((int) state->start_state)) and reads it back with PsqlLexCurrentState(lex). Correctness silently depends on the generated PSQL_STATE_* constants having the exact same numeric ordering as the ScanState enum in psqlscan_int.h. If the .lex %exclusive_state declaration order ever diverges from the enum, this misinterprets scanner state with no compile-time check. Add a translation layer or a static assertion tying the two enumerations together.

---

📄 src/fe_utils/psqlscan.c (L300-L304)

Footgun: these enum cases fall through to continue without advancing the cursor. The comment claims they are unreachable in the SQL scanner, and that matches psqlscan.lex today (STOP_VAR_RECURSE/STOP_SLASH_OK are never set here). But if any of them ever fire with ctx.consumed == 0, the loop restarts at the same position and spins forever. For a can't-happen case in a driver, prefer Assert(false) (or fall through to the same fatal path used for the invalid results elsewhere) rather than a silent continue.

---

📄 src/fe_utils/psqlscan.c (L450-L454)

After free, state->scanbuf is cleared but state->curline, state->refline, state->scanline, and state->cur_line_ptr still point into the just-freed buffer. psql_scan_reset() (called next by psql_scan_destroy) does not clear them either. Any accidental call to psql_scan_get_location() or an emit helper between finish and the next setup would be a use-after-free. Null these out here for consistency with the scanbuf handling and to fail loudly rather than reading freed memory.

---

📄 src/fe_utils/psqlscan.c (L6-L7)

Comment hygiene (PostgreSQL discipline): the file header narrates a migration ("The state machine moved to psqlscan.lex", "Strategy-D streaming driver") and pins an external tool version ("Lime v0.2.2"). Comments should describe what the code does now, not the history of how it got here or a versioned dependency that will rot. Trim the migration/strategy narrative and the version pin.

---

📄 src/include/fe_utils/psqlscan_emit.h (L77-L78)

Dead struct fields. var_text and var_text_len are never assigned or read anywhere in the tree (verified against psqlscan.c, psqlscanslash.c and both .lex sources). They exist only to support the STOP_VAR_RECURSE mechanism, which is never actually produced: recursion is handled inline in psql_emit_var_plain() (pg_log_warning + psqlscan_emit, returning false), so no code path ever sets stop_kind = STOP_VAR_RECURSE. Per the YAGNI/minimalism discipline this is speculative scaffolding for an unwired path and should be removed. (high confidence)

---

📄 src/include/fe_utils/psqlscan_emit.h (L49-L49)

STOP_VAR_RECURSE is dead: it is never assigned anywhere (recursion is handled inline in psql_emit_var_plain by returning false and echoing the raw text). Both driver switch statements (psqlscan.c line ~300, psqlscanslash.c line ~267) list it only as an unreachable fall-through no-op. Remove this enum value and the accompanying var_text/var_text_len fields together, or wire up the mechanism the comments describe. (high confidence)

---

📄 src/include/fe_utils/psqlscan_emit.h (L102-L105)

Inaccurate doc comment: this block states "Each returns true when the action body should LEX_TERMINATE ... false when ...", but only psql_emit_var_plain returns bool. psql_emit_var_squote/dquote/test are declared (and defined in psqlscan.c) as returning void and their callers always LEX_SKIP() unconditionally. The prose should be corrected to say that only psql_emit_var_plain returns bool and the other three always inline-expand into output_buf. As written it misleads a reader into thinking all four share the terminate/skip contract. (moderate confidence)

---

📄 src/include/fe_utils/psqlscan_int.h (L93-L103)

This compatibility shim is dead code and a portability footgun (high confidence). I verified all consumers (psqlscan.c, psqlscanslash.c, exprscan.c) and the .lex sources: none use the bare identifiers INITIAL, xb, xc, ... They all reference the ST_-prefixed enum values directly (e.g. state->start_state = ST_INITIAL), and the Lime lexers use their own PSQL_STATE_INITIAL / %exclusive_state constructs. The comment's justification -- that pgbench's exprscan.l still writes bare INITIAL "until those files are ported" -- is false in this diff: exprscan.c is already the ported hand-rolled scanner and uses ST_INITIAL.

Defining INITIAL (a name with high collision risk) and ten short lowercase names as unconditional macros in an installed fe_utils header that is transitively included by extensions is a real hazard for silent, hard-to-diagnose breakage. Remove the entire shim block and its comment (YAGNI / minimal-diff).

---

📄 src/include/fe_utils/psqlscan_int.h (L207-L217)

This comment is stale and self-contradictory (high confidence). It claims the returned handle is "the same pointer callers stored in StackElem.buf" -- but StackElem.buf was removed in this very diff, and I verified both call sites (psql_scan_setup at psqlscan.c:351 and psqlscan_push_new_buffer at psqlscan.c:669) discard the return with (void) and use only the *txtcopy out-parameter. The return value is never consumed anywhere. Either drop the return value (make it void and delete the misleading opaque-handle narrative), or at minimum rewrite the comment to describe the actual current contract (allocate buffer, apply FF mapping, set *txtcopy). Comments must describe what the code does now, not the flex-era history.

---

📄 src/include/fe_utils/psqlscan_int.h (L31-L32)

Migration-narrative comment describes history, not current behavior (medium confidence). "Pre-Phase 2h, this file used flex's ..." and "flex is no longer involved" are the kind of phase/porting notes that PostgreSQL review discipline asks to be dropped; the header should read as if it had always been written this way. Suggest removing the parenthetical.

---

📄 src/include/fe_utils/psqlscan_int.h (L111-L113)

Same migration-narrative issue as above (low confidence). "Pre-port these owned ..." / "Now we just track ..." narrates the flex-to-handrolled transition rather than documenting the current struct. Describe only what StackElem holds now.

---

📄 src/interfaces/ecpg/preproc/Makefile (L73-L74)

The Makefile is out of sync with meson.build for building pgc.o. meson.build (lines 23-33, 115) generates pgc_lex.c/pgc_lex.h from pgc.lex via the lime lexer and compiles pgc_lex.c into ecpg; the now-committed hand-rolled pgc.c does #include "pgc_lex.h" (pgc.c:52). This Makefile has no rule to generate pgc_lex.{c,h} from pgc.lex and no pgc_lex.o in OBJS. The make build will fail to compile pgc.c (missing pgc_lex.h) and fail to link (missing pgc_lex.o). Add the lex-generation rule and object, mirroring meson.

Confidence: high.

---

📄 src/interfaces/ecpg/preproc/Makefile (L68-L71)

This still relies on bison to build preproc.c from preproc.y (via preproc.c: BISONFLAGS += -d at line 66 and the generic %.c: %.y rule in Makefile.global). meson.build (lines 86-103) instead converts preproc.y -> preproc.lime via lime_convert_gram.py and compiles it with lime, and parser.c's new base_yyparse calls the lime runtime (base_yyAlloc/base_yyLoc/base_yy_drain). A bison-generated preproc.c will not provide those symbols, so ecpg will fail to link. The make path is stuck at "Phase 2k.3" while meson/parser.c are at "Phase 3 final"; the two build systems must stay in sync.

Confidence: high.

---

📄 src/interfaces/ecpg/preproc/Makefile (L104-L104)

derived_gram.y is a build artifact (meson.build:47 states it is "not committed") but clean/distclean does not remove it. It is also not in .gitignore, so it will show up as an untracked file. The same applies to the other new build artifacts required to make this Makefile work (pgc_lex.c, pgc_lex.h, and preproc.lime once the pipeline is completed). Add them here and to .gitignore.

Confidence: high.

---

📄 src/include/parser/parser_extension.h (L196-L204)

Stale/incorrect ABI documentation (high confidence). This comment ties pg_grammar_ext_dispatch_reduce to "the .so produced by the Phase 4 subprocess pipeline" and to a dlopen'd module resolving the symbol via --export-dynamic. But parser_extension.c states the subprocess/.so path "has been removed entirely" and the actual caller is pg_grammar_ext_resolve_reduce -> pg_grammar_ext_dispatch_reduce, all in-process (parser_pushparse.c: pushparse_host_reduce). No .so, no dlopen, no subprocess exists. The doc describes a nonexistent mechanism, which will mislead extension authors and reviewers. Rewrite to describe the live in-process reduce dispatch.

---

📄 src/include/parser/parser_extension.h (L299-L303)

Aspirational/future-tense comment for a removed path (high confidence). This describes "the upcoming subprocess pipeline that will concatenate this fragment with the base gram.lime, fork lime to compile, and dlopen the result." parser_extension.c is explicit that this Track A subprocess/.so path was removed entirely in favor of in-process compose. Future-tense comments ("upcoming", "will") describing behavior that was removed/never shipped are a documented rejection reason on -hackers. Describe only what pg_grammar_ext_get_serialized_lime does today (return the serialized fragment; used by the dummy_grammar_ext smoke test and contrib/quel).

---

📄 src/include/parser/parser_extension.h (L193-L194)

Contradictory description of the removed subprocess path within the same file header (medium confidence). The header first says the subprocess path was "removed" and Track B is in-process, then the pg_grammar_ext_dispatch_reduce doc reintroduces it as the live "Phase 4 subprocess pipeline" / ".so" ABI. A reader cannot tell whether the .so path exists. Since parser_extension.c confirms it is gone, drop the dead references to "subprocess pipeline" and ".so" throughout this header so the doc matches the shipped implementation.

---

📄 src/include/parser/parser_extension.h (L81-L82)

Comment hygiene (low confidence). Referencing external Lime commit hashes ("commit e2f84c6, hardened in 64b6854") and version numbers ("Lime v1.7.1") in an in-tree public header is meaningless to PostgreSQL readers and will drift/rot; likewise the enumerated subtest counts elsewhere in this header ("22 subtests", "44 subtests"). PostgreSQL comments should explain the contract, not narrate external project history. Recommend removing the external commit/version citations and hard-coded subtest counts.

---

📄 src/interfaces/ecpg/preproc/parser.c (L259-L259)

Wrong token name: the ecpg grammar (ecpg.trailer: server: Op server_name, opt_options: Op connect_options) uses the token Op (from the backend gram.y %token <str> Op), so the generated preproc.h defines Op, not OP. OP is not defined anywhere in the ecpg preproc grammar, so case OP: will fail to compile (or silently match an unrelated macro). The comment claiming a rename "in Phase 3 final" is inaccurate -- no such rename exists. This case is what triggers loc_strdup of base_yylval.str for operator tokens; getting it wrong means operator string values are not duplicated. Restore case Op:.

Confidence: high.

💡 Suggested change

Before:

			case OP:			/* renamed from Op in Phase 3 final */

After:

		case Op:

---

📄 src/interfaces/ecpg/preproc/parser.c (L260-L270)

pgindent-hostile reformatting churn on otherwise-untouched lines: over-indented case labels (extra leading tab), doubled tab after case (case\t\tCSTRING:), and a stray blank line before break;. This is whitespace-only churn that will not survive pgindent and violates minimal-diff discipline. Keep the original single-indent case FOO: formatting and the base_yylloc = loc_strdup(...) line immediately followed by break;.

Confidence: high.

---

📄 src/interfaces/ecpg/preproc/parser.c (L353-L359)

This entire ASCII-to-symbolic mapping is dead/incorrect. The Lime symbols it returns -- LPAREN, RPAREN, LBRACKET, RBRACKET, COMMA, SEMI, COLON, DOT, PLUS, MINUS, STAR, SLASH, PERCENT, CARET, PIPE, LT, GT, EQ, LBRACE, RBRACE -- are not defined anywhere in the ecpg preproc grammar (grep finds them only here). The ecpg grammar represents self-chars by their raw ASCII value; pgc.c's base_yylex() already returns those raw char codes (PGC_TOK_RAW_CHAR -> out_code = (unsigned char) text[0]). Mapping '(' to a nonexistent LPAREN both fails to compile and, if it did, would feed the parser the wrong token IDs. Drop this function and push tok directly.

Confidence: high.

---

📄 src/interfaces/ecpg/preproc/parser.c (L348-L351)

base_yyAlloc, base_yyLoc, base_yy_drain, and base_yyFree are referenced only here and are not defined anywhere in the ecpg tree (they are expected to be emitted by a Lime-generated preproc.c that is not part of this change). If the generated driver is not present in the same commit, this translation unit will not link, breaking git bisect/atomic-commit discipline.

Confidence: moderate (depends on generated preproc.c presence in the same commit).

---

📄 src/interfaces/ecpg/preproc/parser.c (L413-L421)

base_yyparse() unconditionally returns 0 and has no error path. Bison's base_yyparse returns non-zero on syntax error; here parse errors detected by the Lime parser during base_yyLoc/base_yy_drain are never surfaced through the return value. While ecpg.c ignores the return value and relies on mmerror/mmfatal, the Lime parser's error action must still be reachable -- there is no visible wiring that a syntax error calls mmerror here, so malformed input risks being silently accepted and mis-translated. Additionally, on the base_yyAlloc failure path the handle is not freed (fine, since NULL), but on any error/longjmp out of filtered_base_yylex the parser handle allocated with malloc is leaked (no PG_TRY/cleanup). Confirm the Lime error routine reports via mmerror and consider freeing the handle on all paths.

Confidence: moderate.

---

📄 src/interfaces/ecpg/preproc/parser.c (L338-L339)

Comment describes a migration process ("Phase 3 final", "opt-in eager default-reduce primitive (P0-NEW-8)", "Lime v0.2.0's Parse_drain()") rather than what the code does now, and references internal phase/version labels that are meaningless in the committed tree. Per PostgreSQL comment discipline, comments should explain the current behavior and rationale, not the porting history. Rewrite to describe why draining after each push is needed, without phase/version-tracking prose.

Confidence: high.

---

📄 src/interfaces/ecpg/preproc/preproc_extern.h (L43-L43)

base_yyleng has no cross-module consumer: it is defined and used only within pgc.c (via set_yytext), and no other translation unit in the ecpg tree references it (unlike the adjacent base_yytext, which parser.c uses). Per YAGNI, an extern in this shared header for a symbol nobody else reads is unnecessary and could just be file-static in pgc.c. If it is intentionally exposed for parity with the flex-generated scanner's public API, keep it; otherwise drop this line. Low confidence / low severity.

---

📄 src/interfaces/ecpg/preproc/pgc_internal.h (L64-L65)

Dead declarations: pgc_lit_len, pgc_lit_peek, and pgc_consume_cvariable_tail are declared here and defined in pgc.c, but have zero callers anywhere in the ecpg tree (verified across pgc.lex/pgc.c). pgc_consume_cvariable_tail's body is a no-op stub (return len;), i.e. speculative scaffolding. Per the tree's minimalism/YAGNI discipline, remove the unused declarations (and their definitions) unless a caller lands in this same commit. (high confidence)

---

📄 src/interfaces/ecpg/preproc/pgc_internal.h (L88-L88)

pgc_consume_cvariable_tail has no callers and its implementation is a no-op returning len unchanged. This is dead API surface; drop it unless it is wired to a caller in this commit. (high confidence)

---

📄 src/interfaces/ecpg/preproc/pgc_internal.h (L86-L86)

The scanner/lexer state is passed as untyped void *user / void *lex across the pgc.lex<->pgc.c boundary, defeating compile-time type checking (a caller can silently pass the wrong pointer). In the current implementation both are simply ignored ((void) user; (void) lex;), so if the parameters are truly unused they should be dropped rather than propagated as opaque pointers; if they are meant to carry the Lime lexer/user context, declare them with the concrete type from the generated lexer header instead of void *. (moderate confidence)

---

📄 src/interfaces/ecpg/preproc/preproc_yytype.h (L11-L12)

The comment misdirects maintainers about where this union must be kept in sync. Verified against the tree: the %union is NOT generated from gram.lime and is not maintained in preproc.y. parse.pl copies the union verbatim from the hand-maintained ecpg.header (include_file('header', 'ecpg.header') -> dump_buffer('header')); preproc.y is itself a generated artifact. Only the grammar rules are derived from gram.lime (via derived_gram.y/lime_to_bison_gram.py). Since this typedef must match the %union byte-for-byte (a mismatch silently corrupts the parser value stack), the comment should point to the real source of truth: ecpg.header. Suggest rewording to state the struct must be kept in sync with the %union in ecpg.header.

💡 Suggested change

Before:

 * The struct definitions match the %union in preproc.y (which is
 * kept in sync via parse.pl-driven generation from gram.lime).

After:

 * The struct definitions must be kept in sync with the %union in
 * ecpg.header, which parse.pl copies verbatim into the generated
 * preproc.y.  (Only the grammar rules are derived from gram.lime.)

---

📄 src/interfaces/ecpg/preproc/pgc.c (L957-L964)

Buffer overflow (confidence: medium). strlcpy fills inc_file up to MAXPGPATH-1 (1023) bytes; the subsequent strcat(inc_file, ".h") writes 3 more bytes with no remaining-space check, overflowing the fixed buffer by up to 3 bytes. base_yytext here derives from the include filename in the (attacker-influenced) ecpg input. The include-paths branch below reserves +4 via its length check, but this quoted branch does not. Use a bounded append (strlcat) or verify space before appending.

💡 Suggested change

Before:

		strlcpy(inc_file, base_yytext, sizeof(inc_file));
		f = fopen(inc_file, "r");
		if (!f)
		{
			if (strlen(inc_file) <= 2 ||
				strcmp(inc_file + strlen(inc_file) - 2, ".h") != 0)
			{
				strcat(inc_file, ".h");

After:

		strlcpy(inc_file, base_yytext, sizeof(inc_file));
		f = fopen(inc_file, "r");
		if (!f)
		{
			if (strlen(inc_file) <= 2 ||
				strcmp(inc_file + strlen(inc_file) - 2, ".h") != 0)
			{
				strlcat(inc_file, ".h", sizeof(inc_file));

---

📄 src/interfaces/ecpg/preproc/pgc.c (L69-L76)

Integer overflow (confidence: low). len is int but callers cast down from size_t (e.g. set_yytext(cur_feed_token_text, (int) cur_feed_token_len)). If len were ever negative, (size_t) len + 1 becomes huge and the newsz *= 2 loop overflows to 0, producing a bogus/zero allocation followed by an out-of-bounds memcpy/write. Token lengths are currently bounded by the per-buffer int len, so this is defensive, but consider asserting len >= 0 (and rejecting len == INT_MAX) to make the invariant explicit.

---

📄 src/pl/plpgsql/src/pl_gram_types.h (L30-L33)

postgres.h must not be included from a header. By tree convention it is the very first include in a .c file only; headers assume the includer already pulled it in. Note the sibling file added in this same change, repl_gram_yytype.h, does NOT include postgres.h -- it lists only its real prerequisites (access/xlogdefs.h, nodes/parsenodes.h, nodes/pg_list.h). This header should do the same for consistency and to avoid include-order breakage when pulled into generated code. The justifying comment ("pulled into Lime's generated pl_gram.c BEFORE the user prologue") describes a code-generator ordering workaround, not a reason to break the header convention.

💡 Suggested change

Before:

#include "postgres.h"
#include "common/keywords.h"
#include "parser/scanner.h"
#include "plpgsql.h"

After:

#include "common/keywords.h"
#include "parser/scanner.h"
#include "plpgsql.h"

---

📄 src/pl/plpgsql/src/pl_gram_types.h (L44-L46)

This YYSTYPE union is a hand-maintained duplicate of the semantic-value types that the grammar declares independently. The former %union in pl_gram.y is gone (pl_gram.y was converted to pl_gram.lime), and pl_gram.lime now carries per-symbol types via separate %type declarations (e.g. %type for_variable {struct { char *name; int lineno; PLpgSQL_datum *scalar; PLpgSQL_datum *row; }}, %type loop_body {...}, %type decl_cursor_intro {...}). So the member types live in two places with no mechanical check that they agree. If a member is added/removed or a type drifts between this header and the .lime %type decls, the parser will reinterpret a union member of the wrong type at runtime -- silent semantic-value corruption or a crash, not a compile error. The comment even concedes the invariant is manual ("must stay in sync ... update both places together"). This is a footgun; at minimum the sync obligation should be enforced (e.g. generate this body from the grammar) rather than relying on a prose reminder.

---

📄 src/pl/plpgsql/src/pl_gram_types.h (L36-L40)

These comments are aspirational/future-tense and lean on internal codenames ("Lime", "Phase 2j") plus a build path that does not exist yet in this commit ("the legacy build path before Phase 2j flips", "Once Phase 2j lands"). Per tree comment discipline, comments must describe what the code does now; references to a not-yet-landed migration phase will read as stale the moment it lands and are opaque to reviewers. It also signals a non-atomic transitional state (a header whose correctness depends on a future flip), which is at odds with the atomic/bisectable-commit expectation. Rewrite to describe the current behavior of the guard without the roadmap narrative.

---

📄 src/pl/plpgsql/src/plpgsql.h (L1314-L1316)

Minor style inconsistency: the parser-handle parameter is named yypParser here but yyp in the two declarations directly below. Since all three functions take the same parser handle, using a single consistent name (e.g. yypParser) would improve readability. (low confidence, style only)

💡 Suggested change

Before:

extern int	plpgsql_yy_drain_lookahead(void *yypParser, yyscan_t yyscanner);
extern int	plpgsql_yy_get_lookahead(void *yyp, union YYSTYPE *yyminor_out, YYLTYPE *yyloc_out);
extern void plpgsql_yy_clear_lookahead(void *yyp);

After:

extern int	plpgsql_yy_drain_lookahead(void *yypParser, yyscan_t yyscanner);
extern int	plpgsql_yy_get_lookahead(void *yypParser, union YYSTYPE *yyminor_out, YYLTYPE *yyloc_out);
extern void plpgsql_yy_clear_lookahead(void *yypParser);

---

📄 src/pl/plpgsql/src/pl_scanner.c (L450-L450)

Local extern declaration inside a function body violates PostgreSQL convention and is a maintainability/portability footgun: the compiler cannot cross-check this prototype against the real definition of plpgsql_lime_to_ascii_token (in pl_gram.lime), so if that signature drifts the mismatch becomes silent UB rather than a build error. Declare the prototype in a shared header (e.g. plpgsql.h, alongside plpgsql_yy_get_lookahead / plpgsql_yy_clear_lookahead) and include it, rather than re-declaring it inline. (moderate confidence)

---

📄 src/pl/plpgsql/src/pl_scanner.c (L462-L462)

The drained lookahead token loses its length: plpgsql_push_back_token() sets auxdata.leng = yyextra->plpgsql_yyleng (the current scanner state), not the length of the token being pushed back, and plpgsql_yy_get_lookahead() only returns the token code, lval and lloc (no length). When this pushed-back token is next re-read via internal_yylex(), plpgsql_token_length() will report the wrong length, which can skew end-location computations in actions such as read_sql_construct (endlocation = *yyllocp + plpgsql_token_length(...)). Confirm none of the drain-then-relex paths rely on plpgsql_token_length() for the first re-read token, or capture and restore the length. (low confidence -- plpgsql_yy_get_lookahead is generated and not fully traceable in this diff)

---

📄 src/test/isolation/Makefile (L51-L52)

The make/autoconf build is broken for the isolation lexer. specscanner.c (committed source) does #include "specscanner_lex.h", and meson.build generates specscanner_lex.c/specscanner_lex.h from specscanner.lex via a custom_target (lime_lex_cmd). But this Makefile adds no rule to run Lime's lexer generator on specscanner.lex, and src/Makefile.global.in only has pattern rules for %.c: %.l (flex) and %.c: %.y (bison) — nothing for .lex. So under make, specscanner_lex.h is never produced and the build fails to compile specscanner.o.

Add a generation rule for the lexer, e.g.:

specscanner_lex.c specscanner_lex.h: specscanner.lex
	lime -d. --lexer $<   # match the flags meson's lime_lex_cmd uses

(confidence: high)

---

📄 src/test/isolation/Makefile (L55-L55)

specscanner_lex.o is not linked. meson lists spec_scanner_lex (the generated specscanner_lex.c) as a source of the isolationtester executable, but here OBJS (lines 15-19) contains only specparse.o and specscanner.o. Even once specscanner_lex.c is generated, the make build will not compile/link it, producing unresolved symbols (SpecLexAlloc, SpecLexFeedBytes, etc.) that specscanner.c calls. Add specscanner_lex.o to OBJS and to the force-dependency line.
(confidence: high)

---

📄 src/test/isolation/Makefile (L61-L61)

clean no longer removes the Lime-generated lexer output. specscanner.lex is compiled to specscanner_lex.c and specscanner_lex.h (per meson.build), but this clean rule only removes specparse.*. These generated files (and specscanner_lex.o) will be left behind after make clean/distclean, breaking VPATH/tarball builds and leaving stale artifacts. Add them to the rm list.
(confidence: high)

---

📄 src/test/isolation/spec_gram_yytype.h (L11-L12)

The comment narrates the migration process ("the retired grammar (pre-Phase 2f specparse.y)") rather than describing what the code does now. Per PostgreSQL comment discipline, comments should explain the current design and rationale, not reference internal migration phase numbers that are meaningless to a reader of the merged tree. Once this lands there is no "Phase 2f" and no in-tree "retired specparse.y" to compare against, so the reference becomes stale on commit. Recommend stating the invariant directly, e.g. that the union shape must be kept in sync with the grammar actions and the scanner, without the phase/history narration. (moderate confidence)

Note: this same phase-narrating pattern recurs across the sibling files in this change (boot_gram_yytype.h, repl_gram_yytype.h, specparse.lime, etc.); consider scrubbing them consistently.

💡 Suggested change

Before:

 * The union shape matches the Bison %union in the retired grammar
 * (pre-Phase 2f specparse.y); it is kept identical so that the

After:

 * The union shape must stay in sync with the semantic actions in
 * specparse.lime and the scanner in specscanner.c so that the

---

📄 src/test/isolation/specscanner.c (L250-L250)

Behavioral regression: SpecLexFeedEOF's return status is discarded. Unlike the SpecLexFeedBytes call above (which checks != SPEC_LEX_OK and reports the error), the EOF pass can raise lexer errors too: specscanner.lex defines qident_eof -> "unterminated quoted identifier" and sqlblk_eof -> "unterminated sql block", both firing during EOF processing. Because the status is (void)-cast and spec_yy(ctx.parser, 0, ...) is then called unconditionally, a spec ending mid-quoted-identifier or mid-SQL-block will silently drop that diagnostic and either produce a misleading parser-level "syntax error" or accept truncated input. The retired flex scanner reported these EOF conditions. Check the status and route it through spec_yyerror the same way the byte-feed path does.

💡 Suggested change

Before:

	(void) SpecLexFeedEOF(lex, spec_emit_cb, &ctx);

After:

	lex_status = SpecLexFeedEOF(lex, spec_emit_cb, &ctx);
	if (lex_status != SPEC_LEX_OK)
	{
		const char *m = SpecLexErrorMessage(lex);

		spec_yyerror(m ? m : "syntax error");
	}

---

📄 src/test/modules/dummy_grammar_ext/dummy_grammar_ext.c (L6-L11)

This entire header block describes a subprocess/.so/dlopen pipeline ("Track A", $PGDATA/pg_parser_cache, "the cached .so dlopens", "raw_parser() dispatches through the dlopn'd base_yyparse") that has been removed. Per parser_extension.c the shipped implementation is Track B in-process compose -- "The historical Track A path (serialize -> fork lime + cc -> dlopen a cached .so) has been removed entirely". The claims (2)-(4) about cache keys, dlopen, and base_yyparse dispatch are false for the current code. Additionally, the header says the rule's LHS is a fresh non-terminal dummy_stmt, but _PG_init() below actually calls pg_grammar_ext_add_rule(ext, "stmt", ...). Rewrite this header to describe the in-process compose path and the real LHS. (high confidence)

---

📄 src/test/modules/dummy_grammar_ext/dummy_grammar_ext.c (L54-L58)

This comment contradicts both the shipped behavior and the inner comment 8 lines below. It claims the trampoline is "not yet" implemented and the callback is "wired but unreachable at runtime ... until then the body is documentation." But parser_extension.h/.c implement live Track B reduce dispatch (extension rules route to their registered PgGrammarReduceFn), and the inner comment at lines 66-69 says "The trampoline now actually fires this callback." One of these is wrong; the code shows the callback does fire, so this stale future-tense comment should be removed. (high confidence)

---

📄 src/test/modules/dummy_grammar_ext/dummy_grammar_ext.c (L90-L92)

Stale comment referencing the removed Track A subprocess pipeline ("the rebuild pipeline runs end-to-end and the dlopen'd parser drops in cleanly") and deferring token-table integration to "Track B's problem" -- but Track B is the shipped implementation, and pg_grammar_ext_add_token here IS the token integration. This drifts from what the code now does. (moderate confidence)

---

📄 src/test/modules/dummy_grammar_ext/dummy_grammar_ext.c (L113-L115)

Misleading log text: "rebuild will run on first parse" describes the removed subprocess-rebuild-on-first-parse model. In the current implementation the grammar is composed in-process at postmaster startup via pg_grammar_ext_prewarm() (before backends fork), not "rebuilt" on first parse. Since this NOTICE is the test's grep target, the wording should reflect the actual compose-at-prewarm behavior. (moderate confidence)

---

📄 src/test/modules/dummy_grammar_ext/dummy_grammar_ext.c (L28-L30)

This module advertises itself as a smoke test whose "primary verification" is grepping LOG/NOTICE output from a "standalone-backend smoke test," but no TAP harness (t/.pl) is checked in for dummy_grammar_ext -- only meson.build builds the shared_module. Its sibling lime_in_process_smoke ships t/001_smoke.pl. As-is, nothing loads this module or asserts on its NOTICEs in CI, so it exercises no behavior and would not catch a regression. Add a t/001_.pl that starts a node with shared_preload_libraries='dummy_grammar_ext' and asserts the expected NOTICE(s), plus the corresponding Makefile if the module is also built under the make system. (moderate confidence)

---

📄 src/test/modules/grammar_ext_compose/compose_ext_foxtrot.c (L6-L9)

This header comment contradicts the actual spec below and the real behavior. It claims register() "should fail ... with a clear error" and that "expect_failure=true", but the code sets .expect_failure = false (line 55), and the inline comment there gives the opposite rationale. Per compose_ext_helpers.h, expect_failure means "register() should fail", and the TAP test (Test 4 in t/001_compose.pl) is wrapped in a TODO block precisely because the conflict is NOT caught at register() time (register succeeds) but only later at in-process compile time. So .expect_failure = false is correct, and this header comment is stale/wrong. Fix the comment to match the code: register() currently succeeds for foxtrot; the collision surfaces at compose time, hence expect_failure=false. Also drop the line-break artifacts ("expect_-\nfailure") which are awkward wrapping.

💡 Suggested change

Before:

 * Re-declares K_GRAMMAR_ALPHA with a DIFFERENT lexeme.  Per the API
 * contract, this should fail register() with a clear error.  expect_-
 * failure=true so the helper logs the failure as expected (NOTICE)
 * rather than as a regression (WARNING).

After:

 * Re-declares K_GRAMMAR_ALPHA with a DIFFERENT lexeme.  The token
 * collision is not detected at register() time (register() currently
 * succeeds); it surfaces later when the merged grammar is composed
 * in-process.  Hence expect_failure=false below.

---

📄 src/test/modules/grammar_ext_compose/compose_ext_golf.c (L12-L15)

This header comment asserts a negative behavior ("reversed order should produce a clear error") that the test suite does not actually exercise. The TAP test's Test 5 only loads grammar_ext_compose_alpha,grammar_ext_compose_golf (the success ordering); there is no case loading golf before alpha to confirm the promised clear error. As written, this describes an untested contract. Either add a reversed-order case in t/001_compose.pl and assert the error, or drop the claim so the comment matches what is verified. (moderate confidence)

---

📄 src/test/modules/grammar_ext_compose/compose_ext_golf.c (L34-L37)

Aspirational/future-tense comment. pg_grammar_ext_add_rule() and pg_grammar_ext_register() perform no RHS symbol-table validation, so the "if we add the symbol-table check" branch describes a check that does not exist today. Per the project rule that comments describe current behavior (no "if we add", "future", "for now"), trim this to what actually happens: an unknown RHS symbol surfaces at lime-rebuild time. (high confidence)

---

📄 src/test/modules/grammar_ext_compose/compose_ext_helpers.h (L50-L50)

pgindent-style inconsistency: ComposeType's closing line renders as }\t\t\tComposeType; (and line 65 as const\t\tComposeType *), unlike the sibling structs ComposeToken/ComposeRule/ComposePrec which use a single space (} ComposeToken;). This happens because ComposeType is being treated as a known type in a typedefs list while the others are not. Ensure git diff --check is clean and run pgindent so all four Compose* typedefs format consistently. Confidence: high.

---

📄 src/test/modules/grammar_ext_compose/compose_ext_hotel.c (L11-L11)

This comment describes a standalone-load scenario (hotel without alpha) that is never exercised by t/001_compose.pl. Hotel is only ever loaded as alpha,hotel (Test 6) or in the heavy combo alpha,beta,golf,hotel (Test 8) -- alpha always supplies K_GRAMMAR_BRAVO. Worse, the comment claims non-deterministic behavior ("the rebuild either resolves them later or errors at compile time") while the spec below is registered with .expect_failure = false, a definitive success contract. Speculative documentation for an untested, ambiguous path is a footgun: it invites a reader to assume standalone-hotel is a covered case with a defined outcome, when it is neither. Trim this to describe only the actual, tested behavior (cross-ext precedence reference with alpha present). Confidence: high.

---

📄 src/test/modules/grammar_ext_compose/compose_ext_hotel.c (L39-L44)

This paragraph documents behavior (standalone load naming an unknown symbol, forward-reference resolution) that no test in t/001_compose.pl exercises, and asserts a non-deterministic outcome ("either resolves them later or errors at compile time") that contradicts the .expect_failure = false set below. Comments must describe current, deterministic behavior. Remove or rewrite to match what the tests actually assert. Confidence: high.

---

📄 src/test/modules/grammar_ext_compose/compose_ext_india.c (L18-L20)

Comment/code symbol mismatch: the productions here abbreviate the token as bare X, but the declared token and every rule below use K_GRAMMAR_XX (lexeme grammar_xx). There is no symbol named X, so the header doesn't match the actual grammar. Use the real symbol name to keep the comment accurate. (low confidence, documentation only)

💡 Suggested change

Before:

 *   india_if ::= K_GRAMMAR_INDIA X K_GRAMMAR_THEN india_if
 *   india_if ::= K_GRAMMAR_INDIA X K_GRAMMAR_THEN india_if K_GRAMMAR_ELSE india_if
 *   india_if ::= X

After:

 *   india_if ::= K_GRAMMAR_INDIA K_GRAMMAR_XX K_GRAMMAR_THEN india_if
 *   india_if ::= K_GRAMMAR_INDIA K_GRAMMAR_XX K_GRAMMAR_THEN india_if K_GRAMMAR_ELSE india_if
 *   india_if ::= K_GRAMMAR_XX

---

📄 src/test/modules/grammar_ext_compose/t/001_compose.pl (L171-L171)

Bug: this eval { $node->start; 1 } cannot trap a failed startup. PostgreSQL::Test::Cluster::start() calls BAIL_OUT("pg_ctl start failed") when the postmaster does not come up (only fail_ok => 1 makes it return 0 instead). BAIL_OUT aborts the entire test file and is not caught by eval, so:

• Per the actual C code, alpha+foxtrot register() both succeed (foxtrot has expect_failure=false and register() does no token-conflict check); the conflict is only caught at prewarm, where pg_grammar_ext_prewarm() does ereport(FATAL, ...). That FATAL makes the postmaster fail to start.
• Therefore $node->start here BAIL_OUTs the whole file rather than reaching the else { pass(...) } branch.

The branch that this test relies on to "pass" when startup fails is unreachable. Pass fail_ok => 1 (as Test 9 does) so start() returns false and the intended else-branch/log grep runs.

💡 Suggested change

Before:

	my $started = eval { $node->start; 1 };

After:

	my $started = $node->start(fail_ok => 1);

---

📄 src/test/modules/grammar_ext_compose/t/001_compose.pl (L156-L159)

This whole block is wrapped in TODO, and its only non-TODO outcome (else { pass(...) }) is unreachable (see the start() BAIL_OUT issue above). Even the like() inside the if branch is under local $TODO, so a mismatch is a non-fatal TODO, not a failure. Net effect: this test provides no regression coverage for the token-name-conflict path -- it cannot fail even if foxtrot silently registered and mis-parsed. A test that passes with the feature reverted is worthless. Either drive the conflict to an assertable outcome (postmaster refuses to start with fail_ok => 1, then assert the compose-error text in the log, mirroring Test 9), or drop the block until register()-time validation lands.

---

📄 src/test/modules/grammar_ext_compose/t/001_compose.pl (L295-L297)

Test 9's header comment describes a "reduce/reduce conflict" from "two identical stmt ::= K_GRAMMAR_INDIA productions", but the india extension actually builds a dangling-else SHIFT/REDUCE conflict via distinct india_if productions (see compose_ext_india.c: india:if-then vs india:if-then-else), and its stmt rule is a single stmt ::= india_if. The comment contradicts the code it documents; correct it to describe the shift/reduce dangling-else case so it matches the extension.

---

📄 src/test/modules/grammar_ext_compose/t/001_compose.pl (L40-L40)

File::Find is imported but never used anywhere in this file. Remove the dead import to keep the test minimal.

---

📄 src/test/modules/grammar_ext_overlap/overlap_helpers.h (L43-L48)

OvlRule.rhs is a fixed 8-element array, and register_overlap_extension passes it to pg_grammar_ext_add_rule without a length. That API determines the RHS length by scanning for a NULL terminator (while (rhs[nrhs] != NULL) nrhs++; in parser_extension.c). If a future OvlRule literal fills all 8 slots with token names and no trailing NULL, the scan reads past the array bounds (out-of-bounds read -> crash/UB). Current callers all NULL-terminate, so this is a latent footgun rather than an active bug. Consider documenting the NULL-termination requirement here, or making it robust by using the array size as an upper bound (e.g. stop at lengthof(r->rhs)). Confidence: moderate.

---

📄 src/test/modules/lime_in_process_smoke/lime_in_process_smoke.c (L79-L79)

The snapshot is released here via lime_snapshot_release(), but the production code that consumes the exact same ParserSnapshot * returned by this compiler API (parser_pushparse.c) releases it via snapshot_release() (declared in <lime/snapshot.h>). This module includes only <lime/lime_compiler.h> and <lime/parser.h> and never includes <lime/snapshot.h>, and lime_snapshot_release appears nowhere else in the tree. If lime_snapshot_release is not an actual exported symbol of the Lime libraries, this shared module will fail to link, breaking the build for this commit. Confirm the symbol name against the Lime headers, or use the same snapshot_release() the production path uses. (moderate confidence)

---

📄 src/test/modules/lime_in_process_smoke/lime_in_process_smoke.c (L59-L59)

Stray hyphen splits the identifier: this should read lime_compile_grammar_text, not lime_compile_grammar_- text. Fix the comment so the referenced symbol name is correct. (high confidence)

---

📄 src/test/modules/lime_in_process_smoke/lime_in_process_smoke.c (L77-L78)

The header comment states the function returns either "ok: snapshot built" or "error: ", but the code actually appends a raw heap pointer: "ok: snapshot built (snap=%p)". Beyond the stale comment, printing a live heap address into user-visible SQL output is non-deterministic (varies by run/ASLR) and leaks address-layout information. The TAP test only needs to confirm the snapshot is non-NULL; drop the pointer from the returned string. (high confidence)

---

📄 src/test/modules/lime_in_process_smoke/lime_in_process_smoke.c (L22-L22)

Committed source should not reference a private, non-tree note path. .agent/notes/track-b-phase2-design.md is not part of the repository, so this reference is dead for any reader of the patch. Remove it (the same reference also appears in t/001_smoke.pl). (high confidence)

---

📄 src/test/modules/lime_in_process_smoke/t/001_smoke.pl (L68-L68)

Portability bug (high confidence): this regex hard-codes a 0x prefix and lowercase hex, but the matching text is produced by appendStringInfo("...(snap=%p)", (void *) snap) in lime_in_process_smoke.c. PostgreSQL's %p (src/port/snprintf.c:fmtptr) delegates to the platform C library's snprintf("%p", ...), whose output is implementation-defined. On Windows/MSVC %p emits uppercase hex with NO 0x prefix (e.g. 00000000012AB340), so this assertion will fail there. Since the module's meson.build has a host_system == 'windows' build path, Windows is a supported target for this test. Match the pointer loosely instead of assuming glibc formatting.

💡 Suggested change

Before:

+like($result, qr/^ok: snapshot built \(snap=0x[0-9a-f]+\)/,

After:

+like($result, qr/^ok: snapshot built \(snap=/i,

---

📄 src/test/modules/lime_in_process_smoke/t/001_smoke.pl (L50-L51)

Non-upstreamable WIP/project-tracking content (medium confidence). This test file's leading comments reference internal artifacts ('.agent/notes/track-b-phase2-design.md', 'Phase 4 Track B Phase 2') and speculate about options (a)/(b)/(c) and whether the 'in-process' label is misleading. Per PostgreSQL comment discipline, comments must describe what the code does now, not internal roadmap notes or future-tense plans ('will be updated'). Trim to describe the actual test behavior before this is list-ready.

---

📄 src/test/modules/lime_in_process_smoke/t/001_smoke.pl (L64-L65)

Fragility (low/medium confidence): the grammar is interpolated into SQL via a bare $$...$$ dollar-quote. The current fixtures don't contain $$, so this works today, but any future fixture containing the sequence $$ (grammars with actions/macros are a plausible source) would prematurely terminate the dollar-quote and break the query in a confusing way. Use a distinctive tag such as $grammar$...$grammar$ to make interpolation robust.

💡 Suggested change

Before:

+my $result = $node->safe_psql('postgres',
+	"SELECT lime_in_process_compile(\$\$$trivial_grammar\$\$)");

After:

+my $result = $node->safe_psql('postgres',
+	"SELECT lime_in_process_compile(\$grammar\$$trivial_grammar\$grammar\$)");

---

📄 src/test/modules/lime_in_process_smoke/t/001_smoke.pl (L82-L83)

Weak assertion (medium confidence): this only checks an error: rc=<int> msg= prefix, which the module emits for ANY failure (per lime_in_process_smoke.c the error branch fires whenever rc != 0 || snap == NULL, with msg=%s from whatever err string Lime returns). It does not verify the error is caused by the intended unmatched-brace parse failure, so it would pass for an unrelated failure and would not catch a regression that changed which grammars are rejected. Assert on a substring of the expected parse-error message so the test genuinely exercises the malformed-grammar path.

---

📄 src/test/modules/grammar_ext_overlap/t/001_overlap.pl (L97-L97)

The 3+3+2+3+2 breakdown is wrong. The keyword map is built in registration (alphabetical .so-load) order: duckdb=3, mongo=2, mysql=3, pg_infer=3, quel_lite=2, i.e. 3+2+3+3+2. The total (13) happens to match, but the annotated per-extension distribution is mislabeled and will mislead a maintainer trying to reconcile this magic number with the ext_*.c sources. Fix the breakdown.

💡 Suggested change

Before:

	# Total keyword map should have 13 entries (3+3+2+3+2).

After:

	# Total keyword map should have 13 entries (duckdb 3, mongo 2,
	# mysql 3, pg_infer 3, quel_lite 2).

---

📄 src/test/modules/grammar_ext_overlap/t/001_overlap.pl (L210-L213)

This comment contradicts what the rest of this file asserts. Tests 1 and 6 explicitly assert there is NO .so cache path and NO subprocess/SHA256 rebuild pipeline (the compose is in-process only: pg_grammar_ext_prewarm -> lime_compile_grammar_in_process_ex). Talking about a result "already cached from test 1" and "the SHA256 input" describes a caching mechanism the code does not have. This stale comment misleads readers about the compose model; drop the caching/SHA256 wording.

💡 Suggested change

Before:

	# Forward: alphabetical (already cached from test 1).
	# Reverse: reversed list.  Cache keys MAY differ (extension
	# fragment order is part of the SHA256 input) but BEHAVIOR
	# must be identical.

After:

	# Forward order is exercised by test 1; here we load the same
	# five extensions in reverse and assert identical behavior
	# (the in-process compose is order-independent).

---

📄 src/test/modules/grammar_ext_overlap/t/001_overlap.pl (L197-L200)

These assertions are too weak to catch regressions. qr/\b1\b/ .. qr/\b4\b/ match almost any psql output (NOTICE text, timestamps, row counts, the reduce messages themselves), so they can pass even if a SELECT never ran or returned the wrong value. Anchor to the actual result rows, e.g. match the reduces in $stderr but assert the SELECT values against $stdout with a tighter pattern (or run the SELECTs via safe_psql and compare exact scalars).

---

📄 src/test/modules/grammar_ext_overlap/t/001_overlap.pl (L223-L224)

This assertion only checks that some overlap reduce fired, not that the correct one did. Since each lexeme maps to a unique label (pivot->duckdb:pivot, mongo_find->mongo:find, infer->infer:infer, retrieve_lite->quel_lite:retrieve, describe_mysql->mysql:describe), the loop could pass even if the wrong extension's reduce ran under reverse load order -- which is exactly what this order-independence test is meant to prove. Assert the exact per-lexeme label as tests 2 and 3 already do.

---

📄 src/test/modules/parser_microbench/parser_microbench.c (L51-L52)

Portability failure. PostgreSQL targets Windows/MSVC where clock_gettime/CLOCK_MONOTONIC and struct timespec are not available; direct use here will break the build. The tree already provides portable high-resolution timing in portability/instr_time.h (INSTR_TIME_SET_CURRENT(t) / INSTR_TIME_GET_NANOSEC(t)), which wraps clock_gettime(CLOCK_MONOTONIC) on POSIX, CLOCK_MONOTONIC_RAW on macOS, and QueryPerformanceCounter on WIN32. Use instr_time instead of raw clock_gettime. Confidence: high.

💡 Suggested change

Before:

	struct timespec t0,
				t1;

After:

	instr_time	t0,
				t1;

---

📄 src/test/modules/parser_microbench/parser_microbench.c (L78-L78)

Unchecked clock_gettime return value combined with non-portable API. If clock_gettime fails it returns -1 and leaves t0/t1 with uninitialized stack garbage, yielding a bogus (possibly negative) ns_total. Switching to INSTR_TIME_SET_CURRENT(t0) (see instr_time.h) both fixes portability and removes the unchecked-syscall hazard. Confidence: high.

💡 Suggested change

Before:

	clock_gettime(CLOCK_MONOTONIC, &t0);

After:

	INSTR_TIME_SET_CURRENT(t0);

---

📄 src/test/modules/parser_microbench/parser_microbench.c (L79-L85)

The timed loop can iterate up to INT_MAX times calling raw_parser() with no CHECK_FOR_INTERRUPTS(). A caller passing a large iterations on a slow query makes this SQL-callable function uninterruptible - it cannot be cancelled with a query cancel or SIGINT until it finishes. Add a CHECK_FOR_INTERRUPTS(); inside the loop. Note this adds a tiny per-iteration cost, so document it or check on a stride if measurement fidelity matters. Confidence: high.

💡 Suggested change

Before:

	for (i = 0; i < iterations; i++)
	{
		old = MemoryContextSwitchTo(bench_ctx);
		(void) raw_parser(query, RAW_PARSE_DEFAULT);
		MemoryContextReset(bench_ctx);
		MemoryContextSwitchTo(old);
	}

After:

	for (i = 0; i < iterations; i++)
	{
		CHECK_FOR_INTERRUPTS();
		old = MemoryContextSwitchTo(bench_ctx);
		(void) raw_parser(query, RAW_PARSE_DEFAULT);
		MemoryContextReset(bench_ctx);
		MemoryContextSwitchTo(old);
	}

---

📄 src/test/modules/parser_microbench/parser_microbench.c (L43-L43)

The header advertises a SQL function parser_microbench(query text, iterations int) RETURNS bigint, but the module directory contains only this .c file and a meson.build - there is no extension .control file, no install .sql with the matching CREATE FUNCTION ... AS 'MODULE_PATHNAME', 'parser_microbench' LANGUAGE C, and no regression/TAP test. As shipped the function is never registered and thus unreachable, and there is no test coverage. Per project standards a new module must build and pass tests on its own. Add the SQL registration and at least a smoke test (a raw-nanoseconds benchmark cannot go in deterministic regress expected output; a TAP test that asserts the function returns a positive bigint is the appropriate form). Confidence: high.

---

📄 src/test/modules/parser_microbench/parser_microbench.c (L21-L22)

ASCII/comment discipline: the header uses -- as an em-dash surrogate ("loaded -- baseline number is"). Project rules require ASCII with no em-dash surrogates. Also, the comment references aspirational/internal specifics of the parser-extension series ("the rebuilt .so", "the static base_yyparse path") that describe design context rather than what this code does; comments should describe current behavior. Trim to the mechanics of this function. Confidence: moderate.

---

📄 src/test/regress/pg_regress.c (L1246-L1246)

Switching execl to execlp changes the shell to be resolved via PATH search when the program name has no slash. Today SHELLPROG is /bin/sh (meson) or $(SHELL) (make) — both contain a slash, so execlp behaves identically to execl and this line accomplishes nothing in practice. Worse, it introduces a subtle footgun: if SHELLPROG ever became a bare name, the test harness would execute whatever sh happens to be first on the caller's PATH instead of a fixed absolute path — a non-deterministic/security-sensitive regression for a shell invocation. This one-liner is also unrelated to the parser/grammar refactoring that is the subject of this patch; unrelated changes should be split out or dropped. (moderate confidence) Suggest reverting to execl.

💡 Suggested change

Before:

		execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL);

After:

		execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);

---

📄 src/tools/lime_format (L39-L39)

In-place rewrite uses the platform default encoding and default newline handling. read_text()/read_text() here and the .formatted file written by lime are compared and moved without an explicit encoding= or newline=. On a non-UTF-8 locale (e.g. Windows MSVC builds, or a C locale) this can corrupt non-ASCII bytes or silently normalize line endings when the file is rewritten in place. Since this tool mutates source files in the tree, pass encoding='utf-8' explicitly (and consider newline='') to keep the round-trip byte-exact.

💡 Suggested change

Before:

    original = lime_path.read_text()

After:

    original = lime_path.read_text(encoding='utf-8')

---

📄 src/tools/lime_format (L25-L27)

The --lime binary is never validated before the loop. If the path is missing or not executable, subprocess.run raises FileNotFoundError/PermissionError on the first file, producing an opaque traceback rather than a single clear diagnostic. Validate it up front (e.g. shutil.which(args.lime) / Path(args.lime).is_file()) and exit with a clear message, mirroring the srcdir check above.

💡 Suggested change

Before:

srcdir = Path(args.srcdir).resolve()
if not srcdir.is_dir():
    sys.exit(f'lime_format: srcdir not found: {srcdir}')

After:

srcdir = Path(args.srcdir).resolve()
if not srcdir.is_dir():
    sys.exit(f'lime_format: srcdir not found: {srcdir}')

if shutil.which(args.lime) is None and not Path(args.lime).is_file():
    sys.exit(f'lime_format: lime binary not found: {args.lime}')

---

📄 src/tools/lime_format (L37-L37)

The skip logic is broader than intended. part.startswith(p) matches any path component that merely begins with a skip prefix (e.g. buildfarm/, installer/), and because rel.parts includes the trailing filename, a .lime file whose own name starts with build/install/tmp_install is also silently excluded. For directory exclusion the intent is exact-name matching on directories only; use equality against the directory components (excluding the filename), e.g. any(part in SKIP_PATTERNS for part in rel.parts[:-1]). Silently skipping files here will let lime_format_check fail in CI on files this tool refused to format.

💡 Suggested change

Before:

    if any(part.startswith(p) for part in rel.parts for p in SKIP_PATTERNS):

After:

    if any(part in SKIP_PATTERNS for part in rel.parts[:-1]):

---

📄 src/tools/lime_format (L44-L49)

Failure paths leave stray .formatted artifacts in the source tree. When lime -F exits non-zero (or on any later failure), the sibling .formatted file may already exist next to the source but is never cleaned up before continue. Combined with processing all remaining files before exiting 1, a failed meson compile lime-format can leave the tree in a partially-formatted state polluted with .formatted files. Remove any produced .formatted on the failure branches (the companion lime_format_check isolates work in a TemporaryDirectory to avoid exactly this).

💡 Suggested change

Before:

    if cp.returncode != 0:
        print(f'FAIL {rel}: lime -F exited {cp.returncode}',
              file=sys.stderr)
        print(cp.stderr, file=sys.stderr)
        failures += 1
        continue

After:

    formatted_path = lime_path.with_suffix(lime_path.suffix + '.formatted')
    if cp.returncode != 0:
        print(f'FAIL {rel}: lime -F exited {cp.returncode}',
              file=sys.stderr)
        print(cp.stderr, file=sys.stderr)
        formatted_path.unlink(missing_ok=True)
        failures += 1
        continue

---

📄 src/tools/lime_format (L7-L9)

The header comment asserts external, unverifiable facts ("regression-tested upstream in v0.6.1", "flake.lock pins v0.6.0") that will drift and cannot be checked from this tree. Note that meson.build's comment for this same target states the opposite -- that two passes are needed because the formatter is not idempotent on the first pass. That contradiction should be resolved; comments should explain why the code is shaped this way, not stake claims on upstream version behavior.

---

📄 src/tools/lime_convert_gram.py (L1594-L1597)

The parse-param declarator matching heuristic is fragile and silently drops the local injection for array declarators. For char buf[32], a.split()[-1] is "buf[32]" and .lstrip("*[]") only strips leading chars, leaving "buf[32]", which never equals the ident "buf"; the a.endswith(ident) fallback also fails. The result is that the extra->buf local is silently not injected, producing uncompilable/incorrect generated C. The a.endswith(ident) fallback additionally risks false-positive substring matches (e.g. ident ctx matching a declarator ending in errcontext). This same heuristic is duplicated in _inject_parse_param_locals() below. Consider parsing the trailing identifier robustly, e.g. re.search(r"([A-Za-z_][A-Za-z0-9_]*)\s*(?:\[[^\]]*\])?\s*$", a), and factor the shared logic into one helper.

---

📄 src/tools/lime_convert_gram.py (L1594-L1597)

Duplicate of the fragile declarator-matching heuristic in rewrite_action(). Same problems: array declarators like char buf[32] won't match (lstrip only strips leading *[], so "buf[32]" != "buf"), and a.endswith(ident) can match unintended substrings. Extract a single helper used by both call sites to keep them in sync and fix the array case.

---

📄 src/tools/lime_convert_gram.py (L1170-L1171)

Only the FIRST prefix-referencing mid-rule action per alternative is restructured. After alt.rhs is rewritten to [helper] + rhs[midact_idx+1:], any subsequent mid-rule action in the same alternative is left with unrewritten $N/@N (or now-wrong position indices after the shift), because the loop breaks here. For grammars where an alternative has two or more mid-rule actions that reference parent prefix positions, this silently emits incorrect C (wrong semantic values) rather than raising an error. Prefer failing loudly (raise/SystemExit) when a second prefix-referencing MIDACT is encountered in the same alternative, so the limitation is not a silent correctness hazard.

---

📄 src/tools/lime_convert_gram.py (L1913-L1914)

The default %syntax_error/%parse_failure function hardcodes parser_yyerror for every non-plpgsql grammar. That is correct only for the backend SQL parser; cube, jsonpath, ecpg, etc. need different functions (ecpg uses base_yyerror) and must remember to pass --yyerror-fn. If they forget, the converter silently emits a grammar that references the wrong yyerror, failing only at link/compile time in a way that is hard to trace back to this default. Consider requiring --yyerror-fn (error out when unset for unknown prefixes) rather than defaulting to a backend-specific name.

---

📄 src/tools/lime_convert_gram.py (L1594-L1597)

Fragile declarator-to-ident matching. For an array declarator like char buf[32], a.split()[-1] is "buf[32]" and .lstrip("*[]") only strips leading chars, leaving "buf[32]" (never equal to buf); a.endswith("buf") is also False, so the local is silently not injected and the generated C references an undeclared name. Conversely a.endswith(ident) can false-match a longer identifier (ident ctx matching errcontext). Parse the declarator's trailing identifier robustly (e.g. re.search(r"([A-Za-z_][A-Za-z0-9_]*)\s*(\[[^\]]*\])?\s*$", a)) instead. This same heuristic is duplicated in _inject_parse_param_locals(); extract a shared helper.

---

📄 src/tools/lime_convert_gram.py (L1169-L1169)

Correctness gap masked as a limitation. Only the FIRST prefix-referencing mid-rule action per alternative is restructured; the break here skips any subsequent mid-rule actions in the same alternative. After alt.rhs is rewritten to [helper] + rhs[midact_idx+1:], a later mid-rule action's helper still holds unrewritten $N/@N refs against the OLD positions, so it emits silently miscompiled action bodies (wrong semantic values) rather than erroring out. If any target grammar (pl_gram.y, ecpg) has an alternative with two or more prefix-referencing mid-rule actions, this produces a subtly wrong parser. At minimum this should raise a hard error instead of silently proceeding.

---

📄 src/tools/lime_convert_gram.py (L1913-L1914)

The default %syntax_error/%parse_failure hardcodes parser_yyerror for every non-plpgsql grammar. That is only correct for the backend SQL parser; cube/jsonpath/seg/ecpg and others have different yyerror entry points. Unless --yyerror-fn is passed at every call site, this silently emits a grammar file that calls a non-existent/wrong error function. Consider deriving the default from the name-prefix (e.g. <prefix>error) rather than a single hardcoded literal, so an omitted flag fails loudly instead of miscompiling.

---

📄 src/tools/lime_lint (L65-L67)

False-negative bug: '0 error(s)' in out is a substring test, so any error count ending in 0 (e.g. 10 error(s), 20 error(s), 100 error(s)) also matches and sets error_count_zero = True. Combined with has_failure = cp.returncode != 0 or not error_count_zero, a failing grammar is reported as OK whenever lime happens to emit such a count and does not set a non-zero exit code. Match the boundary explicitly, e.g. anchor on ' 0 error(s)' / '0 error(s),' or (preferably) parse the count with a regex like re.search(r'(\d+) error\(s\)', out) and compare to 0. This is the most serious defect in the script since it defeats the purpose of the lint gate.

💡 Suggested change

Before:

    error_count_zero = ('0 error(s)' in out
                        or 'OK: no diagnostics' in out
                        or '✓ No errors or warnings' in out)

After:

    import re
    m = re.search(r'(\d+) error\(s\)', out)
    error_count_zero = ((m is not None and int(m.group(1)) == 0)
                        or 'OK: no diagnostics' in out
                        or 'No errors or warnings' in out)

---

📄 src/tools/lime_lint (L59-L59)

Non-ASCII character. The check-mark 'U+2713' violates PostgreSQL's ASCII-only source rule and will trip encoding checks when this file is posted to pgsql-hackers. Match the ASCII substring 'No errors or warnings' instead of embedding the glyph.

💡 Suggested change

Before:

    #   pre-v0.5.0: '✓ No errors or warnings'

After:

    #   pre-v0.5.0: 'No errors or warnings'

---

📄 src/tools/lime_lint (L67-L67)

Non-ASCII character in a matched literal (U+2713). Besides the ASCII-only violation, matching the glyph is unnecessary; the ASCII tail 'No errors or warnings' uniquely identifies the pre-v0.5.0 output shape.

💡 Suggested change

Before:

                        or '✓ No errors or warnings' in out)

After:

                        or 'No errors or warnings' in out)

---

📄 src/tools/lime_lint (L63-L64)

Comment references a --lint-strict option that this script does not define (only --lime, --srcdir, --quiet exist). This is an aspirational comment for a feature that has not shipped, which contradicts the comment-accuracy discipline. Either implement the option or drop the sentence.

---

📄 src/tools/lime_lint (L46-L46)

The skip filter uses part.startswith(p), a prefix match on each path component rather than an exact directory-name match. Any directory whose name merely begins with a skip prefix (e.g. buildfarm/, installer/, a .gitignore component) is silently excluded, so .lime files under it would never be linted while still reporting a clean run. No current source dir collides, but this is a latent coverage hazard; prefer exact matching, e.g. part in SKIP_PATTERNS.

💡 Suggested change

Before:

    if any(part.startswith(p) for part in rel.parts for p in SKIP_PATTERNS):

After:

    if any(part in SKIP_PATTERNS for part in rel.parts):

---

📄 src/tools/lime_convert_gram.py (L2404-L2404)

This 'mechanical' converter bakes plpgsql-specific grammar semantics (mirroring the decl_start/decl_sect reduce actions that flip plpgsql_IdentifierLookup) into the driver via a hardcoded token switch. The correctness of the generated parser now depends on an invariant that lives in a different file (pl_gram.y): if those reduce actions change, or another token acquires a state-mutating reduce action, this mirror silently drifts out of sync with no detection, yielding a parser whose scanner state diverges from the grammar. At minimum this coupling should be validated or asserted against the source grammar rather than duplicated by hand in a general-purpose tool.

---

📄 src/tools/lime_format_check (L47-L47)

The skip filter uses part.startswith(p), which over-matches: a legitimately-named directory such as buildfarm, installcheck, or any path component beginning with build/install is silently excluded, dropping its .lime files from the format check and letting non-canonical files pass CI unnoticed. Since these are meant to be exact directory names (build, install, .git, tmp_install), use exact-equality matching instead of a prefix test. Note the identical over-matching pattern exists in the companion lime_format and lime_lint scripts, so fix them consistently.

💡 Suggested change

Before:

+        if any(part.startswith(p) for part in rel.parts for p in SKIP_PATTERNS):

After:

+        if any(part in SKIP_PATTERNS for part in rel.parts):

---

📄 src/tools/lime_format_check (L72-L73)

read_text() (both here and for the formatted file) uses the platform default encoding and performs universal-newline translation. On Windows/MSVC buildfarm members or non-UTF-8 locales this can raise UnicodeDecodeError or normalize newlines differently on the two sides, causing a canonically-formatted file to be reported as drift (false CI failure). Since PostgreSQL must pass on Windows/macOS/BSD, read both sides with an explicit, matching encoding and newline policy, e.g. read_text(encoding='utf-8') (add newline='' if lime emits a specific line ending you must preserve).

💡 Suggested change

Before:

+                src_text = lime_path.read_text()
+                fmt_text = formatted.read_text()

After:

+                src_text = lime_path.read_text(encoding='utf-8')
+                fmt_text = formatted.read_text(encoding='utf-8')

---

📄 src/tools/pgindent/pgindent (L1-L1)

This deviates from the tree-wide convention: every other Perl script in the PostgreSQL source uses #!/usr/bin/perl (e.g. src/tools/copyright.pl, git_changelog, mark_pgdllimport.pl, genbki.pl, and 30+ others). The #!/usr/bin/env form is used only for Python scripts here. This change makes pgindent the sole outlier. Unless there is a stated portability reason discussed on -hackers, this is an unrelated, convention-breaking hunk that should be dropped to keep the diff minimal.

💡 Suggested change

Before:

#!/usr/bin/env perl

After:

#!/usr/bin/perl

---

📄 src/tools/lime_to_bison_gram.py (L217-L217)

The local annotation for rules here declares list[list[str]], but the values actually stored are (rhs, prec) tuples: line 381 does rules.setdefault(lhs, []).append((rhs, prec)), and both the return-type annotation (line 204) and emit_bison's parameter (line 388, unpacked as rhs, prec = alt) expect list[tuple[list[str], str | None]]. The local annotation is inconsistent with the stored element type and with the two other annotations for the same object. No runtime impact, but it will mislead readers and static checkers. Suggest aligning it.

💡 Suggested change

Before:

    rules: "OrderedDict[str, list[list[str]]]" = OrderedDict()

After:

    rules: "OrderedDict[str, list[tuple[list[str], str | None]]]" = OrderedDict()

---

📄 src/tools/pglime (L111-L113)

Argument validation happens after Lime has already run and after the primary .c/.h files were moved. If --aot is set without --aot-output (or --snapshot without --snapshot-output), the script does expensive work and moves the main outputs before erroring, leaving the build in a partially-completed, non-atomic state. Validate the required companion options up front, right after parse_args(), before invoking the generator.

---

📄 src/tools/pglime (L86-L87)

--host-reduce is only meaningful together with --snapshot (per its own help text), but it is added to the command line based solely on args.host_reduce with no check that args.snapshot is set. A misconfigured target passing --host-reduce alone would silently invoke Lime with an inconsistent flag combination. Add an up-front guard rejecting --host-reduce without --snapshot.

[github-actions]

This entire script (and the accompanying .github/workflows/*, .github/windows/manifest.json, and .github/scripts/ai-review/* scaffolding) is unrelated to the parser/scanner/grammar-extension work that makes up the rest of this changeset. A patch destined for pgsql-hackers must do exactly one thing; bundling personal Windows-dependency download tooling and AI-review scaffolding with a core parser change is a non-minimal, non-bisectable bundle that reviewers will reject. Split the CI/tooling additions out of this patch entirely.

[github-actions]

The -Repository default is hard-coded to a personal fork, gburd/postgres. Anyone running the script (or the -Latest lookup / gh run download) silently targets a specific developer's fork instead of the intended upstream, producing wrong or empty downloads. This same personal fork name is leaked across the rest of the .github/ additions (docs, workflows). This is a footgun and personal-config leak that has no place in a mainline patch; if the script is kept at all, the repository must be discovered dynamically (e.g. from the current git remote) rather than defaulted to one contributor's fork.

[github-actions]

The -Token parameter (defaulting to $env:GITHUB_TOKEN) is declared and documented in the usage header but never referenced anywhere in the script body. Authentication is delegated entirely to the gh CLI's own login state, so this parameter is dead surface that misleads callers into thinking token auth is wired up. Remove the unused parameter (and its usage-header mention), or actually consume it (e.g. via $env:GH_TOKEN). Passing a token as a plain string is also a poorer pattern than relying on gh auth.

[github-actions]

$runs.Count -eq 0 is an unreliable empty-result guard. gh ... --limit 1 | ConvertFrom-Json returns a single object (not an array) when there is one result, so .Count may be $null in Windows PowerShell 5.1 vs 1 in PowerShell 7+, and would be $null for a zero-element case in some versions too. Wrap the result in an array to make the count deterministic: if (@($runs).Count -eq 0) and index via @($runs)[0]. Otherwise a no-results case can fall through to $runs[0].databaseId and throw an unhelpful error instead of the intended message.

Suggested change

	if ($runs.Count -eq 0) {
	if (@($runs).Count -eq 0) {

[github-actions]

These instruction lines build strings by concatenating single-quoted literals with $OutputPath, which is inconsistent with the double-quote interpolation ("$OutputPath") used everywhere else in the script and reads poorly. If $OutputPath contains spaces the emitted instruction is only correct by luck. Use consistent interpolation, e.g. Write-Host " $env:PATH = "$OutputPath\bin;$env:PATH"".

[github-actions]

Jobs lack timeout-minutes. A stalled Windows build (nmake, or a curl download hang) can run up to the platform default (up to 6 hours), consuming paid runner minutes. Add timeout-minutes to each job, especially the long-running Windows build jobs.

Suggested change

	build-openssl:
	name: Build OpenSSL ${{ matrix.version }}
	needs: build-matrix
	build-openssl:
	name: Build OpenSSL ${{ matrix.version }}
	needs: build-matrix
	timeout-minutes: 60

[github-actions]

The pristine-commit skip logic can produce false negatives and silently skip needed builds. fetch-depth: 10 may not fetch base.sha, so git rev-list $COMMIT_RANGE can fail; TOTAL_COMMITS then falls back to echo "1" while the for loop may iterate over a different/empty/partial set. If the loop counts fewer pristine commits than expected or the range resolves oddly, PRISTINE_COMMITS -eq TOTAL_COMMITS can evaluate true and set should_build=false, skipping a build that is actually substantive. Use fetch-depth: 0 (or explicitly fetch the base ref) and guard against git rev-list failure rather than defaulting to 1.

Suggested change

	fetch-depth: 10 # Fetch enough commits to check recent changes
	fetch-depth: 0 # Need full history to resolve base..head reliably

[github-actions]

Non-ASCII characters in shell script strings. The lines emit U+2713 (check mark) and U+2192 (right arrow). PostgreSQL sources must be ASCII-only. Replace with ASCII equivalents (e.g., [skip] / ->).

Suggested change

	echo " ✓ Dev setup/version commit (skippable)"
	echo " [skip] Dev setup/version commit (skippable)"

[github-actions]

The vs_version workflow_dispatch input (2019/2022) is declared but never consumed: every job hardcodes runs-on: windows-2022 and writes vs_version = "2022". This is dead/unused input flexibility (YAGNI) that creates drift between the input and what is actually built. Either wire the input through to runs-on and the BUILD_INFO output, or remove the input.

[github-actions]

The header comment references a non-existent consumer. pgpa_scan.c does not include this header and is unrelated (it does "analysis of scans in Plan trees", not lexing). The actual includers are the Lime lexer source pgpa_scanner.lex and the driver pgpa_parser_driver.c. Fix the file name so the comment matches reality.

Confidence: high.

Suggested change

	* Private to contrib/pg_plan_advice/. Both the Lime grammar
	* (pgpa_parser.lime, via its %include block) and the lexer driver
	* (pgpa_scan.c) include this so the token semantic-value union has
	* exactly one definition.
	* Private to contrib/pg_plan_advice/. Both the Lime grammar
	* (pgpa_parser.lime, via its %include block) and the lexer
	* (pgpa_scanner.lex) include this so the token semantic-value union has
	* exactly one definition.