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)

Confirmed. No mailing-list thread, commit, or commitfest entry references this proposal. The nearest topical matches are generic/unrelated. I have enough to report.

🧵 Related discussion

• No pgsql-hackers thread proposes replacing Flex/Bison with a "Lime" parser generator. Keyword searches for Lime return only unrelated authors/domains (Liming Hu, limey.net, milimeter@163.com); no message discusses the tool. (high confidence: no such discussion exists in the index.)

• The only "Lime"-adjacent name matches and the only bison/flex build-system traffic found is Andres Freund's Windows CI series [PATCH v9a 07/22] ci: windows: Install bison flex via msys — that patch keeps bison/flex, it does not replace them. Not related to this PR beyond touching the same tools.

• Semantically nearest prior art on the runtime grammar-extension idea (the "Track B" / contrib/quel, contrib/upsert, runtime grammar-extension API commits here) is the old Parser extensions and Hook for extensible parsing discussions — none adopted, none about swapping the parser generator:

  • Parser extensions (maybe for 10?) (Arcadiy Ivanov, 2016), incl. Tom Lane's reply
  • Hook for extensible parsing. (Julien Rouhaud, 2021)
  • Parser Hook (2021)
  • How to build a new grammer for pg? (2023)

  These are only thematically adjacent (extensible parsing). Confidence that any of them is the origin of this PR: low — none was cited by the PR and none discusses Lime, push-parsing, or removing Bison.

📋 Commitfest

• No commitfest entry found linked to this work.

🧭 Context for reviewers

• This is not a tracked community proposal. There is no thread, RFC, or commitfest entry behind it; the design was never floated on pgsql-hackers. Treat it as an unsolicited, out-of-tree fork.
• Strong red flags for merge readiness, independent of framing:
  • 26 of ~40 commits are WIP: or [DO NOT MERGE] and explicitly note "session fold pending." The series is not squashed or finished.
  • It introduces a hard build-time dependency on a third-party generator ("Lime", pinned via a Nix flake and versions v1.6.1/v1.8.2/v1.10.2), replacing the well-understood Bison/Flex toolchain across the entire tree (backend SQL grammar, ecpg, plpgsql, jsonpath, bootstrap/BKI, replication, guc, psql/pgbench, cube, seg, isolation). This is exactly the kind of dependency swap the project has historically resisted; no consensus was sought.
  • Large .github/ additions (AI PR-review workflow, Windows build docs, Nix flake) are fork infrastructure, not upstream-appropriate.
  • contrib/quel and contrib/upsert "grammar-extension demonstrators" revive Berkeley QUEL and add UPSERT-via-runtime-grammar-extension purely to exercise the new machinery — not features the project wants.
• Recommendation: reject as-is. If any piece is worth pursuing, the runtime grammar-extension concept must be re-proposed on-list first; the wholesale Bison→Lime replacement needs an explicit design thread and buy-in before any code review is warranted. Nothing here can be verified against community discussion because none exists.

_{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 205 issue(s).

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

---

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

pgindent alignment: char chr; uses a single space where a tab is expected (should be char chr;, matching bool boolean; just below). This will not pass pgindent / a clean git diff --check.

💡 Suggested change

Before:

	char chr;

After:

	char		chr;

---

📄 src/backend/parser/parser_extension.c (L858-L863)

Use-after-free: pg_grammar_ext_foreach_token dereferences pending[i].ext->tokens, but those tokens live in ext->context, which pg_grammar_ext_unregister() deletes. If an extension registers successfully and is later unregistered (the pending list keeps a borrowed ext pointer), any subsequent compose that calls this helper reads freed memory. The unregister path only emits a WARNING for the not-yet-composed case and does not remove the entry from the pending array, so the dangling pointer remains reachable. Either forbid unregister after successful register (ereport ERROR), or remove the pending entry and its borrowed pointers on unregister.

---

📄 src/backend/parser/parser_extension.c (L827-L835)

Latent buffer overflow: frag_array is a function-local static allocated exactly once, sized to the npending seen on the first call, but the fill loop below always runs to the current npending. If more extensions register (npending grows) after this array was first built, the loop writes past the end of frag_array. This is only safe under the (undocumented at this call site) invariant that all registration completes before the first call. Size the array on every call (or when npending changes), or assert the invariant.

---

📄 src/backend/parser/parser_extension.c (L452-L453)

Stale comment: the file header states the Track A (fork lime + cc -> dlopen a cached .so) path "has been removed entirely" and this is the in-process implementation. This dispatch_reduce header still describes the removed path ("Called from the rebuilt parser .so", "the rebuild produced bogus serialized C, or the host/.so are out of sync"). Update to describe the in-process compose path so the comment matches current behavior.

---

📄 src/backend/parser/parser_extension.c (L655-L657)

Stale comment: this block describes reduce dispatch resolving "against the host postgres binary at dlopen time" via the rebuilt .so, but per the file header the .so/dlopen path was removed and compose is now fully in-process (Track B). Rewrite to reflect the in-process ParserSnapshot dispatch.

---

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

Missing space before the operator; pgindent/tree style requires symbol != NULL.

💡 Suggested change

Before:

	Assert(symbol !=NULL);

After:

	Assert(symbol != NULL);

---

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

error_lloc is dead: it is assigned in both error branches but then discarded via (void) error_lloc;, and the actual reported location comes from scanner_yyerror() reading the scanner state. Either use it to drive the error location (matching base_yyerror precisely) or remove the variable and both assignments; a store-and-discard variable is misleading and will draw review fire.

💡 Suggested change

Before:

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

After:

		scanner_yyerror("syntax error", yyscanner);

---

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

Dead store: tree is fetched from parse_result(ctx) and then discarded via (void) tree; while the real result is taken from yyextra->parsetree. Drop the parse_result() call and the tree local entirely, or explain why the call is needed for its side effect (it does not appear to have one).

💡 Suggested change

Before:

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

After:

		/* The start-rule action assigns yyextra->parsetree. */
		*result = yyextra->parsetree;

---

📄 src/backend/parser/parser_pushparse.c (L452-L456)

Footgun: pushparse_peek_resolves_ext() hard-codes the DELETE-vs-QUEL discriminator (next != FROM) but is invoked for ANY both-admissible colliding keyword, not just DELETE. pushparse_resolve_collision() reaches the need_peek branch for every entry whose base_code matches and where both meanings are admissible. A second extension registering a different both-admissible verb would silently apply the FROM heuristic and mis-parse with no diagnostic. At minimum, guard this: assert/error when the colliding base token is not DELETE, so an unhandled both-admissible collision fails loudly instead of being silently misrouted.

---

📄 src/backend/parser/parser_pushparse.c (L216-L220)

The composed error message uses capitalized sentences and a trailing period, and is multi-sentence. This string is embedded into an outer errmsg("grammar extension compose failed: %s", ...) in pg_grammar_ext_lock_parser/prewarm, so it violates PostgreSQL's primary-message conventions (start lowercase, no trailing period, single clause). Move the detail into an errdetail/errhint at the caller, or reword to a lowercase, period-free primary message.

---

📄 src/backend/parser/parser_pushparse.c (L471-L477)

DRY / maintenance footgun: this table is a byte-for-byte duplicate of gram.lime's ascii_to_lime_token() (verified identical). The comment acknowledges it "must be kept in sync by hand." Any divergence silently misroutes single-character tokens on the push path. Factor the mapping into a single shared inline (e.g. exported from a header the generated gram.c and this TU both include) so there is exactly one source of truth.

---

📄 src/backend/parser/scan.c (L858-L859)

The reset of the non-ASCII flag and its read are on two different fields. The generated scan.lex resets SCAN_LEX_SAW_NON_ASCII(user) at each string open, which the macro (scan_lex_internal.h line 151) maps to ctx->saw_non_ascii. But the escape helpers (scan_lex_handle_xeescape/xehexesc/xeoctesc) set ctx->extra->saw_non_ascii, and this SCONST emit reads extra->saw_non_ascii. Because extra->saw_non_ascii is only initialized once in scanner_init and never reset per-string, the first string containing a non-ASCII escape leaves it true for the rest of the pre-scan, so every later SCONST is needlessly re-verified via pg_verifymbstr against its own bytes. Worse, for the string that legitimately needs verification the flag may be set but the shadow field the .lex reads is a different variable, so the two never agree. Consolidate on a single field (make the .lex reset and the emit read the same extra->saw_non_ascii).

---

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

Broken indentation that will fail pgindent and obscures intent. case OP: is indented one level deeper than the sibling cases above it, and the if (t->val.str != NULL) body has stray tabs between the keyword/condition and between if and its body. Reformat so all case labels align and the guarded assignment reads normally.

💡 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 (L490-L490)

Capacity doubling has no int-overflow guard: once cap exceeds INT_MAX/2, cap * 2 overflows to a negative/small value and palloc(newcap * sizeof(ScanToken)) is then computed with a bogus (or overflowing) size. The whole input is pre-scanned into this FIFO in one shot, so a pathologically token-dense query drives ntokens without bound. Guard the growth (e.g. cap against a MaxAllocSize-derived limit / use pg_nextpower2 with an overflow check) so large inputs fail cleanly instead of under-allocating.

---

📄 src/backend/parser/scan.c (L989-L990)

The full input is lexed up front in this single loop with no CHECK_FOR_INTERRUPTS. The retired scanner produced tokens lazily as the parser (which does check for interrupts) pulled them; here a very large statement is fully tokenized into the FIFO before parsing begins, so a huge query cannot be cancelled during the pre-scan and consumes O(n) token memory with no incremental release. Add a CHECK_FOR_INTERRUPTS in the pre-scan path (e.g. periodically in scan_emit_cb / push_token) to preserve cancellability on large inputs.

---

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

Inconsistent pointer-arithmetic spacing that pgindent will not fix but reviewers will flag: text +i reads as unary-plus. Use text + i (and likewise the text + i for dashdash below).

💡 Suggested change

Before:

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

After:

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

---

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

Same spacing nit: text +2 / text +1 should be text + 2 / text + 1 for readability and consistency with tree style.

💡 Suggested change

Before:

	memcpy(buf, text +2, hexlen);

After:

	memcpy(buf, text + 2, hexlen);

---

📄 src/backend/parser/scan.c (L547-L547)

start = (int) (text -ctx->scanbuf); should read text - ctx->scanbuf (binary minus). Fix the spacing.

💡 Suggested change

Before:

		start = (int) (text -ctx->scanbuf);

After:

		start = (int) (text - ctx->scanbuf);

---

📄 src/backend/parser/scan.c (L320-L330)

The identical \uXXXX/\UXXXXXXXX end-offset computation (the if (pos >= 1 && ... scanbuf[pos] == '\\' ...) end = pos + 10; else if ... end = pos + 6; else end = pos + 1; if (end > scanbuflen) end = scanbuflen; block) is copy-pasted verbatim in scan_lex_addunicode, scan_lex_handle_unicode, and scan_lex_handle_xeu_second. Extract a small static helper (e.g. unicode_escape_end(ctx, pos)) and call it in all three places (DRY).

---

📄 src/backend/parser/scan.c (L165-L167)

Integer-overflow risk in the literal accumulator growth. extra->literallen and len are int/size_t; extra->literallen + len + 1 is evaluated before being passed to pg_nextpower2_32, and for a multi-hundred-MB literal literallen + len can overflow int (literallen is int per core_yy_extra_type), yielding an undersized buffer and a subsequent memcpy heap overflow. Likewise scan_lex_addlitchar's literalalloc *= 2 has no overflow guard. Since string literals can legitimately be very large, bound these against MaxAllocSize and error out rather than wrapping.

---

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

repl_gram.out is missing here. The Makefile's clean target (lines 50-52) removes both repl_gram.out and syncrep_gram.out as generated Lime artifacts, and repl_gram.c/repl_gram.h are already ignored just like their syncrep_gram counterparts. Since Lime produces repl_gram.out alongside repl_gram.c, it will appear as an untracked file after a build. Add /repl_gram.out for symmetry with /syncrep_gram.out.

💡 Suggested change

Before:

+/syncrep_gram.out

After:

+/repl_gram.out
+/syncrep_gram.out

---

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

Shadow-field hazard causing a real correctness bug. This saw_non_ascii field on ScanLexCtx duplicates the flag that already exists on core_yy_extra_type (accessed as ctx->extra->saw_non_ascii). The escape helpers in scan.c write the real one (ctx->extra->saw_non_ascii at scan.c:439/460/478) and the SCONST emit path reads the real one (scan.c:858), but the per-literal reset in scan.lex (lines 200/210 via SCAN_LEX_SAW_NON_ASCII) writes this dead shadow field, which is never read. In the single-pass pre-scan, extra->saw_non_ascii is cleared only once in scanner_init; once any string literal contains a non-ASCII escape it stays true, so every subsequent SCONST gets a spurious pg_verifymbstr call and the intended per-literal reset is broken. Drop this shadow field and make the accessor macro target ctx->extra->saw_non_ascii.

---

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

This accessor maps to the dead ScanLexCtx::saw_non_ascii shadow field, so the scan.lex per-literal reset it drives never touches the flag that scan.c actually reads (extra->saw_non_ascii, scan.c:858). Point this macro at the real flag: (SCAN_LEX_CTX(u)->extra->saw_non_ascii).

💡 Suggested change

Before:

#define SCAN_LEX_SAW_NON_ASCII(u)    (SCAN_LEX_CTX(u)->saw_non_ascii)

After:

#define SCAN_LEX_SAW_NON_ASCII(u)    (SCAN_LEX_CTX(u)->extra->saw_non_ascii)

---

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

Comment is inaccurate/aspirational and reads as a WIP note. The scan.c litbuf helpers (scan_lex_addlitchar/scan_lex_addlit/scan_lex_litbuf_*) operate directly on the real core_yy_extra_type literal buffer (extra->literalbuf/literallen), not a "parallel C-side accumulator". Per PostgreSQL comment discipline, describe what the code does now rather than a workaround that isn't what shipped.

---

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

The Makefile has no rule to generate the Lime lexer outputs from the .lex sources, but the committed repl_scanner.c #includes repl_scanner_lex.h and syncrep_scanner.c #includes syncrep_scanner_lex.h. These *_scanner_lex.c/.h files are not committed and only exist as generated artifacts (the meson build produces them via custom_target(... command: lime_lex_cmd) from repl_scanner.lex / syncrep_scanner.lex). There is no equivalent .lex rule here or in common.mk / Makefile.global, so make cannot build repl_scanner.o / syncrep_scanner.o -- the header does not exist. This breaks the autotools build entirely and drifts from meson.build. Add rules to generate repl_scanner_lex.c/.h and syncrep_scanner_lex.c/.h from the .lex files (mirroring repl_gram.c: repl_gram.lime), e.g. a %_scanner_lex.c rule invoking lime -d. (or whatever lime_lex_cmd maps to). (high confidence)

---

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

The compiled objects also depend on the generated lexer headers, not just the grammar headers: repl_scanner.c includes repl_scanner_lex.h and syncrep_scanner.c includes syncrep_scanner_lex.h. These are missing from the forced-dependency lines, so under a parallel build (make -j) repl_scanner.o / syncrep_scanner.o can be compiled before their lexer headers are generated, causing an intermittent race/failure. Add repl_scanner_lex.h and syncrep_scanner_lex.h to the respective dependency lines. (high confidence)

---

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

clean does not remove the generated lexer artifacts repl_scanner_lex.c, repl_scanner_lex.h, syncrep_scanner_lex.c, syncrep_scanner_lex.h (produced from the .lex sources). New generated artifacts must be removed by clean. Add them here. (high confidence)

---

📄 src/backend/replication/repl_gram_yytype.h (L42-L47)

Dead union members. Only str, uintval, and recptr are ever assigned/read (in repl_scanner.c's repl_emit_cb and the grammar's A.str/A.uintval/B.recptr accesses). boolval, node, list, and defelt are never referenced anywhere in the replication subsystem.

The non-terminal %type declarations in repl_gram.lime ({bool}, {Node *}, {List *}, {DefElem *}) do NOT route through this token-value union -- Lime generates its own internal minor-value union for non-terminals, and the action bodies assign directly to the rule value (e.g. R = true, R = (Node *) cmd), never to YYSTYPE.boolval/.node/.list/.defelt. This is a %token_type union; only members carried by actual tokens (SCONST/IDENT -> str, UCONST -> uintval, RECPTR -> recptr) belong here.

Drop the four unused members to avoid dead scaffolding (YAGNI). (high confidence)

💡 Suggested change

Before:

	bool		boolval;
	uint32		uintval;
	XLogRecPtr	recptr;
	Node	   *node;
	List	   *list;
	DefElem    *defelt;

After:

	uint32		uintval;
	XLogRecPtr	recptr;

---

📄 src/backend/replication/repl_gram_yytype.h (L35-L37)

Inaccurate comment. This claims non-terminals with %type declarations "read the relevant member directly in their action bodies" via this union. That is not how Lime works: non-terminal values live in Lime's own generated minor-value union, and the actions in repl_gram.lime never touch boolval/node/list/defelt. This union is the token semantic-value type only. The sentence should be corrected to describe token payloads, not non-terminal %type members. (high confidence)

---

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

Two consecutive, contradictory "Opaque scanner state" comment blocks precede this struct. The first block claims the fields track "a staging buffer for delimited-identifier collection" -- but no such field (e.g. xdbuf) exists in SyncRepYyScanner, and the second block explicitly states no xdbuf field is needed. This is a stale/duplicated comment left from editing. Delete the first block and keep only the accurate second one. Comments must describe current behavior. (high confidence)

---

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

The pos field is dead scaffolding. In syncrep_scanner.c it is written exactly once (s->pos = 0; in syncrep_scanner_init) and never read; the comment itself concedes it is "advisory only." YAGNI: drop the field and the corresponding initialization to avoid unused state and maintenance confusion. (high confidence)

---

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

This .gitignore removes /jsonpath_scan.c (now a committed source file) but fails to add the newly-generated /jsonpath_gram.out artifact. The updated Makefile clean target runs rm -f jsonpath_gram.c jsonpath_gram.h jsonpath_gram.out, confirming jsonpath_gram.out is produced by the lime -d. build step and is not committed. The parallel change in src/backend/parser/.gitignore correctly added /gram.out for exactly this reason. As written, jsonpath_gram.out will show up as an untracked file in git status after every build. Add /jsonpath_gram.out here to stay consistent with the parser directory and keep the working tree clean.

💡 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/repl_scanner.c (L396-L397)

Correctness bug: the catch-all lexer rule emits token -1 (repl_scanner.lex:186), and repl_emit_cb pushes it into the FIFO with code = -1. But replication_yyparse terminates on if (token <= 0), treating -1 as EOF and feeding token 0 (end-of-input) to the parser WITHOUT ever passing the -1 token. The parser then sees a valid-looking premature EOF rather than an unexpected token, so input containing an unrecognized character can be silently accepted instead of raising a syntax error. This diverges from the retired flex/bison scanner, where the catch-all character reached Bison and produced a syntax error. The -1 sentinel must be mapped to a real token the grammar rejects (or handled explicitly before the token <= 0 EOF check).

---

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

UCONST parsing has two defects. (1) errno = 0 is set but never checked after strtoul, and endp is never checked for a trailing non-digit, so overflow (ERANGE) and malformed input are silently accepted. The tree's own convention (pgoutput.c:326-337) checks errno != 0 || *endptr != '\0' and bounds against PG_UINT32_MAX. (2) When len >= sizeof(buf) (a literal of 32+ digits), n is clamped to sizeof(buf)-1, silently truncating the literal and producing a wrong value instead of an error. Both cases can mis-parse a replication command argument rather than reporting a syntax error, diverging from the promised identical behavior.

---

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

RECPTR parsing via sscanf(buf, "%X/%08X", &hi, &lo) is fragile and diverges from the tree's convention. In scanf, the 08 is a maximum field width (the 0 flag is not meaningful), so lo consumes at most 8 hex digits and an over-long low part is parsed differently than the upstream %X/%X used elsewhere (e.g. libpqwalreceiver.c). Also hi/lo are unsigned int, which is not guaranteed 32-bit on all platforms and provides no overflow detection (e.g. FFFFFFFFF/0 silently overflows). Prefer %X/%X and validate/bound the parsed halves, or reuse an existing LSN parser.

---

📄 src/backend/replication/repl_scanner.c (L270-L271)

Dead fields: s->input and s->len are written here but s->input is never read again and s->len is only used locally during init (all tokenization happens up-front in ReplLexFeedBytes). Per YAGNI/minimalism, drop input from the struct and make len a local variable to avoid implying a per-yylex re-scan that does not occur.

---

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

This comment drifted from the code below it. The value-bearing case does not pstrdup, and there is no yylval->str in this driver -- the copy goes into tok.str via palloc/memcpy. Per PG comment discipline, describe what the code does now, e.g. "copy the matched span into tok.str for value-bearing tokens". (low confidence on impact, high confidence the comment is inaccurate)

---

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

The manual palloc/memcpy/NUL-terminate is exactly what pnstrdup(text, len) does. Reusing it is more idiomatic and DRY, and removes the risk of an off-by-one in the manual copy:

tok.str = pnstrdup(text, len);

💡 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/replication/syncrep_scanner.c (L6-L7)

Version-pinned and historical narrative ("Lime v0.2.2", "replaces the hand-rolled tokenizer that used to live in this file") is the kind of comment that drifts and wastes reviewer time. Comments should describe what the file does now, not its migration history or a specific tool version. Consider trimming to the current responsibilities of the driver. (style/maintainability, not a functional bug)

---

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

The make build is out of sync with meson.build and will not compile jsonpath_scan.o. src/backend/utils/adt/meson.build generates jsonpath_scan_lex.c/jsonpath_scan_lex.h from jsonpath_scan.lex via lime -X (lime_lex_cmd), and the committed jsonpath_scan.c #includes jsonpath_scan_lex.h. But this Makefile has no rule to produce those files, and src/Makefile.global.in/common.mk provide only a flex pattern rule, not a lime -X one. Under make, jsonpath_scan_lex.h is never generated, so jsonpath_scan.o fails to build. Add a codegen rule (e.g. jsonpath_scan_lex.c jsonpath_scan_lex.h: jsonpath_scan.lex running lime -X -d. $<) and make jsonpath_scan.o depend on jsonpath_scan_lex.h. Confidence: high.

💡 Suggested change

Before:

jsonpath_gram.o jsonpath_scan.o: jsonpath_gram.h jsonpath_gram_yytype.h

After:

jsonpath_gram.o jsonpath_scan.o: jsonpath_gram.h jsonpath_gram_yytype.h jsonpath_scan_lex.h

---

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

clean no longer removes the generated scanner artifacts. Per meson.build, jsonpath_scan_lex.c and jsonpath_scan_lex.h are generated from jsonpath_scan.lex (lime -X), yet this rule only deletes the grammar outputs. Leaving generated files behind breaks VPATH/tarball rebuilds and violates the requirement that clean/distclean/maintainer-clean remove all new artifacts. Add jsonpath_scan_lex.c jsonpath_scan_lex.h here (and update .gitignore accordingly). Confidence: high.

💡 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
	rm -f jsonpath_scan_lex.c jsonpath_scan_lex.h

---

📄 src/backend/utils/init/miscinit.c (L1865-L1871)

This comment is inaccurate for two of the three callers of process_shared_preload_libraries(). Besides the postmaster (postmaster.c), this function is also called from PostgresSingleUserMain() in postgres.c (single-user/bootstrap: no postmaster and no fork at all) and from SubPostmasterMain() in launch_backend.c (EXEC_BACKEND, e.g. Windows/MSVC: this runs in each already-forked child, i.e. AFTER the fork, not before). So "here in the postmaster, before backends fork, so no session pays a first-query compose cost" is false in the EXEC_BACKEND and single-user paths -- on Windows every backend re-runs prewarm post-fork, and single-user mode composes inline in the sole process. The code is correct (the parser_locked/npending guard makes it idempotent across all three contexts), but per the comment-accuracy discipline the comment must describe what the code does now in its actual call contexts, not just the non-EXEC_BACKEND postmaster case. Reword to note it runs wherever preload libraries are processed (postmaster before fork on non-EXEC_BACKEND, and each child under EXEC_BACKEND / the single process in single-user mode).

---

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

Inconsistent spacing: text +1 will not pass pgindent (the tree requires text + 1). This is the only pgindent violation I found in the new file, but a non-clean diff draws review time on -hackers.

Confidence: high.

💡 Suggested change

Before:

				addstring_internal(true, text +1, (int) len - 1, s);

After:

				addstring_internal(true, text + 1, (int) len - 1, s);

---

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

The comment asserts precise parser token numbers (TO_P=12 ... STR_INITCAP_P=64 plus DOLLAR=65 ... RBRACE=74), but these cannot be verified against the generated jsonpath_gram.h (it is generated by Lime and not in the tree) and appear inconsistent with the grammar: in jsonpath_gram.lime, TO_P is the first %token declared, so under a lemon/Lime first-seen numbering scheme it would be token 1, not 12. The collision-avoidance design (JP_TOK_BASE=1000, with only ~75 terminals) is sound and safe regardless, but the specific numbers here are a drifting/misleading comment: a future maintainer adding a sentinel may trust these hard-coded values. Suggest stating only the invariant that matters (all parser token codes are well below JP_TOK_BASE) rather than citing exact, unverifiable numbers.

---

📄 src/backend/utils/misc/Makefile (L48-L49)

This removal leaves the make build broken and out of sync with meson.build. The renamed guc-file.c now does #include "guc_file_lex.h" (guc-file.c:45), and meson generates guc_file_lex.c/guc_file_lex.h from guc_file.lex via a Lime custom_target (lime_lex_cmd) plus links guc_file_lex.o. This Makefile has none of that: no rule to generate guc_file_lex.{c,h} from guc_file.lex, and guc_file_lex.o is missing from OBJS -- so make will fail to find guc_file_lex.h when compiling guc-file.c (and would fail to link the Guc* lexer symbols even if it compiled). Deleting clean entirely is also wrong: with guc-file.c now a checked-in source, clean should instead remove the newly generated artifacts guc_file_lex.c/guc_file_lex.h (compare the parser/replication/utils-adt Makefiles in this same series, which keep a clean that rm's their generated .c/.h). Add the Lime-lexer generation rule, add guc_file_lex.o to OBJS, and keep a clean target that removes guc_file_lex.c and guc_file_lex.h. (high confidence)

---

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

The Makefile is out of sync with meson.build and will not build. meson.build generates exprscan_lex.c/exprscan_lex.h from exprscan.lex via lime -X and compiles exprscan_lex.c into the pgbench library. exprscan.c #includes the generated exprscan_lex.h (exprscan.c:56). This Makefile has no rule to generate exprscan_lex.{c,h} from exprscan.lex, and exprscan_lex.o is missing from OBJS. A make-based build (VPATH/tarball) will fail to compile exprscan.c (missing exprscan_lex.h) and, even if it compiled, would fail to link the generated lexer. Add a rule to run the Lime lexer generator on exprscan.lex and add exprscan_lex.o to OBJS. (high confidence)

---

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

This dependency line references exprscan_internal.h but omits the generated header exprscan_lex.h that exprscan.c includes (exprscan.c:56). Because there is no rule to produce exprscan_lex.h in this Makefile, exprscan.o cannot be built. Once a .lex generation rule is added, exprscan.o must depend on the generated exprscan_lex.h (and exprscan_lex.o needs to be added to OBJS). (high confidence)

---

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

clean does not remove the generated lexer artifacts. meson.build generates exprscan_lex.c and exprscan_lex.h from exprscan.lex; once the missing generation rule is added to this Makefile, clean/distclean must also remove them (and add them to .gitignore), otherwise stale generated files linger and pollute VPATH/tarball builds. (high confidence)

---

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

Read-error diagnostics regressed. The old flex path caught the read failure via yy_fatal_error/siglongjmp and reported the error at the ConfigFileLineno where the read actually failed, then aborted the file immediately without processing further tokens. Here guc_slurp_file() reads the whole file up front and this io_error check runs only AFTER the entire token FIFO has been drained, so ConfigFileLineno is now the EOF/last line, not the line of the failed read. The comment claiming this surfaces the error "the same way the flex yy_fatal_error path used to" is inaccurate: line number, ordering, and message text all differ. (moderate-high confidence)

---

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

Correctness regression: partial config is applied on a hard read error. On ferror(fp), guc_slurp_file() still returns the bytes read so far, and guc_scan_init() tokenizes that partial buffer. ParseConfigFp then appends every parsed variable to *head_p/*tail_p and processes include/include_dir directives before io_error is ever inspected at the end of the loop. The retired flex fatal path aborted the file immediately (goto cleanup; OK=false) without recording further variables. Now a truncated read can leave a partially-parsed ConfigVariable list even though OK ends up false. The error should abort before processing tokens, not after. (moderate confidence)

---

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

OOM path leaks 'input'. When GucLexAlloc() returns NULL, the function returns early without pfree(input); the success path below does pfree(input). Although ProcessConfigFile runs in a per-call config_cxt that is deleted afterward, ParseConfigFp is an exported entry point that other callers may invoke in a longer-lived context, and this is inconsistent with the success path. Free the buffer before returning. (low-moderate confidence)

💡 Suggested change

Before:

		/* Out of memory: leave queue empty; ParseConfigFp will see EOF. */
		return;

After:

		/* Out of memory: leave queue empty; ParseConfigFp will see EOF. */
		pfree(input);
		return;

---

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

Missing space after '!=' -- pgindent/style. Should be 'if (text != NULL)', consistent with the rest of the file. (high confidence)

💡 Suggested change

Before:

	if (text !=NULL)

After:

	if (text != NULL)

---

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

Header comment is inaccurate and self-contradictory. This file is no longer a "Hand-rolled scanner"; the tokenizer now lives in guc_file.lex and is compiled by the Lime lexer subsystem (as the guc_file_lex.h include block and guc_scan_init acknowledge). This file contains only a driver + a shim that consumes Lime-emitted tokens. Point (1) "Scanner (the tokenizer that used to live in guc-file.l's %% section)" describes code that no longer exists here. Fix the comment to describe what the file does now (a token-consuming driver over the Lime lexer). Also, "still emits byte-identical error messages" is contradicted by the new "could not read from configuration file" message, which had no equivalent in the flex path. (high confidence)

---

📄 src/bin/pgbench/exprscan.c (L95-L99)

Per-scan parser context is held in module-scope mutable globals (expr_source/expr_lineno/expr_start_offset/expr_command/last_was_newline plus the FIFO globals expr_tokens/expr_ntokens/expr_tokens_cap/expr_tokens_next), even though the public API is threaded through yyscan_t and already carries an expr_yy_extra slot. The current callers in pgbench.c invoke expr_scanner_init/expr_yyparse strictly single-shot and non-nested, so this is not reachable today, but it defeats the reentrancy the yyscan_t contract implies and is a footgun for any future nested/concurrent use (globals would be silently clobbered, corrupting error offsets and the token stream). Consider stashing this state in PsqlScanState/expr_yy_extra instead of file-scope statics. (confidence: moderate, maintainability)

---

📄 src/bin/pgbench/exprscan.c (L127-L128)

The initial allocation uses pg_malloc_array (overflow-checked), but the growth path uses raw pg_realloc with a hand-rolled newcap * sizeof(ExprToken) multiply, which has no size-overflow guard and is inconsistent with the surrounding convention. Use pg_realloc_array(expr_tokens, ExprToken, newcap) (defined in fe_utils/fe_memutils.h) to keep the overflow check and match the codebase style. (confidence: high, style/safety)

💡 Suggested change

Before:

			expr_tokens = pg_realloc(expr_tokens,
									 newcap * sizeof(ExprToken));

After:

			expr_tokens = pg_realloc_array(expr_tokens, ExprToken, newcap);

---

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

end_offset (which is later fed straight into set_cur_pos to advance scanbufpos, on which psql_scan_get_location and expr_scanner_get_substring depend) is derived from pointer arithmetic on the emit callback's text argument: (text + len) - ctx->input_base. This silently assumes the Lime emit callback always hands back a text pointer inside the exact buffer fed to ExprLexFeedBytes. The sibling Lime driver src/fe_utils/psqlscan.c instead advances the cursor by a lexer-reported consumed count (ctx.consumed / feed_len) rather than trusting the text pointer. If that assumption ever fails, the cursor desyncs and expr_scanner_get_substring (strlen(scanbuf + start_offset)) can read out of range. At minimum assert text >= ctx->input_base && text + len <= ctx->input_base + fed_len here. (confidence: moderate, bug)

---

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

This breaks the Make build and diverges from meson.build. The comment is factually wrong: psqlscanslash.c in this tree is not hand-rolled -- it is a driver shim that #include "psqlscanslash_lex.h" (line 47) and links against the Lime-generated SlashLex* symbols produced from psqlscanslash.lex. meson.build (lines 22-36) generates psqlscanslash_lex.c/.h via a custom_target using lime_lex_cmd and compiles them into psqlscanslash_lib. This Makefile now has no rule to generate psqlscanslash_lex.c/.h from psqlscanslash.lex, and does not add them to the build. psqlscanslash.o (still in OBJS, line 39) will fail to compile because psqlscanslash_lex.h is missing, and even if present, linking psql will fail on undefined SlashLex* symbols. Add a Lime codegen rule (mirroring src/backend/parser/Makefile's gram.c: gram.lime rule) that produces psqlscanslash_lex.c/.h from psqlscanslash.lex, and add psqlscanslash_lex.o to the build, keeping the Makefile in sync with meson.build. (high confidence)

💡 Suggested change

Before:

# psqlscanslash.c is hand-rolled (Phase 2h); no codegen rule.

After:

# psqlscanslash_lex.c/.h are generated from psqlscanslash.lex by Lime.
psqlscanslash_lex.c psqlscanslash_lex.h: psqlscanslash.lex
	lime --lexer -d. $<

---

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

clean/distclean no longer removes the Lime-generated lexer artifacts for this directory. Since psqlscanslash_lex.c/.h are generated at build time (see meson.build lines 22-27) and are not checked in, they must be removed here on clean, otherwise stale generated files are left behind. Add psqlscanslash_lex.c psqlscanslash_lex.h to the removal list. (high confidence)

💡 Suggested change

Before:

	rm -f sql_help.h sql_help.c tab-complete.c

After:

	rm -f sql_help.h sql_help.c tab-complete.c psqlscanslash_lex.c psqlscanslash_lex.h

---

📄 src/fe_utils/Makefile (L53-L53)

Build break, out of sync with meson.build (high confidence). This change deletes the flex codegen for psqlscan.c and asserts "no codegen rule needed", but that is only half correct. psqlscan.c (the new hand-rolled shim) does #include "psqlscan_lex.h" and calls PsqlLexAlloc/PsqlLexSetState/PsqlLexFeedBytes/PsqlLexCurrentState/PsqlLexFree -- all defined in the generated psqlscan_lex.c/psqlscan_lex.h. meson generates these from psqlscan.lex via lime_lex_cmd (lime -X -d@OUTDIR@ @INPUT@) and links psqlscan_lex.c into the library (see src/fe_utils/meson.build). This Makefile now has neither a rule to generate psqlscan_lex.c/.h from psqlscan.lex nor psqlscan_lex.o in OBJS, so the make-based build cannot compile/link psqlscan.c. Add a codegen rule and object, e.g.:

psqlscan_lex.h: psqlscan_lex.c ;
psqlscan_lex.c: psqlscan.lex
	lime -X -d. $<
psqlscan.o: psqlscan_lex.h

and add psqlscan_lex.o to OBJS. Fix the comment accordingly -- lexer codegen IS still needed; only the old flex scanner is gone.

---

📄 src/fe_utils/Makefile (L66-L66)

clean/distclean no longer removes the generated lexer artifacts (high confidence). Removing rm -f psqlscan.c is correct now that psqlscan.c is a checked-in source, but the new generated files psqlscan_lex.c and psqlscan_lex.h (produced from psqlscan.lex, per meson.build) are not cleaned. Add them here, e.g. rm -f psqlscan_lex.c psqlscan_lex.h.

💡 Suggested change

Before:

	rm -f libpgfeutils.a $(OBJS) lex.backup

After:

	rm -f libpgfeutils.a $(OBJS) lex.backup
	rm -f psqlscan_lex.c psqlscan_lex.h

---

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

This change is a portability regression. The removed #else /* ! x86_64 */ branch existed to #undef the AVX runtime-check symbols on non-x86_64 targets, guarding "universal" macOS builds: pg_config.h gets USE_AVX2_WITH_RUNTIME_CHECK/USE_AVX512_CRC32C_WITH_RUNTIME_CHECK/USE_AVX512_POPCNT_WITH_RUNTIME_CHECK defined because the build host is x86_64, but the aarch64 slice must not compile that AVX code. These symbols are still consumed in checksum.c, pg_crc32c_sse42.c, and pg_popcount_x86.c. By collapsing this into an #elif, the #undefs are gone, so on the aarch64 slice of a universal build those AVX symbols remain defined and the AVX intrinsics code will be compiled, breaking the aarch64 build. The refactor to #elif for USE_NEON is fine, but the three #undefs must be preserved (e.g. in an #else/#endif on the non-x86_64 side, or moved so they still run when !defined(__x86_64__)). (high confidence)

💡 Suggested change

Before:

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

After:

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

#if !defined(__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
#endif

---

📄 src/bin/psql/psqlscanslash.c (L64-L67)

These four scanner-state values are stored in file-scope globals, which contradicts the design invariant documented in psqlscan_int.h ("all lexer state lives in PsqlScanState rather than in file-static variables ... The lexer is recursion-safe because the entire input cursor and buffer stack live in PsqlScanStateData"). Using globals makes the slash scanner non-reentrant across concurrent PsqlScanState instances and is a regression from the flex-era design, where such per-scan state was kept in the scan state (yyextra). These belong as fields in PsqlScanStateData (like paren_depth), passed through PsqlEmitCtx, not as translation-unit globals. Confidence: high.

---

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

The STOP_VAR_EXPAND branch is dead code in the slash scanner: psqlscanslash.lex never sets ctx.stop_kind = STOP_VAR_EXPAND (variable substitution in slash args is always expanded inline via appendPQExpBufferStr in arg_var_plain/bq_var_plain). Only the SQL scanner (psqlscan.c) uses STOP_VAR_EXPAND. Since ctx.var_value/var_name are never populated here, this branch cannot fire; consider dropping it and folding it into the "not used by slash scanner" default to avoid a misleading, untestable code path. Confidence: high.

---

📄 src/bin/psql/psqlscanslash.c (L240-L242)

In the SLASH_LEX_ERROR fallback, psqlscan_emit() dereferences state->output_buf unconditionally (appendBinaryPQExpBuffer(output_buf, ...)). psql_scan_slash_command_end() runs slash_scan_run() with state->output_buf == NULL, so if this path were ever reached in the XSLASHEND state it would be a NULL-pointer dereference. Today the XSLASHEND rules include a catch-all /[\x00-\xff]/, so SLASH_LEX_ERROR is unreachable there, but this defensive branch is unsafe if that ever changes. Guard against a NULL output_buf (or advance without emitting when output_buf is NULL). Confidence: moderate.

---

📄 src/bin/psql/psqlscanslash.c (L15-L15)

Comment discipline: this file header narrates external tool provenance and internal design labels ("Lime v0.2.2's lexer subsystem", "Strategy-D streaming driver") rather than describing what the code does. Version-pinned tool references and internal strategy codenames will drift and are the kind of narrative a pgsql-hackers reviewer will ask to trim. Describe the driver's behavior directly instead. Confidence: low.

---

📄 src/fe_utils/psqlscan.c (L389-L391)

Frontend convention violation: use pg_fatal() (from common/logging.h, already included) instead of raw fprintf(stderr,...)+exit(1). pg_fatal emits the standard program-name prefix and is the pattern used throughout src/fe_utils (archive.c, recovery_gen.c) and psql (mainloop.c, startup.c). This applies to both this can't-happen branch and the one in psql_classify_eol.

💡 Suggested change

Before:

		default:
			fprintf(stderr, "invalid scan result\n");
			exit(1);

After:

		default:
			pg_fatal("invalid scan result");

---

📄 src/fe_utils/psqlscan.c (L438-L440)

Same convention issue: replace fprintf(stderr,...)+exit(1) with pg_fatal() for this can't-happen state.

💡 Suggested change

Before:

		default:
			fprintf(stderr, "invalid scan state\n");
			exit(1);

After:

		default:
			pg_fatal("invalid scan state");

---

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

Hot-path allocation churn: a fresh lexer is allocated (PsqlLexAlloc) and freed (PsqlLexFree) on every iteration of this for(;;) loop. Because the loop re-enters after every stop point (STOP_NONE, STOP_VAR_EXPAND, and PSQL_LEX_ERROR all continue), a single psql_scan() over a large buffer performs one malloc/free pair per token boundary. This is on the psql/pgbench/pg_dump input path where the flex-era scanner reused one buffer. If the generated API permits it, hoist PsqlLexAlloc/PsqlLexFree out of the loop and reset the lexer per iteration (PsqlLexSetState already re-seeds the start state). (moderate confidence: the psqlscan_lex.h lifecycle contract is in a generated header not present in this change, so please confirm reuse is supported.)

---

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

ctx.consumed is set by .lex action bodies (PSQL_TERMINATE_AT computes (matched - buf) + mlen) and is used here to advance the cursor without any upper-bound check against feed_len. If a future rule (or a pushback miscalculation) ever yields consumed > feed_len, advance_cur_pos over-advances past len; a later iteration then computes feed_len = len - pos < 0, which becomes a huge size_t when cast at PsqlLexFeedBytes, causing an out-of-bounds read. Add a defensive Assert(ctx.consumed <= (size_t) feed_len) before advancing.

💡 Suggested change

Before:

		advance_cur_pos(state, (int) ctx.consumed);

After:

		Assert(ctx.consumed <= (size_t) feed_len);
		advance_cur_pos(state, (int) ctx.consumed);

---

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

Dead fields. var_text/var_text_len are never assigned or read anywhere in the tree (only referenced in this header). They exist solely for the STOP_VAR_RECURSE path, which is itself never set: psql_emit_var_plain() handles recursion by echoing the raw text inline and returning false, so STOP_VAR_RECURSE is only reached in dead switch branches marked "Not used". Drop these two fields (and the associated STOP_VAR_RECURSE enumerator) as speculative scaffolding; a struct member with no producer/consumer is YAGNI. (high confidence)

---

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

This enumerator is never assigned anywhere. The recursion case in psql_emit_var_plain() echoes the raw text inline and returns false; no code path sets stop_kind = STOP_VAR_RECURSE. Both drivers list case STOP_VAR_RECURSE: only under a "Not used" comment. Remove it (and the dead var_text/var_text_len fields) rather than shipping an unused stop kind. (high confidence)

---

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

Comment contradicts the actual prototypes and behavior. It states all four helpers return bool ("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 void and their callers unconditionally LEX_SKIP(). The claim that psql_emit_var_plain's result is "STOP_VAR_EXPAND or STOP_VAR_RECURSE" is also wrong: the function returns true only for the STOP_VAR_EXPAND case and returns false (inline echo) for recursion, so STOP_VAR_RECURSE is never produced. Fix the comment to describe only psql_emit_var_plain as returning bool. (high confidence)

---

📄 src/include/fe_utils/psqlscan_emit.h (L22-L23)

Misleading macro documentation. The comment says the action body records consumed = (matched - buf) + matched_len (- pushback if any), but PSQL_TERMINATE_AT never subtracts pushback: it computes (matched - buf) + mlen. Callers that used LEX_PUSHBACK (e.g. the slash cmd_end/arg_end rules) compensate by passing mlen = 0 by hand. Document that the caller must pass an already-pushback-adjusted mlen; the "- pushback if any" phrasing implies the macro does it, which is a footgun that leads to an over-advanced cursor if a caller passes matched_len while also pushing bytes back. (moderate confidence)

---

📄 src/include/fe_utils/psqlscan_emit.h (L88-L93)

Unhygienic macro: PSQL_TERMINATE_AT references a free identifier matched that is not a macro parameter, silently depending on a matched variable being in scope at every call site (it happens to be provided by the Lime-generated action-body context). Combined with the pushback caveat above, this is easy to misuse. Consider taking the match pointer as an explicit parameter (e.g. PSQL_TERMINATE_AT(u, kind, mptr, mlen)) so the dependency is visible, or at minimum document that matched must be in scope. (moderate confidence)

---

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

These compatibility shim macros are dead scaffolding, and their justification is factually wrong. I verified every consumer of this header (psqlscan.c, psqlscanslash.c, pgbench's exprscan.c): none uses the bare identifiers INITIAL/xb/xc/... as code — they all use the ST_-prefixed enum values directly (e.g. exprscan.c: state->start_state == ST_INITIAL, state->start_state = ST_XB). A search for the bare forms returns zero code matches. So the comment claim that "pgbench's exprscan.l embedded INITIAL" and that these must stay "until those files are ported" is false — exprscan is already ported. Beyond being unused (YAGNI), macro-izing an extremely common bare token like INITIAL in a widely-included fe_utils header is a footgun that can silently clobber unrelated identifiers in any translation unit that (transitively) includes this file — the exact hazard the ST_ prefix was introduced to avoid. Remove the shims and their comment.

---

📄 src/include/fe_utils/psqlscan_int.h (L151-L151)

This yyleng field is dead: I searched the whole tree and there is no reader or writer of state->yyleng / ->yyleng / .yyleng anywhere. The three scanners drive matching through PsqlLex*/SlashLex* and the Lime pre-scan; none tracks a struct-level yyleng. Drop the field and its comment (which documents a yyless(N) rewind that no code performs). Unused struct members are churn and mislead readers.

---

📄 src/include/fe_utils/psqlscan_int.h (L208-L208)

This comment is historical narrative, not current-behavior documentation, and it exposes an easy-to-misuse API. Both call sites discard the return value with (void) psqlscan_prepare_buffer(...) and rely solely on *txtcopy, so the returned pointer that "duplicates *txtcopy" is unused surface that only invites double-free/aliasing mistakes. If the return value has no caller, make the function return void; otherwise document the ownership succinctly. Either way, drop the "kept the same name and contract as the flex era" / "treat it as opaque" narrative and describe what the function does now.

---

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

Comments must describe current behavior and explain WHY, not narrate implementation history. These references to "Pre-Phase 2h", flex's machinery, and "The hand-rolled scanners use plain pointers; flex is no longer involved" are transitional narrative that will rot and mean nothing to a pgsql-hackers reader. Describe the current design (ScanState-encoded start_state, owned buffers with a cursor) without the before/after story.

---

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

Same comment-hygiene issue: "Pre-port these owned YY_BUFFER_STATE buf... Now we just track..." is before/after narrative. State only what the fields are now (an owned NUL-terminated buffer plus a cursor). Historical context belongs in the commit message, not the header.

---

📄 src/include/parser/parser_extension.h (L206-L211)

Portability gap in the exported-symbol strategy. The comment asserts that linking with --export-dynamic makes this symbol resolvable by external code. --export-dynamic is a GNU ld / ELF concept; on Windows/MSVC symbols are not auto-exported and a cross-module reference needs PGDLLIMPORT/PGDLLEXPORT handling. If a composed/loaded module must resolve this symbol on Windows, the current strategy will not link. Clarify the export mechanism for non-ELF platforms or annotate accordingly. (moderate confidence)

---

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

Critical build-portability regression (high confidence). $(PYTHON) is only populated when configured --with-python; configure.ac calls PGAC_PATH_PYTHON (which sets @PYTHON@) exclusively under if test "$with_python" = yes. Without that flag, PYTHON expands to empty and this recipe runs ... /src/tools/lime_to_bison_gram.py $< $@ with no interpreter, failing the build. Because preproc.y/preproc.c are build artifacts (see .gitignore), this rule is on the mandatory build path for ecpg on every git build, not a maintainer-only target. The unaccent Makefile guards this exact hazard with ifeq ($(PYTHON),) PYTHON = python endif, but that only works because update-unicode is maintainer-only. Here you either need a fallback plus a hard dependency on Python at configure time, or the derivation should not use $(PYTHON). Note the sibling meson.build uses meson's always-available python, so the two build systems are already out of sync.

---

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

Approach inconsistency worth flagging (moderate confidence). Every other grammar Makefile in this series generates directly from .lime via lime -d. $< (or $(LIME) -d. $< in plpgsql). Only ecpg round-trips gram.lime back to bison via lime_to_bison_gram.py so the legacy parse.pl can run unchanged. This introduces a second, divergent toolchain (Python + a lime->bison converter) solely for ecpg. If this is intended as a transitional scaffold, the "Phase 2k.3" comment is internal-milestone language that won't mean anything to a -hackers reviewer or future maintainer; describe what the rule does and why the round-trip is needed, not the phase number.

---

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

Hardcoded relative path for the grammar source (low confidence). Other Makefiles in this change reference their grammar locally, but ecpg reaches into the backend tree with a bare ../../../backend/parser/gram.lime. This works with the fixed VPATH layout, but for consistency with the rest of the tree consider $(top_srcdir)/src/backend/parser/gram.lime to make the dependency robust to VPATH/out-of-tree builds and readable.

---

📄 src/include/parser/parser_extension.h (L206-L211)

Portability gap: --export-dynamic is a GNU ld / ELF mechanism. On Windows/MSVC symbols are not auto-exported; a symbol resolved across a module boundary needs PGDLLIMPORT/PGDLLEXPORT handling. If this dispatch symbol must be resolved by externally loaded grammar code on Windows, --export-dynamic alone will not suffice. Clarify or fix the export strategy for non-ELF platforms. (moderate confidence)

---

📄 src/include/parser/parser_extension.h (L199-L199)

The claim that --export-dynamic (a GNU ld / ELF mechanism) suffices to export this symbol is Unix-specific. On Windows/MSVC symbols are not auto-exported; a cross-module reference requires PGDLLIMPORT/PGDLLEXPORT. If externally loaded grammar code must resolve this on Windows, the export strategy needs to account for that. (moderate confidence)

---

📄 src/include/parser/parser_extension.h (L305-L305)

This new public runtime grammar-extension API ships with no SGML documentation (verified: no matches for grammar_ext or parser_extension under doc/src/sgml). A user-visible, ABI-defining feature of this size needs user-facing docs plus tests covering error/edge paths, and a reference to the design discussion (pgsql-hackers Message-Id). As-is this reads as WIP, not commit-ready. (high confidence)

---

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

The void *lex parameter is dead weight across this contract: in pgc.c every implementation that takes it does (void) lex; and never uses it, while user is only forwarded to a dispatcher that also ignores it. Worse, at the call sites user and lex are passed as two adjacent untyped pointers (e.g. pgc_handle_c_ident(matched, matched_len, user, lex)), so transposing them compiles silently — a footgun with no type safety. Recommend dropping the unused lex parameter (and user where it is not consumed) rather than carrying dead, untyped params through the interface. At minimum, use the concrete typedefs (e.g. PgcLexer *) instead of void * so the compiler catches misuse.

---

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

Redundant self-declaration: extern YYSTYPE base_yylval; immediately followed by the tentative definition YYSTYPE base_yylval; in the same TU is pointless (the extern adds nothing before a definition in the same file). More importantly, pgc.c separately hand-writes its own extern YYSTYPE base_yylval; (pgc.c:54). This is a DRY/maintainability problem: the shared declaration should live in the header both files already include (preproc_yytype.h), and only parser.c should carry the definition. Drop the local extern lines here and move the cross-module declaration into the header.

💡 Suggested change

Before:

extern YYSTYPE base_yylval;
extern YYLTYPE base_yylloc;
YYSTYPE		base_yylval;
YYLTYPE		base_yylloc;

After:

YYSTYPE		base_yylval;
YYLTYPE		base_yylloc;

---

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

pgindent will not accept this block and it violates minimal-diff. Problems: (1) stray tabs between case and the label (e.g. case CSTRING:); (2) the whole label list and the base_yylloc = loc_strdup(...) body are over-indented one extra level relative to the surrounding switch; (3) a spurious blank line was inserted before break;. Only the token name change (Op -> OP) is functionally required here; the reindentation is churn on lines that did not need to change. Restore the original indentation (a single tab for case, no extra tabs before the label, no blank line before break).

💡 Suggested change

Before:

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

After:

		case OP:
		case CSTRING:
		case CPP_LINE:

---

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

Stray blank line inserted before break;, and the assignment is over-indented. This is whitespace churn that pgindent will rewrite; remove the blank line and align the body to one tab past case.

💡 Suggested change

Before:

						base_yylloc = loc_strdup(base_yylval.str);

			break;

After:

			base_yylloc = loc_strdup(base_yylval.str);
			break;

---

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

This trailing note narrates a migration phase ("renamed from Op in Phase 3 final") instead of explaining the code. Comments should describe current behavior/why, not history. The relevant fact is just that OP is the operator token; drop the phase reference.

💡 Suggested change

Before:

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

After:

		case OP:

---

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

This hand-written ASCII->token switch duplicates the mapping that the Lime conversion tooling generates, and must be kept manually in sync with the symbolic token ids emitted into preproc.h. Any drift (a self-char used by the grammar but missing here, or a symbolic id that changes) silently produces wrong tokens with no compile error whenever the identifier still exists. If the token-id scheme guarantees ASCII bytes already carry their literal meaning, this whole switch is unnecessary and filtered_base_yylex()'s result could be passed through directly; otherwise this table should be generated rather than hand-maintained to avoid divergence.

---

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

base_yyleng is write-only across the entire ecpg tree. It is defined and assigned in pgc.c (set_yytext() sets base_yyleng = len;) but never read anywhere: parser.c's lookahead code saves/restores base_yylval, base_yylloc, and base_yytext but not base_yyleng, and no grammar/header/trailer/lex file references it. Exporting an unused variable via extern is dead scaffolding (YAGNI). Drop this declaration (and the underlying variable/assignment in pgc.c) unless an actual consumer is added. Confidence: high.

Separately, note that an extern variable in a public header that is intended to be read cross-module on Windows/MSVC would need PGDLLIMPORT; that concern is moot here precisely because there is no external reader, which reinforces that the declaration is unnecessary.

---

📄 src/interfaces/ecpg/preproc/pgc.c (L1476-L1480)

Silent data-loss footgun (moderate confidence): when the lexer neither consumes bytes nor emits a token, the entire remaining buffer is discarded (pos = len) and the loop continues without any diagnostic. On malformed ecpg input where the lexer cannot make progress, the rest of the file is silently dropped, producing a wrong translation with no error. The legacy flex scanner would report an error on unmatched input. This path should raise a lexer/parse error instead of silently swallowing input.

💡 Suggested change

Before:

		if (cur_feed_consumed == 0 && !cur_feed_have_token)
		{
			cur_feed_buffer->pos = cur_feed_buffer->len;
			continue;
		}

After:

		if (cur_feed_consumed == 0 && !cur_feed_have_token)
		{
			mmfatal(PARSE_ERROR, "unexpected input at line %d", base_yylineno);
			return 0;
		}

---

📄 src/interfaces/ecpg/preproc/pgc.c (L961-L966)

Unbounded strcat onto a fixed MAXPGPATH buffer (moderate confidence). Unlike the include-path branch below, whose length is pre-checked at 'strlen(ip->path) + strlen(base_yytext) + 4 > MAXPGPATH', this branch reaches strcat(inc_file, ".h") after strlcpy(inc_file, base_yytext, sizeof(inc_file)). If base_yytext filled inc_file up to sizeof(inc_file)-1, appending ".h" overflows the stack buffer. Use bounded strlcat with an explicit room check.

💡 Suggested change

Before:

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

After:

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

---

📄 src/interfaces/ecpg/preproc/pgc.c (L989-L996)

Missing length guard leads to a possible buffer underflow read (moderate confidence). Unlike the quote branch above (which guards with 'strlen(inc_file) <= 2 ||'), this branch calls strcmp(inc_file + strlen(inc_file) - 2, ...) with no check that strlen(inc_file) >= 2. base_yytext can be empty here (e.g. an EXEC SQL INCLUDE <> after the surrounding delimiters are stripped at lines above), so inc_file can be as short as "/" (or shorter with an empty include path), making 'inc_file + strlen(inc_file) - 2' point before the buffer. Guard the length before the strcmp.

💡 Suggested change

Before:

			if (!f)
			{
				if (strcmp(inc_file + strlen(inc_file) - 2, ".h") != 0)
				{
					strcat(inc_file, ".h");
					f = fopen(inc_file, "r");
				}
			}

After:

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

---

📄 src/interfaces/ecpg/preproc/pgc.c (L111-L115)

Integer overflow before allocation (low/moderate confidence). literalalloc is a signed int; for a very large literal the doubling loop 'literalalloc *= 2' can overflow to a negative value, after which realloc(literalbuf, literalalloc) converts it to a huge size_t (or fails) and the subsequent memcpy overruns the buffer. Use size_t for literalalloc and check for overflow (or cap growth), consistent with PostgreSQL's overflow-before-alloc discipline.

---

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

Non-standard spacing 'text +i' / 'text +1' (space before, none after the operator) appears in several places (here, and in the PGC_TOK_PARAM/PGC_TOK_CVARIABLE cases: 'memcpy(nbuf, text +1, ...)', 'memcpy(s, text +1, len - 1)'). This is inconsistent with PostgreSQL style and will churn under pgindent; write 'text + i' / 'text + 1'.

💡 Suggested change

Before:

			slashstar = text +i;

After:

			slashstar = text + i;

---

📄 src/interfaces/ecpg/test/expected/preproc-define.c (L77-L81)

This expected-output change encodes a behavioral regression in the ecpg preprocessor's passthrough of C preprocessor directives. Previously #if 0 / #endif were emitted at column 1 on their own lines (with the surrounding blank line preserved); the new output merges #if 0 and #endif onto the preceding token's line with leading whitespace. This is a user-visible change to the C code ecpg generates. Although #if 0 (whitespace before #) is legal C99 and still compiles, silently editing this golden file to accept the new whitespace/line layout papers over a fidelity drift in the reworked scanner rather than fixing it. Confirm this output change is intended and documented (with a -hackers reference) rather than an accidental consequence of the scanner rewrite; the historical behavior of emitting directives at column 1 on their own line should be preserved.

---

📄 src/pl/plpgsql/src/Makefile (L83-L84)

The make variable $(LIME) is undefined. It is not set in src/Makefile.global.in (which only defines BISON = @BISON@), nor in any other .mk/Makefile fragment, and it is not exported from configure. In GNU Make an undefined variable expands to the empty string, so this recipe becomes -d. pl_gram.lime, which fails and breaks the make build for plpgsql.

This is also inconsistent with every other Makefile in this changeset (src/backend/parser, src/backend/bootstrap, src/backend/replication, src/backend/utils/adt, src/bin/pgbench, src/test/isolation), all of which invoke the tool as the literal lime -d. $<. Use the literal lime here too (or, better, add a proper configure-substituted LIME = @LIME@ to Makefile.global.in and switch all sites to $(LIME) for consistency and out-of-PATH configurability). (high confidence)

💡 Suggested change

Before:

pl_gram.c: pl_gram.lime
	$(LIME) -d. $<

After:

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

---

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

Including postgres.h from a header violates PostgreSQL's include convention: postgres.h must be the first include in every .c file and must never be pulled in transitively via a header. The sibling YYSTYPE headers introduced by this same migration (repl_gram_yytype.h, preproc_yytype.h) deliberately do NOT include postgres.h -- they include only the specific headers the union members require (e.g. nodes/pg_list.h). This header is the outlier. Any consumer that includes this header before its own postgres.h, or in a frontend translation unit, will break the mandatory ordering. Include only the minimal type prerequisites the union actually needs (the List/Oid/bool decls come via plpgsql.h/c.h already through the normal chain).

💡 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 (L8-L10)

This comment is inaccurate. pl_comp.c and pl_exec.c include neither pl_gram.h nor pl_gram_types.h -- verified: only pl_scanner.c includes them. Listing them as consumers that "need the body visible" is misleading. Also, the comment references pl_gram.y as the sync source, but that file no longer exists in the tree (it has been replaced by pl_gram.lime); the invariant it describes points at a retired file. State the actual single consumer (pl_scanner.c) and the current source of truth.

---

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

Aspirational/future-tense comments that reference internal, unshipped migration state ("Phase 2j", "before Phase 2j flips", "Once Phase 2j lands", "legacy build path") violate PostgreSQL comment discipline: comments must describe what the code does now, not a projected future migration. This internal branch/project jargon ("Lime", "Phase 2j") is meaningless to the upstream community and will go stale the moment the migration completes. Rewrite to describe the guard's actual purpose in present tense without referencing internal phase names.

---

📄 src/pl/plpgsql/src/pl_gram_types.h (L103-L105)

Same issue: this comment references the internal "Phase 2j" migration phase and "Lime" in future tense. Describe the guard in present tense without internal project jargon before upstream submission.

---

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

This hand-maintained copy of the plpgsql %union is a correctness footgun: it must stay byte-for-byte identical to the union the parser is generated with, or the semantic-value stack will be interpreted with a mismatched layout, causing wrong parses or memory corruption. The comment even acknowledges the invariant while pointing at a source file (pl_gram.y) that no longer exists. Since lime_convert_gram.py already special-cases plpgsql to emit #include "pl_gram_types.h", prefer generating this header from the grammar's %union rather than maintaining a second synchronized copy by hand -- eliminating the drift hazard entirely (confidence: moderate; verify the converter can emit the header).

---

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

These three prototypes are grouped under the "Scanner functions in pl_scanner.c" comment, but only plpgsql_yy_drain_lookahead is actually defined in pl_scanner.c. plpgsql_yy_get_lookahead and plpgsql_yy_clear_lookahead are the prefix-renamed Lime helpers (per lime_convert_gram.py's comment, they map to Lime's Parse_get_lookahead etc.) generated into pl_gram.c, not pl_scanner.c. Placing them under this comment is misleading; either move them to a separate "Parser functions in pl_gram.c" grouping or adjust the comment. (high confidence)

---

📄 src/pl/plpgsql/src/plpgsql.h (L1315-L1315)

plpgsql_yy_get_lookahead / plpgsql_yy_clear_lookahead are generated into pl_gram.c by Lime. Verify that the generated definitions declare their first parameter as void * as well. If Lime emits the concrete parser type (e.g. yyParser *) instead, this prototype's parameter type disagrees with the definition, which is undefined behavior even though call sites happen to compile via implicit conversion to void *. Confirm the generated signature matches this declaration. (moderate confidence)

---

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

Inconsistent parameter naming for the same Lime parser handle within this block: plpgsql_yy_drain_lookahead names it yypParser while the two below name it yyp. Use one name (yypParser matches the definition in pl_scanner.c and the call sites) for readability. (high confidence)

---

📄 src/test/isolation/.gitignore (L8-L8)

The Lime migration adds a new generated report artifact specparse.out (the Makefile's clean rule now runs rm -f specparse.h specparse.c specparse.out, and specparse.c: specparse.lime invokes lime -d., which emits it). It is not covered by any pattern here, so it will show up as untracked in git status and can be committed by accident. Add it alongside the other generated source files.

Confidence: high.

💡 Suggested change

Before:

-/specscanner.c

After:

/specparse.out

---

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

Convention violation and signature-drift hazard: plpgsql_lime_to_ascii_token is declared here with a function-local extern, and its definition in pl_gram.lime carries only a matching self-extern. PostgreSQL prototypes belong in a shared header so the compiler catches signature drift between the definition and the call site. As written, changing the signature in one place will not be diagnosed. Declare it in plpgsql.h (next to plpgsql_yy_drain_lookahead / plpgsql_yy_get_lookahead, which are already there) and drop the in-body extern. Confidence: high.

---

📄 src/pl/plpgsql/src/pl_scanner.c (L32-L33)

Two consecutive blank lines are introduced here (the added blank line after the new comment block plus the pre-existing blank line before the /* Klugy flag ... */ comment). This is whitespace churn that pgindent / git diff --check discipline flags. Remove one blank line. Confidence: high.

💡 Suggested change

Before:

 * (LBRACE, RPAREN, ...) to the backend grammar's differing values.
 */

After:

 * (LBRACE, RPAREN, ...) to the backend grammar's differing values.
 */

---

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

The Make build is broken for the isolation lexer. This diff adds a rule to generate specparse.c from specparse.lime, but adds no rule to generate specscanner_lex.c/specscanner_lex.h from specscanner.lex. The committed driver specscanner.c does #include "specscanner_lex.h" and calls SpecLexAlloc/SpecLexFeedBytes/SpecLexFeedEOF/SpecLexFree (defined in the generated specscanner_lex.c). Under autoconf/Make:

1. Compiling specscanner.o fails because specscanner_lex.h does not exist and nothing generates it.
2. Even if it compiled, specscanner_lex.o is absent from OBJS, so linking isolationtester fails with undefined references to the SpecLex* symbols.
   The meson build handles this via lime_lex_cmd (see src/test/isolation/meson.build lines 23-28) plus linking spec_scanner_lex, but the Makefile has no equivalent. Add a rule to compile specscanner.lex (e.g. producing specscanner_lex.c/specscanner_lex.h) and add specscanner_lex.o to OBJS. (high confidence)

---

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

This dependency line omits specscanner_lex.h, which specscanner.c includes. Once the missing specscanner.lex generation rule is added, specscanner.o must also be forced to depend on specscanner_lex.h so a stale/absent generated header does not silently break incremental builds. (moderate confidence)

💡 Suggested change

Before:

specparse.o specscanner.o: specparse.h spec_gram_yytype.h

After:

specparse.o specscanner.o: specparse.h spec_gram_yytype.h specscanner_lex.h

---

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

The clean target no longer removes the generated scanner artifacts. Previously specscanner.c was cleaned; now the generated specscanner_lex.c/specscanner_lex.h (produced from specscanner.lex once the generation rule is added) are left behind. Add them to keep make clean complete. (moderate confidence)

💡 Suggested change

Before:

	rm -f specparse.h specparse.c specparse.out

After:

	rm -f specparse.h specparse.c specparse.out
	rm -f specscanner_lex.c specscanner_lex.h

---

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

Stale/contradictory comments describing removed behavior. This header block describes the Track A subprocess pipeline ("queues the extension for rebuild", "subprocess pipeline runs", "$PGDATA/pg_parser_cache", "the cached .so dlopens", "dispatches through the dlopen'd base_yyparse"). Per the authoritative headers, parser_extension.h/.c state the Track A subprocess/dlopen/.so-cache path "has been removed entirely" and Track B in-process compose is the live implementation. This comment describes code that no longer exists and directly contradicts the inline comment at lines 65-70 ("The trampoline now actually fires this callback"). Rewrite the header to describe the current Track B in-process compose flow.

---

📄 src/test/modules/dummy_grammar_ext/dummy_grammar_ext.c (L23-L26)

Stale claims 2-4 reference the removed Track A pipeline (cache key, "cached .so dlopens", "dlopen'd base_yyparse"). None of this exists in the Track B in-process implementation. Update to describe what the smoke test actually proves today (register() succeeds, fragment serializes, in-process compose installs the snapshot, dummy_reduce dispatches).

---

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

This function comment states the callback is "wired but unreachable at runtime" and "Track B ... will exercise this path; until then the body is documentation" -- future tense for behavior that has already shipped. It directly contradicts the very next comment (lines 65-70), which says "The trampoline now actually fires this callback" and the body now logs a NOTICE. Delete or rewrite this stale comment; keep a single accurate description of current behavior.

---

📄 src/test/modules/dummy_grammar_ext/dummy_grammar_ext.c (L71-L74)

Logging raw pointer values via %p produces non-deterministic, platform-dependent output (address, width, 0x prefix vary by platform/ASLR). The file header claims "the test harness can grep for them" -- if any TAP/regress expected output captures this NOTICE it will be flaky and non-portable. Drop the user_data/extra_arg pointer fields from the message; nrhs alone is deterministic and sufficient to prove dispatch fired.

💡 Suggested change

Before:

	ereport(NOTICE,
			(errmsg("dummy_grammar_ext: dummy_reduce fired "
					"(nrhs=%d, user_data=%p, extra_arg=%p)",
					nrhs, user_data, extra_arg)));

After:

	ereport(NOTICE,
			(errmsg("dummy_grammar_ext: dummy_reduce fired (nrhs=%d)",
					nrhs)));

---

📄 src/test/isolation/specscanner.c (L243-L248)

Dead, unreachable code. spec_yyerror() unconditionally calls exit(1) (see line 88), so the SpecLexFree/spec_yyFree/free(input)/return 1 here can never execute, and the comment admits it is kept only "for analyzers." This violates the tree's no-dead-code discipline. Either remove the unreachable cleanup and return (and annotate spec_yyerror as pg_noreturn so analyzers understand), or don't exit inside spec_yyerror for this path. Confidence: high.

---

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

Inconsistent allocation-failure handling: SpecLexAlloc is NULL-checked (lines 229-233), but spec_yyAlloc(spec_palloc)'s result is used directly in spec_yy/spec_yyFree without a NULL check. Since spec_palloc wraps pg_malloc, which itself exit()s on OOM (see fe_memutils.c), the SpecLexAlloc NULL branch is in practice unreachable too. Pick one policy: either both allocation sites are checked, or neither is (rely on pg_malloc's OOM exit). Confidence: high.

---

📄 src/test/isolation/specscanner.c (L140-L145)

Misleading comment. Per specscanner.lex, IDENTIFIER is emitted via a plain LEX_EMIT(IDENTIFIER) whose text points into the input buffer; it does not come from a QIDENT/LEX_BUF_TAKE'd heap copy (there is no QIDENT rule emitting IDENTIFIER). Only SQLBLOCK uses LEX_BUF_TAKE, and there the buffer is freed after the emit callback returns (not "already free'd by the time we get here"). The copy-out is still correct, but the rationale stated here is inaccurate; fix the comment to describe what actually happens. Confidence: high.

---

📄 src/test/isolation/specscanner.c (L54-L56)

Aspirational/future-tense comment referencing an external tracker ticket ("P0-NEW-12") and unshipped upstream work ("When Lime upstream ... lands ... this can collapse"). The tree's comment discipline forbids future-tense/aspirational notes for behavior that hasn't shipped. Describe only what the code does now; drop the ticket reference and the speculative refactor plan. Confidence: high.

---

📄 src/test/isolation/specscanner.c (L196-L203)

spec_yylex is a non-functional stub returning 0 (unreferenced in-tree; callers use spec_yyparse). The future-tense justification ("if a future caller appears we can wire it up") is speculative scaffolding that the tree's comment/YAGNI discipline discourages. The symbol is only needed because isolationtester.h declares it. If the declaration can be dropped, remove both; otherwise keep a minimal stub but replace the future-tense prose with a factual one-line note that it is an unused compatibility stub. Confidence: medium.

---

📄 src/test/isolation/specscanner.c (L169-L174)

INTEGER conversion uses atoi() over a fixed 32-byte truncation buffer with no overflow or error detection: an out-of-range or over-long integer literal in a spec file is silently truncated/coerced with no diagnostic. (The prior scanner also used atoi, so this is not a regression, but it is a robustness gap.) Consider strtol with explicit range/errno checking so a malformed count fails loudly rather than silently producing a wrong value. Confidence: moderate.

---

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

The top header comment contradicts the code and the actual test behavior. It states "this should fail register() with a clear error. expect_failure=true so the helper logs the failure as expected (NOTICE)", but the struct sets .expect_failure = false (below), and the second inline comment gives a different rationale. Verified against the helper (register_compose_extension logs WARNING when !ok && !expect_failure) and the TAP test (Test 4 in t/001_compose.pl is a TODO: block whose own comment documents a "CURRENT GAP": the conflict is NOT rejected at register() time but surfaces later at in-process compile; the harness greps the log directly, not for an expected-failure NOTICE). So the top comment is factually wrong on both counts (claimed expect_failure=true, and claimed register() failure). This is a POLA hazard for a test whose entire value is asserting a specific API contract. Fix the top comment to match reality (expect_failure=false; conflict detection deferred to compile time, tracked in lime-letter-34). Confidence: high.

💡 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.  Per the API
 * contract this ought to be rejected, but token-conflict detection
 * currently happens at in-process compile time, not at register()
 * time (tracked in lime-letter-34).  We therefore set
 * expect_failure=false; the alpha+foxtrot TAP test greps the compile
 * error directly rather than the helper's expected-failure message.

---

📄 src/test/modules/grammar_ext_compose/compose_ext_foxtrot.c (L50-L51)

Comment readability: the mid-token hyphenation "expect_-\n failure=true" and "alpha+ foxtrot" read as line-wrap/OCR artifacts. Even after correcting the semantic contradiction above, ensure the token expect_failure is not split across a line and alpha+foxtrot has consistent spacing. Confidence: low.

💡 Suggested change

Before:

		 * fail.  We mark it as expect_failure=false so the WARNING fires when
		 * standalone-loaded; the alpha+ foxtrot test's TAP harness greps for

After:

		 * fail.  We mark it as expect_failure=false so the WARNING fires when
		 * standalone-loaded; the alpha+foxtrot test's TAP harness greps for

---

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

This comment describes speculative/conditional future behavior ("if we add the symbol-table check") for logic that does not exist. I confirmed there is no register-time symbol-table check in parser_extension.c/.h; undefined RHS symbols are only caught at in-process compose (lime-rebuild) time. Per PostgreSQL comment discipline, comments must state what the code does now, not aspirational "if we add" work. State the actual behavior: the reference to an unknown token surfaces at lime-rebuild time (lime errors on an undefined RHS symbol). [low confidence -> factual: the check is absent]

💡 Suggested change

Before:

	 * surface either at register() (if we add the symbol-table check) or at
	 * lime-rebuild time (lime errors on undefined RHS symbol).

After:

	 * surfaces at lime-rebuild time (lime errors on an undefined RHS symbol).

---

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

compose_reduce() unconditionally writes a pointer-width value: *(void **) lhs_out = NULL. Per the documented reduce ABI in parser_extension.h, lhs_out must be written with the LHS non-terminal's declared C type (*(Type *) lhs_out = v). ComposeSpec supports arbitrary ComposeType.datatype (e.g. "int"), so a spec that declares a non-pointer-width LHS type and reduces to it would make this a wrong-sized / out-of-bounds write that corrupts the parser value stack. This shared reduce callback is a footgun: it silently only works for pointer-width LHS types. Either restrict/document the helper to pointer-typed non-terminals, or derive the write width from the spec.

---

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

Stray double-tab in the typedef name breaks pgindent's canonical } ComposeType; spacing (compare ComposeToken/ComposeRule/ComposePrec above, which use a single space). This whitespace churn will show up under pgindent / git diff --check.

---

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

Same stray double-tab: const ComposeType *t deviates from the single-space const ComposeToken *t / const ComposeRule *r style used in the neighbouring loops. Normalize to const ComposeType *t to keep pgindent clean.

---

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

This comment describes a standalone-load scenario that no test exercises. The TAP test (t/001_compose.pl) only ever loads hotel together with alpha (Test 6: alpha+hotel, and Test 8: heavy load) -- there is no standalone-hotel test. The wording is also speculative/aspirational ("the rebuild either resolves them later or errors at compile time -- either way the test asserts against the postmaster's log"), which violates the comment-accuracy discipline: comments must describe what the code does now, not an either/or outcome, and must not cite a "test asserts against the postmaster's log" that does not exist for this case. Note .expect_failure = false below is only correct for the tested (with-alpha) path; the comment's standalone "errors at compile time" claim directly contradicts it. Trim the comment to the behavior actually tested (cross-ext precedence reference when loaded with alpha).

💡 Suggested change

Before:

	 * Reference an alpha-side token by name.  When loaded with alpha the
	 * precedence applies; when loaded standalone the precedence names a
	 * symbol Lime doesn't know yet.  Lime treats unknown precedence-symbol
	 * names as a forward reference and the rebuild either resolves them later
	 * or errors at compile time -- either way the test asserts against the
	 * postmaster's log.

After:

	 * Reference an alpha-side token (K_GRAMMAR_BRAVO) by name.  This
	 * extension is only loaded together with alpha, which defines
	 * K_GRAMMAR_BRAVO; hotel adds no new token here and only raises the
	 * existing symbol to a binding precedence level.

---

📄 src/test/modules/grammar_ext_compose/compose_ext_india.c (L6-L7)

This file documents a dangling-else SHIFT/REDUCE conflict, but both meson.build (line 22: "india: two identical stmt productions -> reduce/reduce conflict") and t/001_compose.pl (lines 295-296: "registers two identical stmt ::= K_GRAMMAR_INDIA productions -> a reduce/reduce conflict") describe india as a REDUCE/REDUCE conflict built from two identical stmt productions. The grammar in this file is neither identical productions nor a reduce/reduce conflict. Since this is a negative test whose entire value hinges on the composed grammar actually yielding nconflict > 0, these drifted descriptions must be reconciled so the intent is unambiguous and reviewers can confirm the test exercises the gate for the right reason. (high confidence)

---

📄 src/test/modules/grammar_ext_overlap/ext_mongo_jsonb.c (L6-L9)

This header comment is self-contradictory and inaccurate (low/moderate confidence, documentation). It claims "Each takes a JSONB document as RHS" but the rules below are bare single-keyword productions ({"K_MONGO_FIND", NULL} / {"K_MONGO_AGGPIPE", NULL}) with no RHS document symbol at all. It also claims "the value-shaping code lives in the reduce callback", but the shared overlap_reduce in overlap_helpers.h only emits a NOTICE and writes NULL to lhs_out -- there is no value shaping. The sibling ext_duckdb_compat.c / ext_mysql_compat.c comments correctly describe their rules as bare top-level statements; align this one to match reality.

💡 Suggested change

Before:

 * Adds FIND and AGGPIPE -- mongo-flavored entry points for JSONB
 * queries.  Each takes a JSONB document as RHS but for this test
 * the rules are bare keywords (the value-shaping code lives in the
 * reduce callback, not in the grammar).

After:

 * Adds FIND and AGGPIPE -- mongo-flavored entry points for JSONB
 * queries.  For the test, each surfaces as a bare top-level
 * statement that NOTICEs which production fired.

---

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

Test 4 verifies nothing and can mask real regressions. The whole block is under local $TODO, and its own comment (Test 9, plus the confirmed FATAL-at-prewarm path in parser_extension.c: ereport(FATAL, ... "grammar extension compose failed at startup")) shows a duplicate-token compose failure aborts postmaster startup. In that case $node->start in the eval fails, $started is false, and the else branch unconditionally pass()es -- the conflict-detection like() on line 182 never runs. Even if it did run and fail, $TODO swallows the failure. Net result: this sub-test can never catch a foxtrot regression. Either assert the observed prewarm-FATAL behavior deterministically (mirror Test 9: start(fail_ok => 1) then grep the log), or drop the block until register()-time validation actually exists.

---

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

$log_after_start is assigned but never read anywhere in the block; the $log_after_start = $started ? ... : '' computation is dead. With use warnings FATAL => 'all' this is a latent hygiene defect and just wasted work. Remove it.

---

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

Reading the logfile by slurping it directly right after safe_psql (before $node->stop) is racy for the debug1/NOTICE register+compose lines: there is no guarantee they have been flushed to disk when this open() runs, so these like()/unlike() assertions can fail intermittently on the buildfarm. The tree-wide idiom is $node->wait_for_log(qr/.../, $offset), which polls until the line appears. Use that (capture $node->logfile size before the query as the offset) instead of the hand-rolled log_text() slurp for Tests 1,2,3,5,6 and Test 7's first boot.

---

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

Inaccurate count: the module ships seven extensions (alpha, beta, echo, foxtrot, golf, hotel, india), all referenced by this test, but the header says "six compose_ext_* extensions". Fix the count so the comment matches reality.

💡 Suggested change

Before:

# Spins up a postmaster with various combinations of the six
# compose_ext_* extensions in shared_preload_libraries, runs a

After:

# Spins up a postmaster with various combinations of the seven
# compose_ext_* extensions in shared_preload_libraries, runs a

---

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

This comment misdescribes what the india extension does. compose_ext_india.c does NOT register "two identical stmt ::= K_GRAMMAR_INDIA productions" nor a reduce/reduce conflict; it declares a dangling-else grammar (distinct india_if productions) producing a SHIFT/REDUCE conflict. Align the comment with the actual extension to avoid misleading future readers.

---

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

Header claims 1-9 test items but items 7 and 8 describe cache-key / .so byte-equality behavior (SHA256 cache hit, "the cache hit avoids re-running lime+cc", byte equality of the .so) that does not exist in Track B -- the implementation is in-process compose with no .so cache (confirmed by the unlike(..., qr{/pg_parser_cache/...\.so}) assertions and parser_pushparse.c). The actual Test 7 asserts in-process compose determinism and Test 9 is the conflict gate. This aspirational header no longer matches the tests below it; update items 7-9 to describe what the file actually verifies.

---

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

Log-flush race: log_text() reads the server logfile immediately after safe_psql() returns. The 'composing grammar in-process' and 'keyword map ... published' lines are emitted at DEBUG1 by the backend that ran the query; that output may not be flushed to the collected server log yet when this single read happens, producing intermittent failures. Use $node->wait_for_log(qr/.../) (available in PostgreSQL::Test::Cluster) to poll for the expected line instead of a one-shot slurp.

---

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

These stdout assertions are too weak to catch a regression. qr/\b1\b/ .. qr/\b4\b/ are unanchored and order-insensitive against the concatenated output of an 8-statement script; any digit anywhere (e.g. inside a NOTICE, a 'nrhs=1' reduce message routed to stdout, or a row count) satisfies them. 'SELECT 2 ran' could pass even if SELECT 2 did not run. Run each SELECT with a distinct sentinel value via separate psql calls (or capture per-statement output) and match anchored so a reverted feature would actually fail here.

---

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

Comment drift: 'already cached from test 1' is false. Each subtest calls start_with(), which creates a brand-new independent cluster; there is no cache shared between test 1 and test 4's node. The comment describes a cross-node caching relationship that does not exist and risks masking the fact that this subtest re-composes from scratch. Remove or correct the claim.

---

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

This test does not verify what its heading and the file header (points 6/7) claim. It boots a single fresh node exactly once and asserts the compose ran once; it never boots the same node twice nor compares the composed result across two boots, so 'compose determinism across postmaster boots' is not actually exercised. Either restart the node ($node->restart) and re-assert the same compose/reduce behavior across the two boots, or rewrite the comment to match what is tested (a single fresh-boot compose).

---

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

Exposing a raw backend heap pointer (snap=%p) to any SQL caller leaks ASLR/heap-layout information and, more importantly, makes the output non-deterministic and non-portable (pointer width/format varies across platforms). The TAP test (t/001_smoke.pl) matches this exactly via qr/^ok: snapshot built \(snap=0x[0-9a-f]+\)/, so the pointer format is load-bearing yet unstable. Report only a fixed status token; drop the pointer. (high confidence)

💡 Suggested change

Before:

		appendStringInfo(&out, "ok: snapshot built (snap=%p)",
						 (void *) snap);

After:

		appendStringInfoString(&out, "ok: snapshot built");

---

📄 src/test/modules/lime_in_process_smoke/lime_in_process_smoke.c (L85-L86)

err is allocated inside the external lime_compile_grammar_in_process; releasing it with libc free() is only correct if the library uses the process libc allocator. If the library palloc's it (or uses its own allocator), this is a mismatched free that risks heap corruption or a crash. snap is disposed via lime_snapshot_release() while err uses free() -- the ownership/allocator contract for both must be confirmed against the library API before relying on this. (moderate confidence)

---

📄 src/test/modules/lime_in_process_smoke/lime_in_process_smoke.c (L69-L70)

len is derived from strlen() of the NUL-terminated copy. Since the API takes an explicit length parameter alongside the pointer, it clearly wants the exact byte count; if the input text contains an embedded NUL, strlen() truncates len so the length passed no longer matches the grammar content -- a silent correctness bug. Use the real datum length instead of strlen. (moderate confidence)

💡 Suggested change

Before:

	grammar = text_to_cstring(grammar_text);
	len = strlen(grammar);

After:

	grammar = text_to_cstring(grammar_text);
	len = (size_t) VARSIZE_ANY_EXHDR(grammar_text);

---

📄 src/test/modules/lime_in_process_smoke/lime_in_process_smoke.c (L3-L5)

This header comment references a private, non-repository design note (.agent/notes/track-b-phase2-design.md, which does not exist in the tree) and uses aspirational future-tense phrasing ("the foundation for Phase 4 Track B Phase 2", "before we attempt the much more invasive parser.c surgery"). Comments must describe what the code does now and must not cite content absent from the tree. It also claims a trivial grammar "round-trips through the in-process compile path", which contradicts t/001_smoke.pl's own note that without runtime files the API "returns a structured error rather than a snapshot". Trim to describe only current behavior. (high confidence)

---

📄 src/test/modules/lime_in_process_smoke/lime_in_process_smoke.c (L58-L61)

This comment reasserts the same in-tree API claims and drifts into implementation narrative ("the strong definition resolves here rather than the weak no-op stub") that describes build/link internals rather than this function's behavior. Keep the comment focused on why this call is made here; move the linkage rationale to the build file (meson.build already documents the --whole-archive workaround). (low confidence)

---

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

Contradiction between this comment and the happy-path assertion below. This block states the in-process API "returns a structured error rather than a snapshot" without the unshipped runtime files, but line 68 asserts the result matches qr/^ok: snapshot built .../. Both cannot be true: if this comment is accurate, the first like() will always FAIL and break the buildfarm/cfbot; if the assertion passes, this comment is stale. Additionally, this comment references lime_compile_grammar_text, while the header (line 4) and the C implementation call lime_compile_grammar_in_process -- a different function. Resolve the contradiction and align the referenced function name before this can be committed.

---

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

Non-portable assertion. The C function formats the pointer with %p (appendStringInfo(&out, "ok: snapshot built (snap=%p)", ...)), but %p output is implementation-defined: not all platforms/libc prefix with 0x or emit lowercase hex, and PostgreSQL must pass on Windows/MSVC, the BSDs, Solaris, etc. This regex will fail on animals whose %p does not produce 0x[0-9a-f]+. Either make the C code emit a fixed format (e.g., 0x%" PRIxPTR) or relax the regex to not depend on the 0x/lowercase-hex form (e.g., just assert non-NULL / non-empty pointer text).

---

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

This header contradicts the comment block at lines 28-33. Here it claims the entry point "produces a non-NULL ParserSnapshot for a trivial grammar", but the later block says it "returns a structured error rather than a snapshot." It also states "proves liblime_parser.a is linked", whereas the C module's success path relies on liblime_compiler.a's lime_compile_grammar_in_process. Make the file-level description consistent with the actual behavior and the C implementation.

---

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

Remove this aspirational / internal design-note commentary before committing. PostgreSQL comment discipline forbids future-tense/uncertain prose ("will be updated", options (a)/(b)/(c), "the 'in-process' label is misleading") and references to internal project notes (.agent/notes/track-b-phase2-design.md) in committed source. Comments must describe what the code does now. This block reads as WIP scaffolding and would be rejected on -hackers.

---

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

Copyright (c) 2026 is a single future year inconsistent with the tree convention. New files typically use the current year (e.g., a YYYY-2026 range where applicable) matching sibling files such as src/test/modules/meson.build which uses 2022-2026. Confirm/align to the project header convention.

---

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

Portability hard gate: clock_gettime(CLOCK_MONOTONIC, ...) is a raw POSIX API that does not build on Windows/MSVC (which has no clock_gettime and uses QueryPerformanceCounter instead). PostgreSQL provides the instr_time abstraction in src/include/portability/instr_time.h precisely to hide this platform split. Use INSTR_TIME_SET_CURRENT() / INSTR_TIME_SUBTRACT() / INSTR_TIME_GET_NANOSEC() instead of the raw call and manual ns math. (high confidence)

💡 Suggested change

Before:

	clock_gettime(CLOCK_MONOTONIC, &t0);

After:

	INSTR_TIME_SET_CURRENT(t0);

---

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

The return value of clock_gettime is ignored and errno is never checked. If the call fails (e.g. CLOCK_MONOTONIC unavailable), t0/t1 retain uninitialized stack garbage and ns_total becomes meaningless or negative -- and this value is the entire output of the function. Using the instr_time abstraction avoids this since it handles the clock source internally. (high confidence)

---

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

There is no CHECK_FOR_INTERRUPTS() in the timing loop. With a large iterations argument this becomes an uninterruptible backend operation: the user cannot cancel the query and the backend cannot respond to signals for the whole run. Add CHECK_FOR_INTERRUPTS() inside the loop (a single check per iteration adds negligible overhead relative to a full raw_parser() call, and can be placed outside the timed region if desired). (medium confidence)

💡 Suggested change

Before:

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

After:

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

---

📄 src/test/modules/parser_microbench/parser_microbench.c (L58-L61)

This new module ships no test coverage and no SQL registration. The meson.build builds the shared module, but there is no .control/.sql extension file, no CREATE FUNCTION parser_microbench(...) declaration, and no t/*.pl or regression .sql/expected under src/test/modules/parser_microbench/. Per project rules a new user-visible SQL function is not commit-ready without tests exercising the happy path and the error/edge paths -- notably the iterations <= 0 ERROR path added below and an empty/invalid query. As written the function is not installable or tested. (medium confidence)

---

📄 src/test/modules/parser_microbench/parser_microbench.c (L4-L6)

The header comment couples this module's documented purpose to dummy_grammar_ext ("with the dummy_grammar_ext loaded", "Loaded via shared_preload_libraries"), but the code has no dependency on any grammar extension and does not verify or require any preload state -- it works identically standalone (as the comment itself later admits). This aspirational/coupled framing is misleading; describe only what the function does now (time N raw_parser() calls), independent of any extension. (low confidence)

---

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

This execl -> execlp change appears unnecessary and unrelated to any stated purpose. shellprog is SHELLPROG, which is always an absolute path (/bin/sh in meson.build, $(SHELL) which defaults to /bin/sh in the Makefiles). For a path containing a slash, execlp performs no PATH search and is therefore behaviorally identical to execl here — so this is a no-op in normal builds and adds unexplained churn to a stable file (minimal-diff violation).

Worse, it introduces a latent footgun: should $(SHELL) ever be overridden to a bare program name (no slash), execlp would resolve the shell via the PATH environment variable, which is attacker-influenced — an undesirable behavior change for a test harness that runs in CI. Confidence: high that this is unnecessary; moderate on the security angle (depends on override). Recommend dropping this hunk and keeping execl.

💡 Suggested change

Before:

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

After:

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

---

📄 src/tools/lime_format (L56-L57)

Data-safety footgun: original is read, then lime -F is run in place, then the .formatted output is unconditionally moved over the source. If lime -F emits a truncated/empty/corrupt .formatted while still returning 0, the original source is silently destroyed with no backup and no validation. Since meson compile lime-format runs this across the whole tree, a formatter regression could corrupt many source files at once. The companion lime_format_check avoids this by copying each file into a tempdir and never touching the source. Consider formatting into a temp file and only replacing the original after a sanity check (e.g. non-empty output, or successful re-parse), and/or writing atomically.

---

📄 src/tools/lime_format (L37-L37)

Prefix matching over-excludes files. part.startswith(p) with SKIP_PATTERNS=('build','install','.git','tmp_install') skips any path component merely beginning with those strings (e.g. a directory builtins, installer, or a file build_something.lime). Legitimate .lime files under such paths would be silently never formatted, causing formatting drift that only surfaces later in lime_format_check. Use exact component matching, e.g. part in SKIP_PATTERNS. Note the same bug exists in the companion lime_format_check, so they will at least skip consistently, but both are broader than intended.

💡 Suggested change

Before:

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

After:

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

---

📄 src/tools/lime_format (L39-L39)

Encoding/newline handling is left to the platform default. read_text() (and the new == original comparison) uses the locale default encoding and universal-newline translation, which differs on Windows (a hard portability target). This can yield false formatted/unchanged results or a UnicodeDecodeError on non-UTF-8 bytes in grammar files. Pass encoding='utf-8' explicitly (ideally compare bytes) so idempotency detection is deterministic across platforms.

---

📄 src/tools/lime_format (L44-L49)

Orphaned .formatted artifacts are not cleaned up on failure. When lime -F returns non-zero, or produces a .formatted that is then abandoned by a later continue/failure, or when shutil.move throws mid-run, a stale <name>.lime.formatted file can be left in the source tree. It won't match *.lime, so nothing reaps it and it can accumulate silently (and risk being committed). Clean up leftover .formatted files on the failure paths, and confirm they are covered by a .gitignore.

---

📄 src/tools/lime_format_check (L11-L14)

The header claims "flake.lock pins v0.6.0", but the actual pin in flake.nix is ?ref=refs/tags/v1.8.2. This entire block of version narrative (v0.6.0/v0.6.1, the two-pass workaround history, the flake.lock pin) is unverifiable-in-code and, where checkable, factually wrong. Worse, the companion meson.build comment for this same target states the opposite -- that the formatter "is not idempotent on its first pass ... stabilizes after pass 2" -- while this script runs only a single lime -F pass. Either the single-pass assumption here is unsafe (the check could pass/fail on formatter non-determinism) or the meson.build comment is stale. Per project comment discipline, comments must describe what the code does now; drop the stale version-pinned narrative and reconcile the single-pass vs two-pass claim with meson.build.

💡 Suggested change

Before:

+# Lime v0.6.0+ formatter is single-pass idempotent (regression-
+# tested upstream in v0.6.1).  Pre-v0.6.0 needed two passes for
+# %left/%right/%nonassoc symbol order to stabilize; we drop that
+# workaround now since flake.lock pins v0.6.0.

After:

+# Runs a single `lime -F` pass per file and compares against the source.

---

📄 src/tools/lime_format_check (L47-L47)

SKIP_PATTERNS matching uses part.startswith(p), so any path component that merely begins with 'build', 'install', '.git', or 'tmp_install' is excluded (e.g. a directory named 'building', 'installer', or '.gitignore'-adjacent dirs). This can silently omit legitimate .lime files from the check, giving a false pass and defeating the tool's purpose. Use an exact component match (or gate on top-level path parts only) so real files are not silently skipped.

💡 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() uses the platform default encoding and text-mode universal-newline translation. On Windows (a hard portability gate for PostgreSQL) this can produce spurious mismatches or spurious matches versus the on-disk bytes the formatter actually wrote, making the check unreliable across platforms. Read both sides with an explicit encoding (and ideally compare in a newline-consistent way) so the byte-equality assertion is deterministic everywhere.

💡 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/lime_convert_gram.py (L1300-L1304)

Label generation/detection mismatch for RHS positions > 25. _label_for_index returns single letters B..Z for indices 1-25, but falls back to a multi-character P{idx} form (e.g. P26) beyond that. Two downstream problems result:

1. _dollar/_typed substitute $26 into action bodies as P26.
2. _labels_referenced only recognizes single uppercase letters via ([A-Z])(?![A-Za-z0-9_]) and @([A-Z]); it cannot match P26. So label in refs in _emit_alternative is always false for these positions, the (P26) decoration is omitted from the RHS symbol, yet the action body still references P26 -> the generated Lime grammar has an undeclared label (compile error).

Additionally, P{idx} is not a valid single-symbol Lime label form. PostgreSQL's gram.y contains productions with well over 25 RHS symbols, so this breaks on real input. Extend the label scheme to a form both the generator and _labels_referenced agree on (e.g. two-letter labels AA, AB, ...) and update the detection regex accordingly.

---

📄 src/tools/lime_convert_gram.py (L2134-L2137)

The label-reference detector only matches single uppercase letters ([A-Z] not followed by an identifier char) and @[A-Z]. It cannot detect the multi-character labels (P26, etc.) that _label_for_index emits for RHS positions beyond 25. Combined with the emission logic in _emit_alternative that gates RHS decoration on label in refs, this means long-RHS positions referenced in an action are treated as unreferenced, their (LABEL) decoration is dropped, and the resulting Lime grammar references an undeclared label. Update this to match whatever multi-char label scheme _label_for_index produces.

---

📄 src/tools/lime_convert_gram.py (L1301-L1304)

_label_for_index falls back to P{idx} for RHS positions beyond 25 (e.g. P26). This is not a valid Lime symbol label and, worse, is undetectable by _labels_referenced (which only matches single uppercase letters). PostgreSQL's gram.y has productions with well over 25 RHS elements, so this produces broken Lime output for real input. Use a label scheme that stays within Lime's identifier rules and is recognized by the reference detector (e.g. two-letter labels AA, AB, ... that the detector also matches).

---

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

Fragile declarator matching: after the exact trailing-token check fails, the a.endswith(ident) fallback does a raw string-suffix match with no word boundary. ident='result' would match a declarator ending in presult/yyresult, and short idents like p collide with pp. A wrong match emits <full-decl> = extra->wrongident, which either fails to compile or silently binds the wrong parse-param. This same fragile logic is duplicated in _inject_parse_param_locals. Consider tokenizing the declarator and comparing the trailing identifier exactly (stripping */[]) instead of endswith.

---

📄 src/tools/lime_lint (L65-L67)

False-negative in the failure gate: '0 error(s)' in out matches via substring, so lime output like '10 error(s)', '20 error(s)', etc. contains the substring '0 error(s)' and is wrongly treated as clean. Any file with a multiple-of-ten error count silently passes, defeating the lint. Match the count precisely, e.g. anchor on '0 error(s)' only when preceded by a non-digit / start (regex (?<![0-9])0 error\(s\)), or better, rely on lime's non-zero exit code alone if it sets one on errors.

💡 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
    error_count_zero = (re.search(r'(?<![0-9])0 error\(s\)', out)
                        or 'OK: no diagnostics' in out
                        or 'No errors or warnings' in out)

---

📄 src/tools/lime_lint (L59-L59)

Non-ASCII character: the U+2713 check mark ('✓') appears in both this comment and the detection string below. PostgreSQL contribution standards require ASCII-only source, and matching on a non-ASCII literal is fragile across encodings. Since the sibling scripts note the toolchain pins a recent Lime version, consider dropping the pre-v0.5.0 shape entirely; if it must be kept, match an ASCII substring like 'No errors or warnings'.

---

📄 src/tools/lime_lint (L63-L64)

Comment references a --lint-strict option that is not implemented by this script (only --lime, --srcdir, and --quiet are defined). This is comment drift describing behavior that does not exist and will mislead maintainers. Remove the reference or implement the flag.

---

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

restructure_midrules handles only the FIRST mid-rule action per alternative (it breaks after restructuring one), as its own docstring admits ('multi-midact-with-refs would need this generalized'). If a target grammar (notably ecpg's, the stated target) has an alternative with a second mid-rule action that references prefix positions, the second helper's $N/@n prefix references are never rewritten to scratchpad globals and the position shift is applied only once. This is a silent-wrong-output limitation, not an error, so it won't surface until the generated C misbehaves. Either generalize the pass or hard-error when a second prefix-referencing mid-rule action is detected so the failure is loud.

---

📄 src/tools/lime_convert_gram.py (L2476-L2477)

Files are opened with the platform default text encoding. On Windows (a supported platform) the default is cp1252, which differs from the UTF-8/ASCII expected for grammar sources; a non-ASCII byte in the input would raise UnicodeDecodeError and emission could be corrupted. Pass encoding="utf-8" explicitly to both opens for deterministic, portable behavior.

💡 Suggested change

Before:

    with open(args.input) as f:
        text = f.read()

After:

    with open(args.input, encoding="utf-8") as f:
        text = f.read()

---

📄 src/tools/lime_convert_gram.py (L2526-L2527)

Output file opened with the platform default encoding; on Windows this can corrupt emitted content and diverges from the ASCII/UTF-8 expectation. Pass encoding="utf-8" explicitly.

💡 Suggested change

Before:

    with open(args.output, "w") as f:
        f.write(output)

After:

    with open(args.output, "w", encoding="utf-8") as f:
        f.write(output)

---

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

This shebang change makes pgindent the only Perl script in the entire tree using #!/usr/bin/env perl; every other Perl file (e.g. src/tools/copyright.pl, src/tools/mark_pgdllimport.pl, src/backend/catalog/genbki.pl, and ~30 others) uses #!/usr/bin/perl. This is an inconsistent, unrelated change that does not fit the stated purpose of the patch and violates the minimal-diff / project-convention discipline. Unless there is a documented reason to switch (which would need to be applied tree-wide, not to a single script), revert to #!/usr/bin/perl.

💡 Suggested change

Before:

#!/usr/bin/env perl

After:

#!/usr/bin/perl

---

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

Internal annotation contradicts both the function signature (line 204: OrderedDict[str, list[tuple[list[str], str | None]]]) and the runtime value. The code appends (rhs, prec) tuples (rules.setdefault(lhs, []).append((rhs, prec))), and emit_bison unpacks rhs, prec = alt. This declares the value as a bare list[str] list. Runtime behavior is correct, but the annotation is wrong and will mislead readers and static checkers (mypy would flag the tuple append).

💡 Suggested change

Before:

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

After:

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

---

📄 src/tools/lime_to_bison_gram.py (L458-L459)

Both open() calls omit encoding=, so text is decoded/encoded using the platform locale codepage on Windows rather than UTF-8. Per the tree's Windows/BSD portability requirement, grammar tooling that reads/writes source files should pin encoding="utf-8" for deterministic cross-platform output. Applies to the output open(sys.argv[2], "w") below as well.

💡 Suggested change

Before:

    with open(sys.argv[1]) as f:
        text = f.read()

After:

    with open(sys.argv[1], encoding="utf-8") as f:
        text = f.read()

---

📄 src/tools/pglime (L73-L73)

Fail-fast: the --aot/--snapshot consistency checks run only after Lime has already executed and the primary .c/.h have already been moved. An invocation with --aot but no --aot-output wastes the generator run, leaves stray artifacts in the private dir, and moves the primary outputs into place before aborting -- a partially-completed custom_target that can confuse ninja's incremental rebuild. Validate the argument pairing up front, right after parse_args(), before building the command line (consistent with the sibling pgflex wrapper's fail-fast style).

💡 Suggested change

Before:

+args = parser.parse_args()

After:

+args = parser.parse_args()
+
+if args.aot and not args.output_aot:
+    sys.exit('--aot requires --aot-output')
+if args.snapshot and not args.output_snapshot:
+    sys.exit('--snapshot requires --snapshot-output')

[github-actions]

environment.localBinInPath is a NixOS module option, not a valid flake output. It is placed inside the attrset returned by flake-utils.lib.eachDefaultSystem, so it becomes the flake output environment.<system> = true, which is not a recognized flake output schema and has no effect (environment.localBinInPath only does something inside a NixOS configuration.nix). This looks like speculative/dead scaffolding that never wires up to anything -- remove it.

[github-actions]

$CC is unquoted and passed straight to basename. If CC contains flags or a compiler-launcher (e.g. CC="ccache gcc", common in dev setups), word-splitting makes basename see two arguments and returns the wrong name, silently defeating the compiler-change detection below. Quote and take the last word, e.g. local current_compiler; current_compiler=$(basename "${CC%% *}").

Suggested change

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

[github-actions]

return 1 is executed at the top level of the alias expansion, not inside a function. When pg-setup is run interactively, bash emits "return: can only return' from a function or sourced script" and does not stop the alias, so meson setupstill runs with an empty include path. Convertpg-setup` to a shell function (like the other helpers in this file) so the early-exit actually works.

[github-actions]

modified_files is assigned without local, leaking a global into the caller's shell (every other helper here correctly declares local). Since this runs in the interactive shell it can clobber a user variable of the same name. Declare it local.

Suggested change

	modified_files=$(git diff --name-only "${since}" | grep -E "\.c$|\.h$")
	local modified_files
	modified_files=$(git diff --name-only "${since}" | grep -E "\.c$|\.h$")

[github-actions]

--port=40099 is hardcoded. If a developer already has anything bound to 40099 (a prior interrupted run of this same command, or another instance), pg_regress fails to start its temp instance. pg_regress can auto-select a free port; drop the fixed --port (or derive one) rather than hardcoding a single value.

[github-actions]

RAW_PARSER_USE_PUSHPARSE() calls getenv("PG_LIME_PUSHPARSE") on every raw_parser() invocation, i.e. on the hottest parse path for every SQL statement. getenv walks the environment on each call; this is needless per-statement work. More importantly, an environment variable that silently redirects all SQL parsing to an alternate parser is a footgun (POLA / unsafe default) and should not be a committed toggle. If a runtime toggle is genuinely needed, cache it once (e.g. at process start) rather than re-reading it per parse, and prefer a GUC over a raw env var. (high confidence)

[github-actions]

Comment references internal project phases/jargon ("Track B push-parse driver", "used to A/B the push parser") and Lime-internal implementation details. PostgreSQL source comments must describe present behavior for a general audience and avoid internal project-phase shorthand. Rewrite to explain what the code does and why, without the phase/track/A-B jargon. (moderate confidence)

[github-actions]

These prototypes for raw_parser_lime_pushparse() and raw_parser_lime_active() are declared locally in parser.c rather than in a shared header. This bypasses compiler cross-checking of the declaration against the definitions in parser_pushparse.c: if either signature drifts, the mismatch will not be caught. Move these declarations into a shared header (e.g. parser_extension.h) that both TUs include. (moderate confidence)

[github-actions]

This function-pointer indirection is dead code, and its comment is factually wrong. base_yyparse_fn is defined in parser.c initialized to base_yyparse and read once in raw_parser(); grepping the whole tree, it is never reassigned anywhere. The comment claims "parser_extension.c overwrites it after dlsym-ing the rebuilt parser .so", but that subprocess/dlopen path ("Track A") was removed entirely -- parser_extension.c's own header states "The historical Track A path (serialize -> fork lime + cc -> dlopen a cached .so) has been removed entirely", and there is no dlsym call anywhere in src/backend/parser/. Per YAGNI, drop the indirection and call base_yyparse() directly, or if the swap point is genuinely still needed, wire it and fix the comment to describe what the code does now (no "Phase 4", no dlsym). An aspirational/forward-looking comment for behavior that was removed will mislead the next reader.

[github-actions]

This comment misdescribes the sync target. gram.lime has no union body in its %include block; per-symbol value types are declared by scattered %type <symbol> {<C type>} directives (e.g. %type singlePartitionSpec {SinglePartitionSpec *}), and Lime builds its internal YYSTYPE from those. As written the comment points a maintainer at a nonexistent block. More importantly, this hand-maintained union must match the aggregate of every %type/%token_type Lime uses, with no compile-time guard (no StaticAssert, no generation step) against drift -- a member added or retyped in gram.lime but not mirrored here silently reinterprets semantic-value bits (e.g. reading a Node* slot as int), i.e. corruption/crashes at parse time. At minimum fix the comment to point at the %type declarations; ideally generate this union from the same source Lime consumes, or add a build-time consistency check.

[github-actions]

🔍 OCR found 262 issue(s).

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

---

📄 src/backend/replication/syncrep_scanner.c

Comment has drifted from the code: it says "pstrdup into yylval->str", but the code below uses a manual palloc+memcpy+NUL into tok.str (no pstrdup, no yylval). Also the reference to what "the original flex scanner used" is historical narration that describes removed code rather than what this code does now; PostgreSQL comments should explain the current behavior. Consider trimming to describe the current logic, and using pnstrdup(text, len) in place of the manual palloc/memcpy/terminate to match tree conventions.

---

📄 src/test/modules/grammar_ext_compose/compose_ext_india.c

This header describes the conflict as a SHIFT/REDUCE dangling-else resolved keep-shift, and the rules[] below implement exactly that. However, the TAP test that drives this extension (t/001_compose.pl, Test 9) documents the same india extension as a REDUCE/REDUCE conflict from "two identical stmt ::= K_GRAMMAR_INDIA productions" resolved keep-first. Those two descriptions of the same artifact contradict each other. Reconcile the two comments so the documented conflict type matches the grammar actually built here (shift/reduce dangling-else); otherwise a reader cannot tell which conflict the gate is being tested against.

---

📄 .github/workflows/windows-dependencies.yml (L86-L86)

Non-ASCII characters used in echoed strings: U+2713 (check mark) here and on the '.github/ changes' line, plus U+2192 (right arrow) in the 'substantive changes' line. PostgreSQL requires ASCII-only in source and diffs. Replace with ASCII equivalents (e.g. '[skip]' / '->').

💡 Suggested change

Before:

              echo "  ✓ Dev setup/version commit (skippable)"

After:

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

---

📄 contrib/pg_plan_advice/pgpa_parser_yytype.h (L23-L25)

These three forward typedefs duplicate the identical typedef struct ... ; declarations already present in pgpa_ast.h (lines 35-40, 47, 112-116). Both translation units that include this header — pgpa_parser_driver.c (includes pgpa_ast.h at line 32, then this header at line 33) and the grammar generated from pgpa_parser.lime (line 48 then line 49) — include pgpa_ast.h first, so the same typedef name is redefined twice in one TU. Repeating a typedef for the same type is only allowed in C11 (6.7p3); PostgreSQL's baseline is C99 (see src/include/c.h), where this is a constraint violation. Strict compilers warn and MSVC (a hard portability gate) can reject it.

The header comment's premise ("so the union compiles without pulling in pgpa_ast.h") does not hold, since both real consumers do pull in pgpa_ast.h before this header. Either drop these forward typedefs and include "pgpa_ast.h" here directly, or use plain struct forward tags (struct pgpa_advice_item;) and reference the members as struct pgpa_advice_item *item; to avoid the duplicate typedef. Confidence: high.

💡 Suggested change

Before:

typedef struct pgpa_advice_item pgpa_advice_item;
+typedef struct pgpa_advice_target pgpa_advice_target;
+typedef struct pgpa_index_target pgpa_index_target;

After:

struct pgpa_advice_item;
struct pgpa_advice_target;
struct pgpa_index_target;

---

📄 contrib/cube/cubeparse_driver.c (L261-L265)

extra.aborted is set by the grammar's YYABORT shim (cubeparse.lime: #define YYABORT do { extra->aborted = true; } while (0)) but is never inspected here. cube_yyparse unconditionally return 0 after the token loop, so a parser-side abort is indistinguishable from success via the return value. This diverges from the bison contract (YYABORT -> non-zero return). It happens to be harmless today only because the sole caller (cube_in) ignores the return value and the soft-error framework re-checks SOFT_ERROR_OCCURRED, but a return value that never reflects failure is a footgun for any future caller. Consider return extra.aborted ? 1 : 0;.

💡 Suggested change

Before:

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

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

After:

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

	cube_yyFree(s->parser, pfree);
	return extra.aborted ? 1 : 0;
}

---

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

The lexer's catch-all rule reports a specific reason via LEX_ERROR_AT("syntax error: unexpected character") (cubescan.lex), but this path discards it and hardcodes the generic "syntax error". The specific lexer message is lost. Confidence: high (verified against cubescan.lex).

---

📄 contrib/cube/cubeparse_driver.c (L48-L55)

struct GramParseExtra is hand-duplicated here and must stay byte-for-byte identical with the definition generated into cubeparse.lime (fields result, scanbuflen, escontext, yyscanner, aborted, in this exact order). They match today, but this is a fragile ODR coupling: if the converter ever reorders/adds a field (e.g. moves aborted), cube_yy() writes through a mismatched layout and silently corrupts memory, with no compile-time check. Prefer emitting this struct into a generated header both the driver and parser include, rather than maintaining two copies. Confidence: high.

---

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

This COMMENT text is stale and contradicts the shipped behavior. It claims feature reachability "is gated on Track B scanner-table updates which are not yet wired", but the runtime status message this function returns (quel.c:678-690) reports the features as fully live ("keyword override live", "RETRIEVE / REPLACE / APPEND / DELETE build real PG parse trees that flow through parse_analyze + planner + executor"), and t/001_quel.pl exercises all of these end-to-end and asserts identical results to SQL. Per PostgreSQL comment-accuracy discipline, aspirational/"not yet" wording for behavior that already ships must be dropped -- describe what the function does now, not a superseded roadmap state. (High confidence.)

💡 Suggested change

Before:

'(reachability is gated on Track B scanner-table updates which '
'are not yet wired).';

After:

'and which QUEL features are reachable in the current build.';

---

📄 contrib/pg_plan_advice/pgpa_parser_driver.c (L294-L301)

This if (ctx.had_error) branch has an empty body (comment only), which is dead code and does nothing at runtime. The lex-time error was already stashed inside pgpa_emit_cb (via s->lex_errmsg/s->lex_errtext), so this branch is never needed. Remove the empty if and, if the explanation is worth keeping, move the comment to the emit callback where the stashing actually happens. Empty conditional blocks like this get flagged in review and by static analyzers.

---

📄 contrib/pg_plan_advice/pgpa_parser_driver.c (L270-L271)

s->input and s->input_len are written here but never read anywhere in this translation unit (the FIFO of pre-scanned tokens and s->yytext are what the rest of the driver uses; the feed loop uses the local input_len). PgpaYyScanner is private to this file, so these are dead fields. Per YAGNI, drop input/input_len from the struct and this initialization; retaining the input pointer also falsely implies a lifetime dependency on the caller's string that this driver does not actually have.

---

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

struct GramParseExtra is hand-duplicated here to "match the converter's emitted struct body". This is a fragile, silently-breakable ABI coupling: if the Lime converter ever changes the emitted field set/order in pgpa_parser.lime, this copy diverges and pgpa_yy() will write through a mismatched layout (memory corruption), with no compile-time check. Note also the field type drift already present -- the .lime declares yyscan_t yyscanner while this copy uses void *yyscanner. Prefer emitting this struct into a shared generated header (e.g. pgpa_parser_yytype.h) and including it, rather than re-declaring it in every driver.

---

📄 contrib/quel/quel_grammar.h (L149-L152)

Comment contradicts the implementation. quel_resolve_tuple_var() in quel_grammar.c calls ereport(ERROR) on an unbound tuple variable, which longjmps and never returns. It does not "Return NULL". Fix the comment to describe the actual behavior (raises an error via ereport(ERROR)). (high confidence)

💡 Suggested change

Before:

+ * Build a RangeVar from a tuple variable name (e.g. "e") by
+ * resolving against the rangetab.  Returns NULL with an ereport
+ * if the tuple variable is unbound.
+ */

After:

+ * Build a RangeVar from a tuple variable name (e.g. "e") by
+ * resolving against the rangetab.  Raises ereport(ERROR) if the
+ * tuple variable is unbound (does not return).
+ */

---

📄 contrib/quel/quel_grammar.h (L117-L124)

Dead scaffolding (YAGNI). These 11 coarse builders (quel_build_retrieve/_replace/_append/_delete/_create/_destroy/_copy/_define_view/_remove_view/_index/_help) are declared here and defined as empty no-op stubs in quel_grammar.c, but nothing in quel.c's dispatch table ever calls them; the wired-up dispatch uses only the fine-grained builders (quel_build_retrieve_simple/_where/... etc.). Unused prototypes plus stub definitions that build empty nodes are speculative scaffolding that pgsql-hackers will reject. Remove them (and their stub definitions) until they are actually wired to a rule. (high confidence)

---

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

The lineno field is dead: quel_rangetab_set() in quel_rangetab.c always stores 0, and no code path ever sets a meaningful line number. Either populate it from the RANGE statement location or drop the field (and the parallel field in QuelRangeSlot). (moderate confidence)

---

📄 contrib/quel/quel.c (L468-L469)

These rules are registered and accepted by the grammar, but their labels ("retrieve (bare)", "retrieve into IDENT", "replace IDENT", "append to IDENT") have no matching strcmp branch in quel_reduce(). They therefore fall through to the default *(void **) lhs_out = NULL;, so a syntactically valid statement like a bare retrieve reduces to a NULL Node. That NULL becomes the statement in the List returned by raw_parser, which is a NULL-deref/crash hazard downstream in parse_analyze/transformStmt. Either add builders/handlers for these labels or remove the rules until they are supported.

Confidence: high.

---

📄 contrib/quel/quel.c (L186-L191)

The reduce callback is documented twice by two near-identical multi-paragraph comment blocks (the first block is fully superseded by the second). Drop the redundant first block to avoid stale/duplicated documentation churn.

Confidence: high.

---

📄 contrib/quel/quel_grammar.c (L404-L406)

Assert() must be used only for can't-happen invariants, never for user-reachable input. nrhs here comes from the parser dispatching a user-typed QUEL statement, and every subsequent access reads rhs_values[...]/rhs_locs[...] at fixed indices derived from this count. In a production build (Assert compiled out), any drift between the rule shape registered in quel.c and these hardcoded indices turns into out-of-bounds reads and dereferences of arbitrary pointers, crashing the backend from ordinary SQL input. Replace the Assert with a real runtime check that raises a clean ereport(ERROR) (or ensure the dispatch layer validates arity) before indexing. This pattern repeats across every Phase B builder (attr, retrieve, replace, append, delete).

---

📄 contrib/quel/quel_grammar.c (L222-L225)

These builders (quel_build_retrieve, quel_build_replace, quel_build_append, quel_build_delete) are never referenced anywhere: quel.c dispatches to the _simple/_where/_full variants instead, and nothing calls these bare-name functions. They are dead scaffolding that additionally return malformed parse nodes (UpdateStmt/InsertStmt/DeleteStmt with NULL relation and no targetList/selectStmt) which would crash parse analysis if ever reached. Remove them (and their declarations in quel_grammar.h) per YAGNI.

---

📄 contrib/quel/quel_grammar.c (L296-L299)

quel_build_create, quel_build_destroy, quel_build_copy, quel_build_define_view, quel_build_remove_view, quel_build_index, and quel_build_help are never registered or called from quel.c (no CREATE/DESTROY/COPY/DEFINE VIEW/REMOVE/INDEX rules exist). They build empty, unusable nodes. This is dead scaffolding for a path that isn't wired up; remove it (and its header declarations) to keep the change minimal.

---

📄 contrib/quel/quel_grammar.c (L207-L210)

This errmsg violates PG message conventions and leaks internals: user-facing text should not carry a "QUEL RANGE:" uppercase prefix with a colon, and printing raw pointer values (tv=%p rel=%p) exposes backend addresses to the client. Report the missing operand with a plain lowercase message and no pointers.

---

📄 contrib/quel/quel_grammar.c (L67-L72)

errmsg convention: do not embed the "QUEL RANGE:" prefix/colon, and do not hand-format the source location as "at character %d" (location + 1). Use parser_errposition(location) / errposition to attach the cursor position so the message reads like the rest of the parser's diagnostics. Same applies to the other "QUEL ...:"-prefixed messages in this file.

---

📄 contrib/quel/quel_grammar.c (L142-L144)

quel_implied_from_clause is never called: all RETRIEVE builders use quel_synthesize_from, which is a near-verbatim duplicate of this function. This is dead, duplicated code. Remove quel_implied_from_clause (and its declaration in quel_grammar.h), or unify the two into one helper.

---

📄 contrib/quel/quel_grammar.c (L79-L82)

quel_make_column_ref and quel_resolve_tuple_var (below) are never called from any builder in this file or from quel.c; the only mentions of quel_resolve_tuple_var are inside comments. These are dead exported helpers -- remove them and their header declarations.

---

📄 contrib/quel/quel_grammar.c (L172-L172)

Aspirational/WIP documentation for behavior that has not shipped. Comments must describe what the code does now; phrases like "These are sketches", "For now they are stubs", "Phase A/Phase B", and the reference to an internal planning doc (.agent/notes/quel-full-implementation-plan.md) are exactly the future-tense/stale-TODO comments PG review rejects. Either implement the code and describe it in present tense, or drop the unimplemented stubs and the aspirational prose entirely.

---

📄 contrib/quel/quel_grammar.c (L19-L25)

The file-header comment claims a full QUEL -> PostgreSQL mapping (CREATE/DESTROY/COPY/DEFINE V./REMOVE V./INDEX/HELP), but none of those forms are registered in quel.c and their builders are dead code. The header overstates what the module actually does; trim it to the forms that are actually wired (RANGE, RETRIEVE, REPLACE, APPEND, DELETE).

---

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

string_hash(key, keysize) hashes only Min(strlen(key), keysize - 1) bytes. Passing strlen(name) as keysize therefore drops the final character of every name from the hash, so e.g. "e1"/"e2" and any names differing only in their last char hash to the same bucket. Lookups still work (set/lookup are consistent and strcmp disambiguates), but hash distribution is badly degraded. Pass strlen(name) + 1 to hash the whole NUL-terminated string, matching how string_hash is meant to be used.

💡 Suggested change

Before:

	uint32		h = string_hash(name, strlen(name));

After:

	uint32		h = string_hash(name, strlen(name) + 1);

---

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

quel_rangetab_reset() is exported but never called anywhere in the tree (verified: no callers). The file-header comment claims the table is "reset ... only on backend start", and quel_grammar.h says it "is reset at backend start", but no backend-start hook or xact callback invokes it. These comments are aspirational and describe behavior that isn't wired up. Either wire up the reset (e.g. from _PG_init / a backend-start path) or drop the dead function and correct the comments to describe what the code actually does (lazy init on first set/lookup, persists for backend lifetime).

---

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

quel_rangetab_iterate(), quel_rangetab_count(), and the lineno field are dead code: no caller exists in the tree (verified), and lineno is only ever written as 0 in quel_rangetab_set. This is speculative scaffolding (YAGNI). Remove the unused iterate/count API and the always-zero lineno field, or populate lineno from a real parse location and add a consumer that needs it.

---

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

lineno is hard-coded to 0 here and never assigned a meaningful value, yet it is surfaced through quel_rangetab_iterate into QuelRangeEntry. Dead flexibility with no data behind it. Drop the field or wire it to the actual RANGE parse location.

---

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

The comment claims names are "lowercased", but quel_rangetab_set/lookup hash and strcmp the raw incoming string, and no caller (quel_grammar.c / quel.c) lowercases via downcase_identifier/pg_tolower (verified). As written, E and e bind to distinct slots, contradicting the documented case-insensitive behavior. Either lowercase in set/lookup or fix the comment to state names are matched case-sensitively as received.

---

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

This is a hand-rolled open-addressed hash table (probe + rehash + reset) duplicating existing in-tree infrastructure. Prefer dynahash (utils/hash, hash_create with HASH_STRINGS in TopMemoryContext) or simplehash.h, which are well-tested and eliminate the entire bespoke probe/rehash/reset surface (and the string_hash misuse above). DRY: reuse rather than reinvent.

---

📄 contrib/quel/t/001_quel.pl (L248-L251)

This join comparison is non-deterministic. Neither the QUEL query nor the SQL query has an ORDER BY, yet the two multi-row result sets are compared verbatim with is(). Row order from a two-table join is not guaranteed and can differ between the QUEL and SQL paths depending on scan/join method chosen by the planner, so this can flake on the buildfarm. Sort both sides before comparing (as the earlier retrieve tests do), or add a matching ORDER BY to both queries.

Confidence: high.

💡 Suggested change

Before:

		  retrieve (e.name) where e.dept = d.name;}),
	$node->safe_psql('postgres',
		q{SELECT e.name FROM qb_emp e, qb_dept d WHERE e.dept = d.name;}),
	'QUEL multi-tuple-variable join matches SQL FROM-list join');

After:

		  retrieve (e.name) where e.dept = d.name;});
my @quel_join = sort split /\n/, $node->safe_psql('postgres',
	q{range of e is qb_emp;
	  range of d is qb_dept;
	  retrieve (e.name) where e.dept = d.name;});
my @sql_join = sort split /\n/, $node->safe_psql('postgres',
	q{SELECT e.name FROM qb_emp e, qb_dept d WHERE e.dept = d.name;});
is_deeply(\@quel_join, \@sql_join,
	'QUEL multi-tuple-variable join matches SQL FROM-list join');

---

📄 contrib/quel/t/001_quel.pl (L260-L261)

Comparing EXPLAIN plan text byte-for-byte between two different statements is fragile. Even with COSTS OFF, plan output includes alias/relation names and node details that need not be identical between the QUEL-composed statement and the hand-written SQL; a future planner change or an alias-naming difference in the QUEL rewrite would break this without indicating a real regression. Consider asserting on the plan shape (e.g. like($quel_plan, qr/Seq Scan on qb_emp/)) rather than requiring the two plans to be textually identical.

Confidence: moderate.

---

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

These two unlike() checks are weak: they only pass because the strings are never written to the log during this run, not because the in-process compose is verified. running lime to rebuild parser does not appear to be emitted anywhere in the tree, so this assertion is vacuously true and would silently keep passing even if in-process compose broke. Prefer a positive assertion that in-process composition actually happened (e.g. matching a specific log line the composer emits), rather than relying on the absence of strings from an unrelated code path.

Confidence: moderate.

---

📄 contrib/upsert/upsert--1.0.sql (L9-L10)

The comment is inaccurate. Per upsert.c, the UPSERT keyword and grammar production are registered process-wide in _PG_init() at postmaster startup (guarded by process_shared_preload_libraries_in_progress), and the extension defines no SQL-level objects. Consequently the feature is active in every database as soon as the library is preloaded, and CREATE EXTENSION upsert does not enable/disable anything on a per-database basis. Claiming it "can be enabled per-database in the standard way" is misleading (POLA violation) and will confuse users who expect DROP EXTENSION/not running CREATE EXTENSION to disable UPSERT. Reword to describe what actually happens (e.g., the empty extension exists only for packaging/registration; the grammar is enabled globally via shared_preload_libraries).

💡 Suggested change

Before:

+-- objects; CREATE EXTENSION exists only so the feature can be enabled
+-- per-database in the standard way.

After:

+-- objects; CREATE EXTENSION exists only for packaging.  The UPSERT
+-- grammar is registered process-wide at postmaster startup and is
+-- therefore active in every database once the library is preloaded.

---

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

seg_yyparse always returns 0 on this path, ignoring extra.aborted. The grammar's YYABORT/YYERROR shims (segparse.lime: swapped boundaries, seg_atof failure) set extra->aborted = true. In soft-error mode (fcinfo->context != NULL) errsave does not longjmp, so a genuinely failed/aborted parse still returns 0. seg.c:115 then treats 0 as success and returns a partial/garbage SEG. The sibling drivers gate their return on the failure flag (e.g. pgpa_yyparse returns (*parse_error_msg_p != NULL) ? 1 : 0). Return nonzero when the parse aborted.

💡 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 ? 1 : 0;
}

---

📄 contrib/seg/segparse_driver.c (L138-L143)

seg_emit_cb keeps feeding tokens to seg_yy even after the parser has aborted. Once a semantic action runs YYABORT/YYERROR (segparse.lime lines 160/204/216/229) extra->aborted becomes true, but this callback ignores it and continues driving the parser with the remaining tokens. The sibling cube_emit_cb guards this with if (ctx->extra->aborted) return; at the top. Add the same early-out so no tokens are pushed into an aborted parser.

💡 Suggested change

Before:

	struct EmitContext *ctx = user;
	SegYyScanner *s = ctx->s;
	YYSTYPE		yylval;
	char	   *literal;

	memset(&yylval, 0, sizeof(yylval));

After:

	struct EmitContext *ctx = user;
	SegYyScanner *s = ctx->s;
	YYSTYPE		yylval;
	char	   *literal;

	if (ctx->extra->aborted)
		return;

	memset(&yylval, 0, sizeof(yylval));

---

📄 flake.nix (L73-L73)

environment.localBinInPath is a NixOS module option (config.environment.localBinInPath), not part of the flake output schema. Placed here inside eachDefaultSystem's per-system output attrset, it is not consumed by anything and is silently ignored by the flake evaluator. This looks like a copy-paste from a NixOS configuration and adds dead/misleading scaffolding. Remove it.

---

📄 contrib/upsert/upsert.c (L270-L273)

On register() failure this only emits WARNING and returns, leaving the allocated ext handle and its MemoryContext (a child of TopMemoryContext created by pg_grammar_ext_create) leaked for the postmaster's lifetime. The sibling grammar-extension module contrib/quel/quel.c handles the same failure by calling pg_grammar_ext_unregister(ext) to delete the context. Follow that convention for consistency and to avoid the leak.

Also consider whether WARNING is the right severity here: for a shared_preload_libraries module, a failed grammar registration silently leaves the extension loaded but non-functional (the UPSERT keyword/production is never composed), which is a POLA violation. quel matches this WARNING behavior, so this is a lower-priority point, but the missing unregister cleanup should be fixed.

💡 Suggested change

Before:

	if (!pg_grammar_ext_register(ext, &err))
		ereport(WARNING,
				(errmsg("upsert: register() failed: %s",
						err ? err : "(no detail)")));

After:

	if (!pg_grammar_ext_register(ext, &err))
	{
		ereport(WARNING,
				(errmsg("upsert: register() failed: %s",
						err ? err : "(no detail)")));
		pg_grammar_ext_unregister(ext);
	}

---

📄 pg-aliases.sh (L1-L1)

This whole file is a personal developer-convenience script. It is only ever sourced from shell.nix and is documented in .github/docs/pristine-master-policy.md as "Personal shell aliases". Such tooling does not belong in a patch destined for pgsql-hackers: it is out of scope, duplicates in-tree infrastructure (pgindent, pg_regress, meson test targets), and depends on many external tools not required by the project (trash, compdb, rr, flamegraph, codespell, clang-tidy, iostat/vmstat). Recommend keeping it in a personal/uncommitted overlay rather than the source tree, or at minimum outside the mailing-list patch.

---

📄 pg-aliases.sh (L7-L7)

$CC is unquoted and passed to basename. If CC contains spaces (e.g. ccache gcc, clang -flto, or a path with spaces), basename receives multiple arguments and returns the wrong value, silently misdetecting compiler changes and potentially triggering the destructive clean below. Quote it and pass only the program name.

💡 Suggested change

Before:

	local current_compiler="$(basename $CC)"

After:

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

---

📄 pg-aliases.sh (L191-L194)

The heredoc terminator EOF is unquoted, so $PG_SOURCE_DIR, $PG_BENCH_DIR and $bindir are expanded at generation time and written into the wrapper unquoted. Any of these paths containing a space (or shell metacharacter) yields a broken or injectable wrapper script (e.g. --suppressions=/my dir/... splits into two args). Quote the interpolated values inside the generated script.

💡 Suggested change

Before:

	--suppressions=$PG_SOURCE_DIR/src/tools/valgrind.supp \\
	--time-stamp=yes \\
	--log-file=$PG_BENCH_DIR/valgrind-%p.log \\
	$bindir/postgres "\$@"

After:

	--suppressions="$PG_SOURCE_DIR/src/tools/valgrind.supp" \\
	--time-stamp=yes \\
	--log-file="$PG_BENCH_DIR/valgrind-%p.log" \\
	"$bindir/postgres" "\$@"

---

📄 pg-aliases.sh (L515-L515)

Iterating over an unquoted $modified_files word-splits on whitespace, so any modified file whose path contains a space is broken into multiple bogus tokens (and then reported as "File not found"). Read the file list with a loop that preserves paths, e.g. git diff --name-only "$since" | grep -E '\.[ch]$' | while IFS= read -r file; do ...; done.

---

📄 pg-aliases.sh (L538-L540)

Unquoted $files in for f in $files word-splits filenames on whitespace, mishandling paths with spaces. Prefer piping git diff --name-only into a while IFS= read -r f loop.

---

📄 pg-aliases.sh (L514-L515)

pg-format reinvents a minimal pgindent driver by looping over changed files, duplicating functionality the in-tree src/tools/pgindent/pgindent already provides (it accepts a file list / commit range directly). This is a DRY violation and will drift from the canonical workflow; delegate to pgindent's own file-selection instead of re-implementing it here.

---

📄 shell.nix (L308-L308)

These scripts are exported as flake packages (packages = { inherit ... pgbenchScript; } in flake.nix), so they can be run standalone outside a devShell. They pin perf, flamegraph, and bash via store paths but leave pgrep/nproc/free/awk (and $CC, which is only set by the shellHook) as bare PATH lookups. Run outside an active devShell (or on a system lacking procps/coreutils/gawk in PATH), the script fails at the pgrep call. Either reference these via ${pkgs.procps}/bin/pgrep, ${pkgs.coreutils}/bin/nproc, etc., or add procps/gawk to the closure so the exported package is self-contained.

---

📄 shell.nix (L4-L6)

The system argument is accepted but never referenced anywhere in shell.nix. Drop the unused parameter (flake.nix passes it via inherit ... system ..., so also stop passing it there) to avoid dead argument surface.

---

📄 src/backend/bootstrap/.gitignore (L3-L3)

This .gitignore no longer covers bootparse.out, a generated build artifact. The Lime rule lime -d. bootparse.lime emits it, and the Makefile clean target explicitly removes it (rm -f ... bootparse.out). The sibling conversions add the corresponding .out entry (e.g. src/backend/parser/.gitignore adds /gram.out, src/backend/replication/.gitignore adds /syncrep_gram.out). Without it, bootparse.out shows up as untracked after a build. Add /bootparse.out here to match the established pattern.

💡 Suggested change

Before:

-/bootscanner.c

After:

/bootparse.out

---

📄 src/backend/Makefile (L201-L201)

Inconsistent removal of now-committed scanner sources (moderate confidence). This patch converts the flex/bison-generated scanners into checked-in source files: per src/backend/parser/Makefile and src/backend/replication/Makefile, scan.c, repl_scanner.c and syncrep_scanner.c no longer have generation rules and are committed (ADDED in the change set). You correctly dropped syncrep_scanner.c from this target, but left scan.c (line above) and repl_scanner.c as requested targets. They now resolve to committed files with no rule, so $(MAKE) -C <dir> ... repl_scanner.c only succeeds because GNU make treats an existing prerequisite-less file as up-to-date; the entries are stale and misleading. Drop scan.c and repl_scanner.c here too, matching the syncrep_scanner.c removal, since generated-parser-sources exists only to materialize generated files before nls.mk's find runs.

💡 Suggested change

Before:

	$(MAKE) -C replication	repl_gram.c repl_gram.h repl_scanner.c syncrep_gram.c syncrep_gram.h

After:

	$(MAKE) -C replication	repl_gram.c repl_gram.h syncrep_gram.c syncrep_gram.h

---

📄 src/backend/bootstrap/Makefile (L15-L18)

The make build path is broken for the bootstrap scanner. bootscanner.c (now a checked-in file, per its removal from .gitignore) does #include "bootscanner_lex.h", and meson.build generates bootscanner_lex.c/bootscanner_lex.h from bootscanner.lex via lime_lex_cmd. This Makefile has no rule to generate those files and does not add bootscanner_lex.o to OBJS, so under autoconf/make bootscanner.c will fail to compile (missing header) and the lexer object will never be linked. This also diverges from meson.build, which the rules require to stay in sync. Add a generation rule (e.g. bootscanner_lex.c bootscanner_lex.h: bootscanner.lex running lime -X), add bootscanner_lex.o to OBJS, and add the header to the forced dependencies.

---

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

The clean target does not remove the Lime-generated scanner artifacts bootscanner_lex.c and bootscanner_lex.h (generated from bootscanner.lex per meson.build). Generated files must be removed by clean; otherwise stale artifacts survive make clean. Add them to the rm -f list.

---

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

This drops the touch $@ that the referenced parser Makefile deliberately keeps. Per the parser Makefile's own comment, the touch exists so gram.h is marked no-older-than gram.c, otherwise VPATH builds from tarballs repeatedly try to rebuild the header. Lime emits bootparse.c and bootparse.h in a single lime -d. invocation here too, so this file is subject to the same timestamp ordering hazard. Either keep the touch $@ recipe for consistency with the parser Makefile, or the comment should explain why the timestamp concern does not apply here.

💡 Suggested change

Before:

bootparse.h: bootparse.c ;

After:

bootparse.h: bootparse.c
	touch $@

---

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

This rationale is stale/inaccurate. The sibling scanner file (bootscanner.c, header comment) states plainly that "boot_yylex no longer exists -- the parser is fed by the driver loop directly, not by a yylex() pull callback." boot_yylex is not defined anywhere in the bootstrap module. Justifying the union's tag/name on a function that no longer exists is misleading and will confuse future readers (POLA). Explain the real constraint instead: the union tag/name must match %token_type {YYSTYPE} in bootparse.lime and the union YYSTYPE forward declaration consumed by the generated parser and scanner.

---

📄 src/backend/catalog/genbki.pl (L1031-L1031)

The comment now points at bootscanner.c, but per the new scanner's own header, bootscanner.c is the generated parser-driver shim -- the id pattern [-A-Za-z0-9_]+ is actually declared in the source bootscanner.lex (line 42). Referencing the generated file sends a reader to the wrong place to find/verify this pattern. Point at the source instead. (moderate confidence)
s/bootscanner.c/bootscanner.lex/

💡 Suggested change

Before:

		# the "id" pattern in bootscanner.c, currently "[-A-Za-z0-9_]+".

After:

		# the "id" pattern in bootscanner.lex, currently "[-A-Za-z0-9_]+".

---

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

This Assert checks the parameter pointer funcname (a char **), which is always non-NULL, so the assertion is a no-op tautology. It appears the intent was to guard against strrchr returning NULL, i.e. Assert(*funcname). But note the check comes too late anyway: (*funcname)++ on line 1066 already dereferences the strrchr result before this Assert. High confidence.

---

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

NULL-deref hazard / behavior regression. The old code did name += strlen("pgextern.") first, then strrchr on the remainder, and explicitly handled the "no second dot" case (modname = NULL, funcname = whole rest). The new code calls strrchr(name, '.') on the full string and unconditionally does (*funcname)++. Because name starts with "pgextern.", there is always at least one dot, so this happens to not crash today. However, if the symbol is exactly "pgextern.foo" (no module separator), strrchr finds the dot in the prefix; modname then becomes an empty string via pnstrdup(name + 9, funcname - name - 9 - 1) where the length is -1 cast to size_t (a huge value) -> heap over-read/crash. The removed else branch previously protected exactly this case. Restore explicit handling for the missing second dot. High confidence.

---

📄 src/backend/bootstrap/bootscanner.c (L56-L57)

DeescapeQuotedString is already declared in src/include/utils/guc.h (line 166), which explicitly notes it is "exported because it is also used by the bootstrap scanner." Re-declaring it here via a bare extern in a .c file is fragile: if the canonical signature ever changes, this shadow declaration will silently drift and break at runtime. Include the header instead. Note also the comment is inaccurate: the function is defined in guc-file.c but declared in utils/guc.h.

---

📄 src/backend/bootstrap/bootscanner.c (L256-L261)

Line-number tracking is a diagnostic regression. This loop counts every newline in the whole input before any token is lexed or parsed, so s->yylineno is always the total line count of stdin by the time boot_yyerror runs. Every syntax error will therefore report the last line of the input rather than the line of the offending token. The retired flex scanner tracked the current line as it scanned. The current line needs to be advanced during lexing (e.g., in the emit callback / a per-newline rule), not pre-counted here.

---

📄 src/backend/bootstrap/bootscanner.c (L6-L11)

Aspirational/narrative comments discouraged by project standards. This header block narrates tool version history ("Lime v0.2.1's lexer subsystem"), the size of the retired implementation ("502-line hand-rolled state machine"), and an unused future capability ("Lime's lexer can suspend mid-token across feeds (post-v0.2.1)" further down). Comments should explain why the code is as it is now, not narrate history or advertise capabilities the code does not exercise. Trim to what a future reader needs.

---

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

This switch body will not pass pgindent: mixed/misaligned indentation such as if\t\t\t(len >= 2 ..., case\t\tOPEN: with embedded tabs, and the over-indented memcpy/literal[len] = '\0'; lines. Run pgindent (tabs, width 4) before submitting.

---

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

This Makefile no longer generates the scanner, so the make build breaks. In the meson build, scan.lex is compiled to scan_lex.c/scan_lex.h (via lime_lex_cmd) and scan_lex.c is linked as its own TU; scan.c here is only a driver that #includes scan_lex.h. This Makefile removed the old scan.c: FLEXFLAGS/FLEX_NO_BACKUP rules but added no replacement rule for scan.lex -> scan_lex.{c,h}, and OBJS has no scan_lex.o. Result: scan.c fails to compile (missing scan_lex.h) and the scanner is never built/linked. Add a scan_lex.c scan_lex.h: scan.lex rule (with scan_lex.o in OBJS, and the two-output primary/secondary pattern used for gram.c/gram.h) and keep it in sync with meson's scan_lex_gen. (high confidence)

---

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

The generated-grammar codegen here is drastically out of sync with src/backend/parser/meson.build, which is a hard requirement. meson invokes the pglime wrapper with --builddir/--srcdir/--privatedir and, for the backend grammar, --snapshot producing a third output gram_snapshot.c that defines base_yyBuildSnapshot() (and optionally gram_aot.c under AOT). The new parser_extension.c (added to OBJS) and parser_pushparse.c depend on that runtime snapshot subsystem. A bare lime -d. $< produces only gram.c/gram.h, never gram_snapshot.c, and neither gram_snapshot.o nor parser_pushparse.o are in OBJS, so the make build will fail to link (undefined snapshot/push-parse symbols) or silently drop the extension feature. The Makefile must generate and compile the same set of artifacts as meson. (high confidence)

---

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

lime is invoked as a bare hardcoded command. Every other generator in the tree goes through a configured make variable (e.g. $(BISON), $(FLEX)) so the build honors the configure-detected tool path and can be overridden. Introduce and use a $(LIME)/$(LIMEFLAGS) variable (defined in Makefile.global from configure) instead of hardcoding lime; otherwise VPATH/out-of-tree and cross builds that rely on a specific toolchain path will break. (moderate confidence)

---

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

The clean target no longer removes all generated artifacts. It now deletes only gram.c, gram.h, gram.out, but the make build is expected to generate scan_lex.c/scan_lex.h from scan.lex (see meson) and gram_snapshot.c (and gram_aot.c under AOT). Those generated files must be removed by clean (or distclean/maintainer-clean) or repeated builds will leave stale generated sources. Note scan.c was correctly dropped from clean since it is now a committed source, but the newly generated outputs are unaccounted for. (high confidence)

---

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

base_yyparse_fn is never assigned anywhere in the tree (searched all .c/.h): it is only defaulted to base_yyparse and read once. The comment above claims "parser_extension.c owns the swap" and gramparse.h claims the "Phase 4 subprocess pipeline overwrites it after dlsym", but no such write exists. This is dead flexibility / speculative scaffolding (YAGNI) that adds an indirect call on the SQL parse hot path for a path that isn't wired. Either wire the actual swap or drop the indirection and call base_yyparse() directly. Additionally, if it is ever written from another module, the extern declaration in gramparse.h needs PGDLLIMPORT or the Windows/MSVC build will fail to link. (high confidence)

---

📄 src/backend/parser/parser.c (L61-L64)

These cross-TU functions are declared inline in parser.c rather than in a shared header. The identical declarations are duplicated in parser_pushparse.c (its lines 70-73), so the two are only kept consistent by hand -- a signature drift (parameter types/order or return type) would compile in each TU yet be undefined behavior at link/runtime. Per PostgreSQL convention, declare these in a header (e.g. parser_extension.h) included by both the definition and the caller so the compiler enforces signature agreement. (moderate confidence)

---

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

getenv("PG_LIME_PUSHPARSE") runs on every raw_parser() call, i.e. on the hot path for all SQL parsing. Using an environment variable as a runtime knob violates PostgreSQL configuration conventions (this should be a GUC), is undocumented and untested, and getenv is not the intended backend-config mechanism. It is also a footgun: an env var silently switching the entire parse path is easy to misuse. Replace with a GUC (or drop it if it's only a temporary A/B scaffold), and add docs + tests for the behavior it enables. (high confidence)

---

📄 src/backend/parser/parser.c (L84-L88)

Comment is inaccurate about what the call does. pg_grammar_ext_lock_parser() does not merely make subsequent register calls fail: on its first call with pending extensions it composes the extension grammars into the active snapshot and can ereport(ERROR) on compose failure (see parser_extension.c:502-519). Calling it unconditionally at the top of raw_parser() therefore means a broken grammar extension aborts every parse, not just registration. Fix the comment to describe the actual behavior (compose-on-first-call, may ereport). (moderate confidence)

---

📄 src/backend/parser/parser.c (L129-L131)

The push-parse path returns early via return ok ? pushtree : NIL;, mapping a failed parse to NIL -- the same value a legitimately empty parse yields. The pull path below distinguishes error (yyresult != 0) from an empty parsetree; here a parse error is silently swallowed into an empty result unless raw_parser_lime_pushparse() itself ereport(ERROR)s on failure. Confirm that raw_parser_lime_pushparse reports parse errors via ereport(ERROR) rather than returning false, otherwise malformed SQL on the push path is silently treated as an empty statement list. (moderate confidence)

---

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

This comment is inaccurate and describes a mechanism that does not exist. base_yyparse_fn is defined in parser.c (line 45), initialized to base_yyparse, and never reassigned anywhere in the tree. The referenced "Phase 4 subprocess pipeline (parser_extension.c) [that] overwrites it after dlsym-ing the rebuilt parser .so" was removed: parser_extension.c states "The historical Track A path (serialize -> fork lime + cc -> dlopen a cached .so) has been removed entirely", and parse-time extension dispatch now goes through raw_parser_lime_pushparse()/pg_grammar_ext_reduce_by_ruleno(), not through this pointer. As committed, (*base_yyparse_fn)(yyscanner) always calls base_yyparse, so the function-pointer indirection is dead scaffolding (YAGNI) and the comment is aspirational/false. Remove the indirection and the extern, and call base_yyparse() directly; or, if kept, the comment must describe actual current behavior. Confidence: high.

---

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

Portability: if this exported variable is retained, it needs PGDLLIMPORT to be usable from other modules/extensions on Windows/MSVC, matching the convention of peer parser globals (e.g. extern PGDLLIMPORT int backslash_quote; in parser.h). Note, however, that the preferable fix is to delete base_yyparse_fn entirely, since it is only ever set to base_yyparse and the dlsym-overwrite path it exists for was removed. Confidence: high.

---

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

Comment inaccuracy. Lime does not emit a YYSTYPE "union body": gram.lime declares %token_type {YYSTYPE} (which references this typedef) plus per-symbol %type name {C-type} declarations; there is no %union block "inside gram.lime's %include block" to stay in sync with. Also, the listed consumers pl_gram, ecpg do not use this declaration -- they have their own separate unions (src/pl/plpgsql/src/pl_gram_types.h and src/interfaces/ecpg/preproc/preproc_yytype.h) and do not include gramparse.h. This makes the DRY hazard worse than stated: the member list here must be hand-kept consistent with dozens of scattered %type declarations in gram.lime, with no single authoritative union to diff against. Fix the comment to describe the real Lime mechanism, and ideally generate this typedef from the grammar rather than hand-maintaining it. Confidence: high.

---

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

Use-after-free: this deletes ext->context (which owns both the ExtToken list and the ext handle itself), but it never removes the corresponding entry from the static pending[] array. pending[i].ext still points at the now-freed handle. After compose (parser_locked == true), pg_grammar_ext_foreach_token() iterates pending[i].ext->tokens (parser_pushparse.c calls it), dereferencing freed memory. This path is reachable: contrib/quel/quel.c unregisters on failure. The WARNING only fires in the !parser_locked case and doesn't prevent the dangling entry either. Remove the pending entry (or reject unregister after compose) instead of documenting it as acceptable.

---

📄 src/backend/parser/parser_extension.c (L722-L723)

Non-idiomatic allocation: when pending is NULL this passes palloc0(0) to repalloc, allocating a throwaway zero-length chunk on the first call. Use a plain palloc on first allocation and repalloc thereafter.

💡 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 (L314-L314)

Missing space after != (pgindent/style). Should be Assert(symbol != NULL);.

💡 Suggested change

Before:

	Assert(symbol !=NULL);

After:

	Assert(symbol != NULL);

---

📄 src/backend/parser/parser_extension.c (L436-L437)

Future-tense/aspirational comment describing unshipped work. Per project comment hygiene, comments must describe current behavior, not planned steps. Trim the "SIGHUP-driven teardown/recompose is a later step; for now document the limitation" narrative.

---

📄 src/backend/parser/parser_extension.c (L700-L706)

Dead branching: both the if (rule->reduce != NULL) and else branches emit the identical .\n. Collapse to a single unconditional append (the reduce vs silent distinction is handled elsewhere), so the code isn't misleading about doing different things.

---

📄 src/backend/parser/parser_pushparse.c (L224-L232)

Potential snapshot leak on the compile-error path. The nconflict > 0 branch below correctly calls snapshot_release(snap), but this branch does not. If lime_compile_grammar_in_process_ex can return rc != 0 while still having written a non-NULL snap (partially built snapshot), it is leaked here. For symmetry and safety, release it before returning.

Confidence: medium (the Lime err/snap ownership contract is not verifiable from the tree, but the asymmetry with the nconflict path is a real smell; a defensive release is harmless when snap == NULL).

💡 Suggested change

Before:

	if (rc != 0 || snap == NULL)
	{
		if (errmsg_out)
			*errmsg_out = psprintf("in-process grammar compose failed: %s",
							   err ? err : "(no detail)");
		if (err)
			free(err);
		return false;
	}

After:

	if (rc != 0 || snap == NULL)
	{
		if (snap != NULL)
			snapshot_release(snap);
		if (errmsg_out)
			*errmsg_out = psprintf("in-process grammar compose failed: %s",
							   err ? err : "(no detail)");
		if (err)
			free(err);
		return false;
	}

---

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

error_lloc is dead state: it is written on both error paths but never read — the only use is (void) error_lloc;. The actual error location is taken from the scanner's own state inside scanner_yyerror(). Remove the variable and both assignments (YAGNI); it adds noise and falsely implies the location is used.

Confidence: high.

---

📄 src/backend/parser/parser_pushparse.c (L517-L523)

This switch hand-duplicates gram.lime's ascii_to_lime_token() (verified currently identical, all 18 cases). This is a DRY/maintainability footgun: any single-character token added or renamed in gram.lime must be mirrored here by hand, and silent drift would reject tokens at the wrong external code with no compile-time signal. Consider sharing a single mapping (e.g. exporting/reusing the generated helper) rather than maintaining two copies.

Confidence: high (duplication is real and confirmed against gram.lime:1619).

---

📄 src/backend/parser/parser_pushparse.c (L485-L486)

This ponytail: block is aspirational/future-tense scaffolding commentary ("If a future grammar extension ... upgrade this to Lime's Tier-1 full-statement fork-resolve", "Not done now"). Per project comment discipline, comments should describe what the code does now and explain WHY, not sketch unbuilt future work. Trim to the rationale for why one-token lookahead is sufficient today and drop the speculative upgrade path.

Confidence: high.

---

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

repl_gram.out is missing from this .gitignore. The Makefile's clean rule (lines 50-55) removes both repl_gram.out and syncrep_gram.out, meaning lime -d. repl_gram.lime emits repl_gram.out as a generated build artifact just like syncrep_gram.out. Since this directory has two grammars, both .out files must be ignored; otherwise a freshly built repl_gram.out shows up as untracked and can be accidentally committed. Add /repl_gram.out alongside /syncrep_gram.out.

💡 Suggested change

Before:

+/syncrep_gram.out

After:

+/repl_gram.out
+/syncrep_gram.out

---

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

These comments describe a design that was not shipped. They claim the append helpers "can't easily reach LEX_BUF_APPEND from outside an action body" and that the implementation "approximate[s] with a parallel C-side accumulator." The actual implementations in scan.c (scan_lex_addlit / scan_lex_addlitchar / scan_lex_litbuf_take / scan_lex_litbuf_start / scan_lex_litbuf_len) operate directly on the core scanner's real literal buffer (extra->literalbuf / extra->literallen) — there is no separate/parallel accumulator, and no LEX_BUF_APPEND reachability problem. Per PG comment discipline, comments must describe what the code does now; rewrite these to reflect that these helpers wrap the core_yy_extra_type literal buffer, and drop the aspirational "we approximate"/"can't easily reach" wording.

---

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

Comment drift: this block states the append helpers are "Implemented in scan.c using the Lime literal-buffer macros ... reachable only inside action bodies; we approximate with a parallel C-side accumulator." The shipped code does the opposite — it manipulates core_yy_extra_type->literalbuf/literallen directly and uses no parallel accumulator. Update to match the implementation.

---

📄 src/backend/parser/scan_lex_internal.h (L144-L145)

SCAN_LEX_OFFSET and SCAN_LEX_SET_LOC are non-hygienic: they hardcode the identifier user (via SCAN_LEX_CTX(user)) instead of taking an explicit argument, unlike every sibling macro below which takes u. This silently captures whatever user happens to be in scope. It compiles today only because every scan.lex action body has a user parameter, but any future caller without exactly that name gets a confusing error, and a differently-typed user in scope would bind silently. Make these consistent with the others by taking an explicit parameter, e.g. SCAN_LEX_OFFSET(u, p) / SCAN_LEX_SET_LOC(u, p), or at minimum document that user must be in scope.

---

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

Misindented switch arm will not survive pgindent. case OP: is indented one level deeper than the sibling case FCONST:, and the if/assignment use stray tab alignment (if (...)). The logic (falling through the string-bearing token codes to a single pstrdup) is correct, but this block must be reformatted to standard PG style (aligned case labels, single tab before the statement) so git diff --check / pgindent is clean.

💡 Suggested change

Before:

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

			break;

After:

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

---

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

Non-standard pointer-arithmetic spacing (text +i, slashstar = text +i, dashdash = text +i). PG style / pgindent requires text + i. Same problem below in memcpy(buf, text +2, hexlen) and memcpy(buf, text +1, octlen), and in scan_emit_cb start = (int) (text -ctx->scanbuf). These read as unary-plus/minus and will be flagged by pgindent; normalize to text + i, text + 2, text + 1, text - ctx->scanbuf.

💡 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/parser/scan.c (L455-L455)

Same text +N spacing defect here: memcpy(buf, text +2, hexlen) should be text + 2. (The octal handler below has the identical text +1 issue.)

💡 Suggested change

Before:

	memcpy(buf, text +2, hexlen);

After:

	memcpy(buf, text + 2, hexlen);

---

📄 src/backend/parser/scan.c (L547-L547)

start = (int) (text -ctx->scanbuf) has non-standard spacing; pgindent requires text - ctx->scanbuf.

💡 Suggested change

Before:

		start = (int) (text -ctx->scanbuf);

After:

		start = (int) (text - ctx->scanbuf);

---

📄 src/backend/parser/scan.c (L6-L6)

File header narrates internal project state that does not belong in committed source. References like "Lime v0.2.2's lexer subsystem" and "Phase 4 Track B" (the latter appears again in the SCAN_TOK_IDENT_RAW extension-hook comment) narrate implementation phases rather than explaining why the code is shaped this way. Comments should explain intent, not track project milestones/versions; these will draw fire on pgsql-hackers. Remove the version/phase references and keep only the design rationale.

---

📄 src/backend/parser/scan.c (L30-L30)

Copyright year is set into the future ("1996-2026"). PostgreSQL headers use the current year; a future year is incorrect and will be flagged. Use the current year range (e.g. 1996-2025).

💡 Suggested change

Before:

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

After:

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

---

📄 src/backend/parser/scan.c (L165-L170)

Potential int overflow in literal-buffer growth. literallen and literalalloc are int (see core_yy_extra_type). Here extra->literallen + len is computed in size_t, but the result is fed to pg_nextpower2_32(), whose contract requires the argument be <= PG_UINT32_MAX/2 + 1 (it Asserts, and shifts undefined otherwise). For a literal larger than ~2GB this asserts in debug builds and yields an under-sized allocation (then a heap overflow in the following memcpy) in production builds. Add an explicit bound against MaxAllocSize before growing, rather than relying on the input never being that large.

---

📄 src/backend/parser/scan.c (L181-L186)

literalalloc *= 2 can overflow int (literalalloc is int) and wrap negative for a large literal, causing repalloc() to be called with a negative/tiny size and a subsequent out-of-bounds write. Guard the doubling against MaxAllocSize (mirror the overflow-safe growth of the original scanner) instead of unconditionally doubling.

---

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

Every string-bearing token is pstrdup'd here on the parse hot path, but the FIFO already holds a palloc'd copy of the same string (made during scanner_init). That is a second allocation + copy for every IDENT/SCONST/OP/etc. token in the statement -- pure palloc churn the retired flex scanner did not incur (it palloc'd once, inside yylex). If the intent is only to relocate the string into the parser's memory context, document why the FIFO copy is insufficient; otherwise scanner_init should allocate directly in the parser context and this copy should be dropped.

---

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

Lifetime hazard for extension-keyword lexemes. On the extension-keyword hit path, val.keyword is set to lower, a string palloc'd in whatever context scanner_init ran in. Unlike the str-bearing token codes, keyword-typed tokens are NOT pstrdup'd into the parser context in core_yylex (the switch there only covers IDENT/UIDENT/SCONST/USCONST/BCONST/XCONST/FCONST/OP). parser_extension.h itself states token strings must outlive the parse call (RawStmt is consumed later by parse_analyze/planner). If this lower copy lives in a shorter-lived context than the returned parse tree, it dangles. Either pstrdup it into the same lifetime as base keywords (which point into a static const table) or ensure the allocation context matches the parse-tree lifetime.

---

📄 src/backend/parser/scan.c (L756-L758)

The comment claims val.keyword points at a "canonical lowercase form" to "keep the downstream contract," but the base path sets val.keyword = GetScanKeyword(...) which returns a pointer into a static const keyword table (stable, never freed, const-qualified). Assigning a freshly palloc'd lower here does NOT match that contract in lifetime or const-ness. If any grammar consumer relies on pointer identity into ScanKeywords or on the pointer's lifetime, this diverges. Clarify or align the two paths.

---

📄 src/backend/parser/scan.c (L633-L641)

Silent truncation of long $N parameter markers can produce a wrong parameter number. The param pattern (\${decdigit}+) permits arbitrarily many digits, but this copies at most 31 bytes into buf[32] and then parses that truncated string. For an over-long-but-leading-zero marker such as $00000000000000000000000000000005 (>31 chars) the copy stops before the significant digit, so pg_strtoint32_safe parses 0 instead of erroring/parsing 5 -- silent corruption, and a behavior change from the original scanner which parsed the full lexeme. Copy/parse the full lexeme (palloc len+1 like the ICONST path does) instead of a fixed 32-byte stack buffer.

---

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

Build is out of sync with meson.build and will not compile. The committed shims include generated lexer headers: repl_scanner.c does #include "repl_scanner_lex.h" and syncrep_scanner.c does #include "syncrep_scanner_lex.h". In meson (src/backend/replication/meson.build) these are produced by custom_target(... command: lime_lex_cmd) from repl_scanner.lex/syncrep_scanner.lex, emitting repl_scanner_lex.{c,h} and syncrep_scanner_lex.{c,h}, and the *_lex.c are compiled into repl_parser.

This Makefile has no rule to run lime on the .lex files, so repl_scanner_lex.h/syncrep_scanner_lex.h never exist and repl_scanner.o/syncrep_scanner.o fail to compile. Even if the headers existed, repl_scanner_lex.o/syncrep_scanner_lex.o are never added to OBJS, so the lexer implementation would be missing at link time.

Add rules to generate the lexer sources (e.g. repl_scanner_lex.c: repl_scanner.lex running lime with the lex subcommand, plus the .h: .c ; companion), add repl_scanner_lex.o syncrep_scanner_lex.o to OBJS, add them as prerequisites of the scanner objects, and remove all four generated *_scanner_lex.{c,h} files in clean. (high confidence)

---

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

clean removes the two grammar .out files but not the lexer artifacts that the meson build generates (repl_scanner_lex.c/.h, syncrep_scanner_lex.c/.h). If/when the missing lexer-generation rules are added (see the OBJS/rule finding above), these generated files must be removed here too, otherwise make clean leaves stale generated sources. (high confidence)

---

📄 src/backend/replication/syncrep_parse.h (L55-L63)

Duplicated comment block. Two nearly identical "Opaque scanner state." comments sit back-to-back before this struct. The first one describes "a staging buffer for delimited-identifier collection", but the second one explicitly states that accumulation lives in the Lime lexer's %literal_buffer and "no xdbuf field is needed here." The first block is stale/contradictory. Delete the first block and keep only the second, accurate one.

💡 Suggested change

Before:

/*
 * Opaque scanner state.  yyscan_t (public typedef: void *) points at one of
 * these.  The fields track the input cursor, the text of the last matched
 * flex-style rule (for yyerror's "at or near" message), and a staging
 * buffer for delimited-identifier collection.
 */
/*
 * Opaque scanner state.  yyscan_t (public typedef: void *) points at one of
 * these.  The fields track the input cursor and the text of the last

After:

/*
 * Opaque scanner state.  yyscan_t (public typedef: void *) points at one of
 * these.  The fields track the input cursor and the text of the last

---

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

The pos field is dead scaffolding. In syncrep_scanner.c it is only written (s->pos = 0; in syncrep_scanner_init) and never read; the lexer drives the scan via SyncRepLexFeedBytes. The comment even admits it is "advisory only." Per YAGNI, drop the field (and its initialization) unless a real consumer exists.

---

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

Correctness bug: the catch-all rule emits token -1 for any unexpected character, but the parse loop below (replication_yyparse) treats token <= 0 as end-of-input. So replication_yylex returning -1 makes the parser see EOF and stop cleanly instead of raising a syntax error. Trailing/embedded garbage characters are therefore silently accepted, contradicting the comment's claim of "same observable outcome as the flex/bison build" (where an unknown character yields a syntax error). Emit a token code that is positive and unknown to the grammar (so the parser reports a syntax error), or handle -1 explicitly in the lex/parse loop rather than folding it into the <= 0 EOF check.

---

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

UCONST value is truncated and unvalidated: the token text is copied into a fixed buf[32] (n capped at 31 chars) before strtoul, so an integer literal of 32+ digits is silently truncated to a wrong value. endp and errno are set up (errno = 0) but never checked, so overflow/garbage is ignored. The retired scanner ran strtoul over the full token text (no 31-char cap). For a token that is already NUL-terminable, prefer parsing the full text (e.g. pnstrdup then strtoul) and reject overflow/non-numeric input rather than truncating.

---

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

Unbounded token FIFO growth with no overflow guard: newcap = s->cap * 2 and newcap * sizeof(ReplToken) can overflow int/size_t for pathological input, producing a too-small allocation and subsequent heap overflow. Guard the doubling (cap the growth / check for overflow before multiplying), or use the existing List/StringInfo infrastructure which already handles this.

---

📄 src/backend/replication/repl_scanner.c (L214-L217)

Stale comment: this block describes the retired flex/bison implementation ("flex returned the raw character code; Bison reported ...") rather than what this code does now. Per project conventions comments must describe current behavior. Reword to state that this branch handles the lexer's unexpected-character sentinel and how the driver surfaces it as a syntax error.

---

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

The Makefile in this same change now generates two additional Lime artifacts that are not committed as source: jsonpath_gram_yytype.h (used as a build dependency: jsonpath_gram.o jsonpath_scan.o: jsonpath_gram.h jsonpath_gram_yytype.h) and jsonpath_gram.out (removed in the clean: rule). Neither is tracked, so both will appear as untracked files after a build. While correctly dropping the now-real source /jsonpath_scan.c, this .gitignore should also ignore the new generated outputs. Confidence: high.

💡 Suggested change

Before:

-/jsonpath_scan.c

After:

/jsonpath_gram_yytype.h
/jsonpath_gram.out

---

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

Dead / misleading error path. SyncRepLexAlloc is passed palloc as its allocator, and in the backend palloc never returns NULL — on OOM it ereport(ERROR)s and longjmps out, so this lex == NULL branch is unreachable. The same assumption is baked into syncrep_yyAlloc(palloc) below. This reads as if a malloc-style (NULL-returning) allocator were in use, which is a footgun. Either drop the dead check (backend palloc semantics), or, if the intent is genuine OOM tolerance, pass a soft allocator such as one wrapping MemoryContextAllocExtended(..., MCXT_ALLOC_NO_OOM). As written it is confusing dead code.

---

📄 src/backend/replication/syncrep_scanner.c (L133-L134)

Typo in comment: "unterminated-quoted- identifier" has a stray space after the hyphen (pgindent line-wrap artifact). Should read "unterminated-quoted-identifier".

💡 Suggested change

Before:

* scanner-level error (currently only the unterminated-quoted- identifier
	 * case).  syncrep_yyerror's first-error-wins guard makes this safe even

After:

	 * scanner-level error (currently only the
	 * unterminated-quoted-identifier case).

---

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

Build-breaking: this rule lists jsonpath_gram_yytype.h as a prerequisite, but that file does not exist in the tree and no rule generates it. Lime's output for this grammar is only jsonpath_gram.c/jsonpath_gram.h (see lime_kw in the top-level meson.build: output: ['@BASENAME@.c', '@BASENAME@.h']), so _yytype.h is not generated. Every other subsystem in this change that uses this pattern commits the header (e.g. spec_gram_yytype.h, repl_gram_yytype.h, boot_gram_yytype.h, seg_gram_yytype.h are all added files), and jsonpath_gram.lime's own header comment says the jpMakeItem*() constructors are "exported through jsonpath_gram_yytype.h". Since the file is missing, make in src/backend/utils/adt will fail with "No rule to make target 'jsonpath_gram_yytype.h', needed by 'jsonpath_gram.o'". Either add the committed jsonpath_gram_yytype.h header, or drop it from this prerequisite (the jpMakeItem* prototypes are actually already declared in jsonpath_internal.h). Note meson.build also never references this header, so the two build systems are out of sync. Confidence: high.

💡 Suggested change

Before:

jsonpath_gram.o jsonpath_scan.o: jsonpath_gram.h jsonpath_gram_yytype.h

After:

jsonpath_gram.o jsonpath_scan.o: jsonpath_gram.h

---

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

Stale comment: jsonpath_yylex_internal does not exist anywhere in the code. The scanner is now a hand-rolled callback-based lexer; yytext is actually updated by set_yytext() inside the emit callback (jp_emit_cb) in jsonpath_scan.c and cleared before pushing the EOF token in jsonpath_yyparse. Update the comment to describe the current mechanism instead of a nonexistent function. (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 scanner's emit callback (jp_emit_cb).  Read by
	 * jsonpath_yyerror{,_token} for the "at or near \"X\"" branch.  Mirrors

---

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

Dead field: pos is only ever written (s->pos = 0 in parsejsonpath) and never read. The hand-rolled lexer feeds the whole buffer via JsonPathLexFeedBytes and does not use a cursor stored here. Per YAGNI, drop this field (and its initialization). (high confidence)

---

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

Dead field: this struct-level hi_surrogate is initialized to -1 in parsejsonpath but never read. The actual surrogate-pair state is tracked by a local int hi_surrogate passed to addUnicode() in jsonpath_scan.c. Remove this unused field to avoid the false impression that scanner-wide surrogate state is carried here. (high confidence)

---

📄 src/backend/utils/init/miscinit.c (L1865-L1871)

The comment's claim is inaccurate for two of the three callers of process_shared_preload_libraries(). Besides the fork-based postmaster (postmaster.c), this function is also called from the standalone/single-user path (tcop/postgres.c) and from the EXEC_BACKEND/Windows child path (postmaster/launch_backend.c, SubPostmasterMain). In single-user mode there is no fork at all, and under EXEC_BACKEND/Windows this runs in every exec'd child (with a fresh, unlocked parser state), so pg_grammar_ext_prewarm() re-composes the grammar in each backend -- exactly the per-session first-query compose cost this comment claims to avoid. Reword so it describes the actual behavior across all callers (compose once per process where preloads are processed; on fork-based Unix the composed snapshot is inherited by forked backends, but EXEC_BACKEND/single-user do not benefit from that inheritance) rather than asserting a postmaster-only, fork-only guarantee. Confidence: high.

---

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

Behavioral change vs. the pre-port scanner needs confirmation. The upstream jsonpath scanner's <INITIAL>{special} rule returned the matched byte to the parser (e.g. return *yytext;), so an input character like # reached the grammar and triggered a syntax error. Here, # (and the default case) is silently dropped without emitting any token, so a jsonpath such as $# would parse as $ instead of erroring. The comment claims "the pre-port scanner returned 0 for '#'", but returning 0 to a bison-style parser means end-of-input, not a syntax error either. Please verify the exact pre-port behavior for #: if it produced a syntax error, silently skipping it here is a correctness regression (a footgun that silently accepts previously-invalid input). If it must be rejected, emit a token the grammar rejects rather than returning.

---

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

Irregular spacing text +1 will fail pgindent / git diff --check. Use text + 1.

💡 Suggested change

Before:

				addstring_internal(true, text +1, (int) len - 1, s);

After:

				addstring_internal(true, text + 1, (int) len - 1, s);

---

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

File-header comment describes migration history and pins a tool version ("Lime v0.2.2", "replaces the hand-rolled state machine that used to live here", "copied verbatim from the pre-port scanner"). Per the project's comment-accuracy discipline, header comments should describe what the code does now, not the porting history; the "v0.2.2" pin will drift as the generator evolves. Consider rewording to describe the current responsibilities of this file without the migration narrative and version pin.

---

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

The safety of these sentinel codes rests entirely on this hand-written comment: the driver in jsonpath_scan.c dispatches with if (code >= JP_TOK_BASE) to tell sentinels apart from real parser token codes. Nothing machine-checks that the highest parser token stays below JP_TOK_BASE (1000). If jsonpath_gram.lime grows enough tokens, or Lime renumbers tokens, a parser code could reach >= 1000 and be silently misclassified as an "unknown sentinel" (suppressed) or as a JP_TOK_* case -- producing wrong tokens with no compile-time failure. Add a StaticAssertDecl (e.g. StaticAssertDecl(UMINUS < JP_TOK_BASE, "parser token codes must stay below JP_TOK_BASE"); against the highest-numbered generated token) so the boundary is enforced by the compiler rather than a comment. Confidence: high that the coupling is unchecked; the specific token values quoted in the comment (TO_P=12 ... RBRACE=74) cannot be verified here since jsonpath_gram.h is generated and not in the diff.

---

📄 src/bin/pgbench/.gitignore (L3-L3)

The removal of /exprscan.c is correct, since it is now a committed source file. However, this .gitignore is now out of sync with the new build outputs. The Makefile's exprparse.c: exprparse.lime rule runs lime -d. $<, which also produces exprparse.out in the source tree (confirmed by distclean now doing rm -f exprparse.h exprparse.c exprparse.out). Since exprparse.out is a generated artifact left in the working tree after a build, it should be added here so it is not accidentally committed. Suggest adding /exprparse.out.

💡 Suggested change

Before:

-/exprscan.c

After:

/exprparse.out

---

📄 src/backend/utils/misc/Makefile (L48-L49)

The clean rule is removed entirely, but the make build now produces new generated artifacts that must be cleaned. meson.build generates guc_file_lex.c and guc_file_lex.h from guc_file.lex via a Lime custom_target, and guc-file.c #includes the generated guc_file_lex.h. To stay in sync, this Makefile needs (a) a rule to generate guc_file_lex.c/guc_file_lex.h from guc_file.lex, (b) guc_file_lex.o added to OBJS (otherwise GucLexAlloc/GucLexFeedBytes/... referenced by guc-file.c won't be compiled or linked and the make build breaks), and (c) a clean rule that removes guc_file_lex.c guc_file_lex.h (mirroring how src/backend/parser/Makefile cleans its lime-generated gram.c/gram.h). As written the make build is broken and leaves generated files behind on make clean. (high confidence)

---

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

The Makefile is out of sync with meson.build for the Lime lexer. meson.build generates exprscan_lex.c/exprscan_lex.h from exprscan.lex via a custom target (lime -X -d<outdir> exprscan.lex, i.e. lime_lex_cmd) and compiles/links the result. This Makefile adds only the exprparse.lime -> exprparse.c rule and never generates the Lime lexer. Since exprscan.c does #include "exprscan_lex.h" and calls ExprLexAlloc/ExprLexFeedBytes/ExprLexFeedEOF/ExprLexFree/ExprLexErrorMessage (all defined only in the generated exprscan_lex.c), the autoconf build will fail: missing exprscan_lex.h at compile time and undefined ExprLex* symbols at link time. You need to (1) add a rule to generate exprscan_lex.c/exprscan_lex.h from exprscan.lex, (2) add exprscan_lex.o to OBJS so it is compiled and linked, and (3) update .gitignore/clean accordingly.

💡 Suggested change

Before:

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

After:

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

# exprscan_lex is the Lime-generated lexer (from exprscan.lex).
exprscan_lex.h: exprscan_lex.c ;

exprscan_lex.c: exprscan.lex
	lime -X -d. $<

---

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

exprscan.c includes the generated exprscan_lex.h, so it must depend on that header too (not just exprparse.h/exprscan_internal.h); otherwise a parallel (make -j) build can compile exprscan.o before the Lime lexer header is generated. Add exprscan_lex.h (and add exprscan_lex.o to this line / OBJS once the generation rule exists).

💡 Suggested change

Before:

exprparse.o exprscan.o: exprparse.h exprscan_internal.h

After:

exprparse.o exprscan.o exprscan_lex.o: exprparse.h exprscan_internal.h exprscan_lex.h

---

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

clean/distclean removes the generated parser artifacts but not the generated Lime lexer files. exprscan_lex.c and exprscan_lex.h are build products (per meson.build's exprscan_lex custom target) and must be removed here, otherwise make distclean leaves generated files behind and out-of-tree/rebuild behavior breaks.

💡 Suggested change

Before:

	rm -f exprparse.h exprparse.c exprparse.out

After:

	rm -f exprparse.h exprparse.c exprparse.out exprscan_lex.c exprscan_lex.h

---

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

Build breakage on the Makefile (autoconf) build. guc-file.c #includes the generated "guc_file_lex.h", but the Makefile provides no rule to run the Lime lexer generator, no dependency edge for guc_file_lex.c/.h, and does not add guc_file_lex.o to OBJS. The removed clean: rm -f guc-file.c rule was the only edit here. Only meson.build has the custom_target. Result: the autoconf build cannot compile guc-file.o (missing header) and won't link the lexer, so this commit is not buildable/bisectable on the Makefile path. Add the generation rule, the header dependency, and the object to OBJS (or drop the Makefile build for this dir consistently).

---

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

Silent failure on allocation failure. GucLexAlloc(palloc) is passed palloc, which itself ereport(ERROR)s on OOM, so it never returns NULL - this branch is dead code. But if the intent is that GucLexAlloc can return NULL, then returning here leaves the token queue empty and ParseConfigFp treats a real allocation failure as a valid, empty config file (OK=true). That silently masks misconfiguration/OOM instead of erroring, unlike the flex path which used siglongjmp to report a fatal scanner error per-file. Either remove the dead NULL branch (if palloc is used) or surface an error (set io_error / record_config_file_error and OK=false) instead of pretending EOF.

---

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

A non-OK GucLexFeedBytes status is silently discarded: it only gates the GucLexFeedEOF call. If the Lime lexer reports a scan failure via lex_status (rather than emitting a GUC_ERROR token), the tokens collected so far are processed and no error is surfaced - the tail of the file is silently dropped and ParseConfigFp can return OK=true. The retired flex path treated fatal scanner errors as per-file failures. Capture and surface a non-GUC_LEX_OK status (record_config_file_error + OK=false) rather than ignoring it.

---

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

Eager whole-file slurp + full-token FIFO materialization changes memory behavior vs. the streaming flex scanner and is unbounded. There is no size limit on the config file and the comment's 'Config files are bounded in size' is an unverified assumption. Worse, ParseConfigFp recurses on include/include_dir/include_if_exists inside this loop while the parent's fully-materialized token FIFO (each token with its own palloc'd text) is still live, so deeply nested includes multiply live memory. A large or hostile postgresql.conf / included file can cause excessive memory use. Consider streaming, or at least bound the input size and free the parent's FIFO before recursing.

---

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

Unbounded doubling of cap (cap *= 2) can overflow size_t for very large inputs, wrapping to a small value and passing a too-small size to repalloc, leading to a heap buffer overflow in the subsequent fread. Add an overflow guard (e.g. reject files above a sane limit, or check for wraparound before doubling). Also there is no CHECK_FOR_INTERRUPTS in this read loop, so reading a huge/blocking file cannot be interrupted.

---

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

Missing space after '!=': if (text !=NULL). Inconsistent with surrounding style and will not pass pgindent/whitespace checks.

💡 Suggested change

Before:

	if (text !=NULL)

After:

	if (text != NULL)

---

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

Per-token round-trip through StringInfo is needless palloc/memcpy churn: each token text is already palloc'd once in the FIFO, and guc_yylex copies it again into tokbuf on every call (tokbuf grows to the largest token). Since GUC_YYTEXT is only read immediately after each guc_yylex (for opt_name/opt_value/near-token error), GUC_YYTEXT could simply point at tok->text (with a stable empty-string fallback for the NULL/EOF case), avoiding the duplicate storage and the extra memcpy per token.

---

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

Implementation-diary / aspirational commentary. References to 'Lime v0.2.2', 'Phase 5', 'guc-file.lex', and '~470 lines of hand-rolled state machine' describe project history and process rather than what the code does now. PostgreSQL comments explain the current 'why', not the migration story - trim these to describe present behavior.

---

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

This is out of sync with src/bin/psql/meson.build. There, psqlscanslash.lex is a Lime source compiled by a custom_target into psqlscanslash_lex.c/psqlscanslash_lex.h, and psqlscanslash.c #includes the generated psqlscanslash_lex.h. This Makefile removes the old flex rule but adds no rule to generate psqlscanslash_lex.c/.h via Lime and never adds psqlscanslash_lex.o to OBJS. Under make, psqlscanslash.c will fail to find psqlscanslash_lex.h and the generated lexer object will be missing, so the autoconf/make build breaks. The claim "no codegen rule" contradicts meson, which clearly runs codegen. Add the Lime codegen rule and the generated object to OBJS (mirroring the meson custom_target). (high confidence)

---

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

"Phase 2h" is an internal project-management label with no meaning in the committed tree; it will read as stale the moment it merges. Comments should describe what the code does now. Drop the phase tag (and reconcile the comment with the actual codegen requirement above). (high confidence)

---

📄 src/bin/pgbench/exprscan.c (L127-L128)

Overflow-unsafe growth: newcap is int and newcap * sizeof(ExprToken) is an int * size_t multiplication that can overflow before the size_t widening, under-allocating and causing a heap buffer overflow on the subsequent expr_tokens[expr_ntokens] write. This also uses raw pg_realloc while the initial allocation uses the overflow-checked pg_malloc_array, so the two paths have inconsistent overflow semantics. Use the overflow-checked idiom pg_realloc_array.

💡 Suggested change

Before:

			expr_tokens = pg_realloc(expr_tokens,
									 newcap * sizeof(ExprToken));

After:

			expr_tokens = pg_realloc_array(expr_tokens, ExprToken, newcap);

---

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

pgindent will not accept this spacing: the unary/pointer-arithmetic operands are written as text +1 and text +len. They must be text + 1 and text + len (with spaces on both sides of the binary +).

💡 Suggested change

Before:

		end_offset = ctx->input_start_offset +
			(int) ((text +len) -ctx->input_base);

After:

		end_offset = ctx->input_start_offset +
			(int) ((text + len) - ctx->input_base);

---

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

Same spacing defect: text +1 should be text + 1. Will not pass pgindent / git diff --check.

💡 Suggested change

Before:

					memcpy(s, text +1, nlen);

After:

					memcpy(s, text + 1, nlen);

---

📄 src/bin/pgbench/exprscan.c (L417-L418)

The FUNCTION token's name string is pg_malloc'd here and stored in val.str, then consumed only by pgb_find_func(), which does a case-insensitive compare and never takes ownership or frees it. Unlike the VARIABLE case (where pgb_make_variable stores the pointer), this string is leaked on every function reference. pgbench is short-lived so it is not fatal, but it is a genuine leak; free it after pgb_find_func returns (or avoid the allocation and match directly on the token text).

---

📄 src/bin/pgbench/exprscan.c (L95-L99)

Reentrancy footgun: the API surface (yyscan_t expr_scanner_init(...), threading yyscanner through the parser) advertises per-instance scanner state, but all real state (expr_source, expr_lineno, expr_start_offset, expr_command, last_was_newline, and the whole token FIFO) is module-scope static. This works only because pgbench parses one expression at a time and never nests, but the mismatch between the yyscan_t contract and the global state is a maintenance hazard: any future nested/overlapping expr_scanner_init would silently clobber the FIFO and offsets. Consider hanging this state off PsqlScanState (or the scanner object) rather than file-scope statics.

---

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

expr_pre_scan asserts buffer_stack == NULL and reads only state->scanbuf/scanbufpos/scanbuflen, yet the INITIAL-mode path and the cur_buf/cur_pos/set_cur_pos helpers fully support a non-NULL buffer_stack. In a production build (NDEBUG) the Assert is compiled out, so if a buffer is ever pushed during expression scanning the pre-scan would read the wrong buffer while INITIAL mode reads the correct one, silently mis-tokenizing. Either remove the now-dead buffer_stack handling in the helpers (if it truly can't happen) or make the pre-scan honor buffer_stack, so both paths agree.

---

📄 src/fe_utils/Makefile (L53-L53)

This breaks the make-based (autoconf) build and desyncs it from meson.build. The claim "no codegen rule needed" is wrong: only the flex scanner (scan.c-style) went away. psqlscan.c now #includes psqlscan_lex.h and calls PsqlLexAlloc/PsqlLexFeedBytes/PsqlLexFree, all of which come from the Lime-generated psqlscan_lex.c/.h. meson generates these via a custom_target (lime_lex_cmd) and archives the object into psqlscan_lib, but this Makefile has (a) no rule to run lime on psqlscan.lex -> psqlscan_lex.c/.h, and (b) no psqlscan_lex.o in OBJS. Result: psqlscan.c fails to compile (missing header) and libpgfeutils.a is missing the PsqlLex* symbols at link time. Add a Lime codegen rule and the object to OBJS, mirroring the gram.c: gram.lime / lime -d. $< pattern used in src/backend/parser/Makefile and src/bin/pgbench/Makefile in this same series. (high confidence)

---

📄 src/fe_utils/Makefile (L68-L68)

clean/distclean no longer removes the newly generated Lime artifacts. rm -f psqlscan.c was correctly dropped (psqlscan.c is now committed source), but the make build now generates psqlscan_lex.c and psqlscan_lex.h (and a possible .out file), none of which are cleaned here. This leaks stale generated files across rebuilds and diverges from the clean targets updated in the parser/pgbench Makefiles in this series. Add rm -f psqlscan_lex.c psqlscan_lex.h. (high confidence)

---

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

Removing the #else /* ! x86_64 */ block that #undefs the AVX symbols is a portability regression, not a cosmetic cleanup. USE_AVX2_WITH_RUNTIME_CHECK, USE_AVX512_CRC32C_WITH_RUNTIME_CHECK, and USE_AVX512_POPCNT_WITH_RUNTIME_CHECK are configure-time defines (set in configure.ac/meson.build, exposed via pg_config.h) that gate compilation of x86 AVX intrinsics in checksum.c, pg_crc32c_sse42.c, and pg_popcount_x86.c. In a "universal" macOS build the configure host may be x86_64 (so these get defined), but the aarch64 slice is compiled from the same pg_config.h; the deleted #undefs were exactly what stopped those x86-only code paths from being built for aarch64. Dropping them will cause the aarch64 compile to pull in AVX intrinsics and fail to build. This block must be retained. (high confidence)

The #elif defined(__aarch64__) refactor for USE_NEON is fine on its own, but it should be done without removing the AVX #undef guard.

💡 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/fe_utils/psqlscan.c (L247-L248)

PsqlLexAlloc()'s return value is used immediately by PsqlLexSetState(lex, ...) without a NULL check. psql_lex_malloc wraps pg_malloc (which aborts on OOM), but that only guarantees the allocator callback won't return NULL to Lime; it does not guarantee PsqlLexAlloc itself can't return NULL on an internal failure. If it can, this is a NULL dereference. Confirm the Lime contract; if NULL is possible, check it here.

---

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

A fresh PsqlLexer is allocated and freed on every loop iteration. On the PSQL_LEX_ERROR / {other} path the loop emits a single byte and advances by 1, so a run of unrecognized bytes triggers one malloc+free per byte -- O(n) allocator churn on a hot interactive/scripting path, a regression versus the persistent flex scanner. Consider allocating the lexer once per psql_scan() call and reusing it across iterations (reset state instead of realloc). Any performance claim here needs a reproducible benchmark.

---

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

The round-trip PsqlLexSetState(lex, (int) state->start_state) / state->start_state = (ScanState) PsqlLexCurrentState(lex) silently assumes the generated lexer's internal start-condition integers are numerically identical to the ScanState enum ordinals (ST_INITIAL, ST_XB, ...). The .lex uses generated PSQL_STATE_* constants while this file uses ST_* enum values; the two are independently generated artifacts. If Lime ever renumbers its start conditions, psql_classify_eol will misclassify quote/comment/dollar-quote continuation states and produce wrong prompts or mis-terminate statements, with no compile-time check catching it. Add a static assertion or an explicit mapping to lock the coupling.

---

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

Copyright range ends in 2026 (a future year relative to the current date) and the header narrates version-pinned tooling ("Lime v0.2.2's lexer subsystem") plus an internal codename ("Strategy-D streaming driver"). Per PostgreSQL comment discipline, use the tree's standard copyright range and avoid version-pinned/codename references that will drift and mean nothing to future readers; comments should explain why, not narrate an internal strategy name.

💡 Suggested change

Before:

 * The state machine moved to psqlscan.lex (Lime v0.2.2's lexer
 * subsystem).  This file holds:

After:

 * The state machine lives in psqlscan.lex; this file holds:

---

📄 src/fe_utils/psqlscan.c (L604-L606)

psql_scan_reset() does not reset state_before_str_stop, xcdepth is reset but state_before_str_stop is left stale across a reset. The .lex sets state_before_str_stop before consuming it, so this is likely harmless, but for symmetry and to avoid a stale-state footgun on reuse, reset it alongside the other cross-line fields.

---

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

Dead scaffolding: the var_text / var_text_len fields are never written or read by any consumer. STOP_VAR_RECURSE is documented as the case that populates these fields, but psql_emit_var_plain() handles recursion by echoing the raw text inline and returning false (no LEX_TERMINATE), so STOP_VAR_RECURSE is never assigned and both scanners treat it as "Not used". Per YAGNI/minimal-diff discipline, drop these two fields (and the STOP_VAR_RECURSE handling/comment) until an actual caller needs them. Confidence: high.

---

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

STOP_VAR_RECURSE is defined but never assigned anywhere; both driver shims list it under a "Not used" case. It is dead alongside the unused var_text/var_text_len fields. Remove it unless a caller is added. Confidence: high.

---

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

Comment/parameter mismatch: the file-header comment describes the consumed formula as (matched - buf) + matched_len (- pushback if any), but the macro parameter here is named mlen (not matched_len) and the macro implements no pushback subtraction. Align the comment wording with the actual parameter name and drop the "(- pushback if any)" clause since the macro does not do it. Confidence: high.

---

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

These 11 unprefixed macro shims are dead code and a serious footgun. Verified: none of the three consumer files (psqlscan.c, psqlscanslash.c, exprscan.c) use the bare names -- they all reference the ST_-prefixed enum values directly (e.g. exprscan.c line 739 does state->start_state = ST_INITIAL;). Defining xb, xc, xd, xe, xh, xq etc. as object-like macros in a header that is included by multiple translation units means any local variable/parameter/field named xb..xq in any TU that transitively includes this header gets silently rewritten to ST_XB.. -- baffling compile errors or silent miscompilation. Remove the entire shim block. (high confidence)

---

📄 src/include/fe_utils/psqlscan_int.h (L58-L60)

This justification is factually wrong and describes a compatibility need that does not exist. exprscan.c uses ST_INITIAL directly (line 739: state->start_state = ST_INITIAL;, and line 778 uses ST_XB), not the bare INITIAL. There is no caller relying on the INITIAL alias, so the stated rationale for the shims is invalid. Drop this comment along with the shim block. (high confidence)

---

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

Stale/aspirational comments. The source is now psqlscan.lex (psqlscan.l no longer exists in the tree), so psqlscan.l references are outdated. Also, project-internal roadmap language ("Pre-Phase 2h", "until those files are ported in their respective phases") and future-tense scaffolding notes do not belong in upstream comments -- comments must describe current behavior. Update .l -> .lex and drop the phase/porting narrative. (high confidence)

---

📄 src/include/fe_utils/psqlscan_int.h (L178-L178)

Stale file reference: this refers to psqlscan.l, which no longer exists (the source is psqlscan.lex). Fix the filename so the pointer isn't misleading. (high confidence)

💡 Suggested change

Before:

	 * query early.  See the {identifier} rule in psqlscan.l.

After:

	 * query early.  See the {identifier} rule in psqlscan.lex.

---

📄 src/include/fe_utils/psqlscan_int.h (L54-L55)

Stale file reference: the original flex %x states now live in psqlscan.lex, not psqlscan.l. Update the filename. (high confidence)

---

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

This paragraph documents an obsolete internal contract in future/temporary terms and even contradicts itself ("actually a pointer to char ... but callers should treat it as opaque", while the return type is now plainly void * and the sole in-tree callers at psqlscan.c:351 and :669 discard the return value with (void)). Since there is no YY_BUFFER_STATE handle stored in StackElem anymore, describe the current contract plainly: it prepares/FF-maps a buffer and returns it (same pointer as *txtcopy). Drop the flex-era narrative. (moderate confidence)

---

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

Incomplete/leftover sentence: "Pre-port these owned YY_BUFFER_STATE buf plus an externally-managed yyscan_t." is grammatically broken (missing verb) and describes the removed flex layout rather than the current one. Trim to the current-behavior description of bufstring/pos only. (moderate confidence)

---

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

This doc block is stale and self-contradictory. The file header (top of this file) states the Track A subprocess/.so path "was removed entirely" and that the in-process compose is the live implementation. Yet here pg_grammar_ext_dispatch_reduce is documented as a "[t]rampoline called from the rebuilt parser .so" and "part of the implicit ABI ... [with] the .so produced by the Phase 4 subprocess pipeline" that is resolved via dlopen. That mechanism no longer exists: the only live caller is pg_grammar_ext_reduce_by_ruleno() (in-process host-reduce dispatch), verified in parser_extension.c. Rewrite the comment to describe the actual in-process caller and drop the removed-.so/dlopen/--export-dynamic narrative. Comments must describe what the code does now.

---

📄 src/include/parser/parser_extension.h (L315-L317)

Aspirational/future-tense comment for a mechanism that was removed, not upcoming. The file header says the subprocess/fork-lime/dlopen path "has been removed entirely", but this comment advertises it as "the upcoming subprocess pipeline that will concatenate this fragment ... fork lime to compile, and dlopen the result." Per PostgreSQL comment discipline, remove the future-tense text; document only the current use (the dummy_grammar_ext smoke test verifying the serialized .lime fragment).

---

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

High confidence. $(PYTHON) is only assigned by PGAC_PATH_PYTHON, which configure runs solely when --with-python=yes (configure.ac:1296). Since --with-python defaults to no (configure.ac:905), @PYTHON@ is empty in a default build, so this recipe expands to running <empty> .../lime_to_bison_gram.py $< $@ -- i.e. it tries to exec the .py file as a shell script and the ecpg build fails for the majority of users who don't build PL/Python.

Unlike the make build, the meson counterpart uses meson's always-available python (the build-script interpreter), so the two paths are not in sync. This derived_gram.y step is required for every ecpg build, so it must not depend on the optional PL/Python interpreter.

Follow the established precedent in contrib/unaccent/Makefile, which guards this exact case ("Allow running this even without --with-python") by falling back to a plain python:

ifeq ($(PYTHON),)
PYTHON = python
endif

(Or otherwise ensure a Python 3 interpreter is unconditionally available.)

---

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

High confidence. derived_gram.y is a new build artifact generated by the rule above, but it is not removed by clean/distclean (nor added to this directory's .gitignore). PostgreSQL requires clean/distclean to remove every generated file. Add it here:

	rm -f preproc.y derived_gram.y preproc.c preproc.h c_kwlist_d.h ecpg_kwlist_d.h

and add /derived_gram.y to .gitignore.

💡 Suggested change

Before:

	rm -f preproc.y preproc.c preproc.h c_kwlist_d.h ecpg_kwlist_d.h

After:

	rm -f preproc.y derived_gram.y preproc.c preproc.h c_kwlist_d.h ecpg_kwlist_d.h

---

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

base_yy_drain is called after every push, but nothing in the tree emits a function by that name. The Lime driver templates in lime_convert_gram.py (and the generated backend gram.lime) emit only base_yyAlloc/base_yyLoc/base_yyFree and no drain primitive; the only "drain" helpers that exist are the hand-written plpgsql_yy_drain_lookahead (pl_scanner.c) and pgpa_yy_drain. If ecpg's generated preproc.c does not actually define base_yy_drain, this file will fail to link. Confirm the exact symbol the ecpg Lime backend emits and use that name (or drop the call if the ecpg preproc.c is not built with the drain primitive).

---

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

Signature mismatch risk: the backend and both driver templates declare base_yyLoc with five parameters (..., YYLTYPE yyloc, core_yyscan_t yyscanner), and the generated push parser is what actually defines this symbol. Here it is declared with four parameters. If ecpg's generated preproc.c emits the 5-arg reentrant form (or any form differing from this hand-written extern), the mismatched declaration is undefined behavior on the ABI and will corrupt the pushed token/args at runtime. Verify the exact prototype ecpg's Lime output emits and match it exactly rather than re-declaring it by hand here.

---

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

Double token translation. pgc.c's scan_emit_cb_dispatch() already resolves self-characters to raw ASCII codes (e.g. out_code = (unsigned char) text[0] for RAW_CHAR/OP, and out_code = ':' for RAW_CHAR_COLON). Re-mapping those raw bytes here through ascii_to_lime_token() only makes sense if ecpg's generated preproc.c was built with the same Lime symbolic-id scheme (LPAREN/COLON/... ids > ASCII range) as the backend. The ecpg grammar has no ascii_to_lime_token of its own, and this mapping is copied from the backend driver. If the ecpg parser tables expect the raw ASCII code (as the pull-mode base_yylex returns), passing COLON/LPAREN/... instead will misparse or reject valid input. Confirm the token-id contract of ecpg's generated parser before applying this remap.

---

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

On the success path the parser object from base_yyAlloc(malloc) is freed only at the end via base_yyFree. ecpg's lexer/emit callbacks call mmfatal() on errors (e.g. out-of-memory, parse errors, missing include file), which exits the process; but any non-fatal longjmp/error unwinding past this loop would leak the parser. Since ecpg exits on mmfatal this is low-impact today, but note the allocation is unguarded against error paths that don't reach the trailing free.

---

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

Gratuitous whitespace/indentation churn on otherwise-unchanged lines. Only the Op->OP rename is functional; the re-indented case labels (extra tabs, e.g. case\t\tCSTRING:), the shifted base_yylloc = loc_strdup(...) line, and the added blank line before break will not survive pgindent and inflate the diff. Revert these to the original indentation and keep only the rename to preserve a minimal diff.

---

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

The trailing comment "renamed from Op in Phase 3 final" documents patch history/process rather than current behavior, which violates the comment-accuracy discipline (comments explain why the code is the way it is now, not how it got here). Drop the comment; the token is simply OP.

---

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

These direct system includes deviate from ECPG's private-header convention and are redundant. The sibling header preproc_extern.h obtains bool/size_t via project headers (common/keywords.h, type.h), and the only consumer of this header, pgc.c, already includes postgres_fe.h before pgc_internal.h. Directly pulling in <stdbool.h>/<stddef.h> here diverges from the subsystem's established include discipline (frontend code should rely on postgres_fe.h) and can be inconsistent with the tree's MSVC bool handling. Confidence: moderate.

---

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

The user and lex parameters are typed void *, erasing all compiler type checking. Because both arguments have the same type, a caller in pgc.lex that transposes them (emit(lex, ..., user)) would compile cleanly and fail only at runtime. PostgreSQL style prefers a concrete/forward-declared struct pointer (e.g. PgcLexer *lex) so the compiler catches argument-order/type mistakes at both call and definition sites. This applies to the other void *-parameter prototypes below as well (pgc_emit_string_token_for, pgc_emit_xdolq, pgc_emit_xd_close, pgc_emit_xdc, pgc_do_include, pgc_handle_pop, pgc_terminate). Confidence: moderate.

---

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

base_yyleng is write-only dead state. It is defined in pgc.c and only ever assigned (in set_yytext()); no code anywhere in the ecpg tree reads it. In the legacy flex scanner yyleng/base_yyleng was a flex-provided global that grammar actions could read, but the new Lime-based shim never consumes it. Exporting a symbol with no consumer is dead code (YAGNI). Confidence: high. Either drop this extern declaration (and the corresponding global/assignment in pgc.c), or wire up the actual reader if one is intended. (Note: the pgc.c change is out of scope for this file's review; flagging the header addition here.)

---

📄 src/pl/plpgsql/src/Makefile (L83-L84)

$(LIME) is an undefined make variable, so this recipe expands to -d. pl_gram.lime, which tries to execute -d. as a program and fails. There is no LIME variable in Makefile.global.in, configure, or anywhere in the tree (confirmed by search). Every other Makefile converted in this series invokes the literal command lime (e.g. src/backend/parser/Makefile, src/backend/bootstrap/Makefile, src/backend/utils/adt/Makefile, src/test/isolation/Makefile all use lime -d. $<). Use the same literal here for consistency and to make the recipe actually run. (high confidence)

💡 Suggested change

Before:

pl_gram.c: pl_gram.lime
	$(LIME) -d. $<

After:

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

---

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

The buffer-growth loop is not overflow-safe. len is a signed int (fed from cur_feed_token_len, a size_t, cast to int at the call site). For a pathologically long token, (size_t) len + 1 can be huge and the newsz *= 2 loop can wrap newsz to 0, producing an infinite loop or an undersized allocation that the subsequent memcpy overflows. Add an explicit bound check (e.g. reject/clamp len before doubling, or use pg_nextpower2_size_t with an upper-bound guard). This is driver code newly written for the shim, not inherited from the legacy scanner, so it is worth hardening.

---

📄 src/interfaces/ecpg/preproc/pgc.c (L1476-L1480)

When the Lime lexer neither consumes bytes nor emits a token, this branch silently discards the rest of the current buffer (pos = len) and continues. Any un-tokenizable trailing bytes are dropped without a diagnostic, which can mask a real scan failure and produce incorrect ecpg output instead of a parse error. Consider emitting an mmfatal(PARSE_ERROR, ...) here (or asserting this state is unreachable) rather than silently swallowing input.

---

📄 src/interfaces/ecpg/preproc/pgc.c (L6-L7)

The file header comment is PR/process narrative ("Phase 5 ecpg pgc port", "Lime v0.2.2's lexer subsystem") rather than a description of what the code does now. pgsql-hackers convention is that comments describe current behavior, not the development history/tooling version. Recommend removing the phase/version references and keeping only the functional description of the shim.

---

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

pgindent style: missing space after the binary + operator (text +i should be text + i). Same issue occurs elsewhere in this file (e.g. text +1 in the PARAM/CVARIABLE emit cases). Run pgindent to normalize before submission.

💡 Suggested change

Before:

			slashstar = text +i;

After:

			slashstar = text + i;

---

📄 src/interfaces/ecpg/test/expected/preproc-define.c (L77-L77)

This expected-output change documents a behavioral regression in the rewritten ecpg scanner (src/interfaces/ecpg/preproc/pgc.c), not a legitimate output difference. In the source define.pgc the #if 0/#endif C-preprocessor directives sit at column 0 on their own lines, and reference ecpg echoes them verbatim at column 0 (as the deleted -#if 0 / -#endif lines here show, and as every other file in test/expected/ does — all #if/#endif there are at column 0). The new scanner instead merges the directive onto the trailing whitespace of the preceding echoed line and emits #if 0 / #endif with a leading space. This is the only expected file in the suite carrying that leading-space form, which confirms the pass-through echo/line-tracking in pgc.c changed rather than the test being updated for a real, intended change. #if/#endif with leading whitespace is still valid C, so this particular case compiles, but relaxing the expected file to match the new echo hides a fidelity regression: any pass-through directive that ends up sharing a line with echoed code would silently break the generated C. Fix the echo/whitespace handling in the scanner so pass-through lines are reproduced faithfully (directives at column 0 on their own line) and revert this expected file to the reference output. high confidence.

---

📄 src/pl/plpgsql/src/Makefile (L83-L84)

This new lime -d. invocation emits a pl_gram.out report file (as confirmed by the sibling conversions in src/backend/parser/Makefile, src/backend/utils/adt/Makefile, and src/test/isolation/Makefile, which all add <parser>.out to their clean rules). The clean distclean rule below still only removes pl_gram.c pl_gram.h ... and does not remove pl_gram.out, so this new artifact is leaked and never cleaned. Add pl_gram.out to the clean rule to match the sibling Makefiles. (moderate confidence)

---

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

These two functions are declared here and called from pl_scanner.c and the generated pl_gram.c (via pl_gram.lime, at lines 2505/2723/2730/... and by plpgsql_yy_drain_lookahead in pl_scanner.c), but no definition exists anywhere in the tree. A whole-tree search finds only these declarations and the call sites -- no function body for either symbol, in any .c file, in pglime, or emitted by lime_convert_gram.py. The converter comment claims they come from "Lime upstream a9706ad" as a prefix-renamed Parse_get_lookahead helper, but pglime contains no such helper and no Parse_*_lookahead symbol is present. The result is unresolved-symbol link failures for the plpgsql module (unlike plpgsql_lime_to_ascii_token, which the .lime epilogue actually defines). Either emit these definitions from the Lime template/generator, or add them to pl_scanner.c, before this can build. (high confidence)

---

📄 src/pl/plpgsql/src/pl_gram_types.h (L8-L12)

This sync instruction is stale and self-contradictory. pl_gram.y no longer exists (it was converted to pl_gram.lime), and pl_gram.lime contains no %union block -- the union body now lives only here and is pulled into the generated parser via #include "pl_gram_types.h". There is no "original %union from pl_gram.y" and no "both places" to keep in sync. Comments must describe current behavior; rewrite this to state that this header is the single source of truth for plpgsql's YYSTYPE and that the core_yystype/ival/str/keyword prefix must mirror core_YYSTYPE in parser/scanner.h. (high confidence)

---

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

These comments describe a transitional bison/Lime coexistence and private milestone names ("Phase 2j flips", "Once Phase 2j lands", "legacy build path before Phase 2j"). The Makefile in this same change already generates pl_gram.c exclusively via $(LIME) (no bison rule remains), so the "legacy bison-generated pl_gram.h" path described here is not live. Aspirational/future-tense comments and project-internal phase names are non-committable per PostgreSQL comment discipline -- comments must describe what the code does now. Either drop the phase references and describe the guard's actual purpose, or remove the guard if the bison path is truly gone. (high confidence)

---

📄 src/pl/plpgsql/src/pl_gram_types.h (L103-L105)

Same stale-comment issue: "Once Phase 2j flips to Lime" is a future-tense reference to a private milestone, and "bison-generated pl_gram.h (legacy build path)" no longer applies given the Lime-only Makefile rule in this change. Describe the guard's current role instead of the abandoned transitional state. (low confidence)

---

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

The in-function extern int plpgsql_lime_to_ascii_token(int t); is a convention violation. PostgreSQL declares prototypes in headers, not inside function bodies. This local extern bypasses cross-TU type checking: if the real definition's signature in pl_gram.lime's epilogue drifts, the compiler won't catch the mismatch. plpgsql_yy_drain_lookahead, plpgsql_yy_get_lookahead, and plpgsql_yy_clear_lookahead are all already prototyped in plpgsql.h -- plpgsql_lime_to_ascii_token should be declared there too, and this local extern removed. (moderate confidence)

---

📄 src/test/isolation/.gitignore (L6-L7)

The removal of /specscanner.c is correct (it is now a committed source file). But this same "Local generated source files" section is now incomplete after the Lime migration. lime -d. generates specparse.out in-tree (proven by the Makefile clean rule that now runs rm -f specparse.h specparse.c specparse.out), yet it is not ignored here, so it will appear as an untracked file after an in-tree build. Peer Lime-migrated directories already ignore this artifact (e.g. src/backend/parser/.gitignore has /gram.out, src/backend/replication/.gitignore has /syncrep_gram.out). Add /specparse.out. (high confidence)

💡 Suggested change

Before:

 /specparse.h
 /specparse.c

After:

 /specparse.h
 /specparse.c
 /specparse.out

---

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

Make-based build is broken for the scanner. specscanner.o is compiled from the committed specscanner.c, which does #include "specscanner_lex.h" and calls SpecLexAlloc/SpecLexFeedBytes/SpecLexErrorMessage/SpecLexFeedEOF/SpecLexFree. Those live in specscanner_lex.{c,h}, which meson generates from specscanner.lex (see meson.build spec_scanner_lex custom_target / lime_lex_cmd). This Makefile has no rule to generate specscanner_lex.c/specscanner_lex.h from specscanner.lex, so make will fail to compile specscanner.o (missing header) and, even past that, fail to link isolationtester (undefined SpecLex* symbols) because there is no specscanner_lex.o in OBJS. Add a Lime lexer-generation rule for specscanner.lex and add specscanner_lex.o to OBJS so the make and meson builds stay in sync. (high confidence)

---

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

The clean target no longer removes the generated scanner outputs. Since the scanner is now generated from specscanner.lex into specscanner_lex.c/specscanner_lex.h (see meson.build), make clean must remove those, otherwise stale generated files are left behind. Add specscanner_lex.c specscanner_lex.h here (and drop specscanner.c correctly, as done). (high confidence)

💡 Suggested change

Before:

	rm -f specparse.h specparse.c specparse.out

After:

	rm -f specparse.h specparse.c specparse.out specscanner_lex.c specscanner_lex.h

---

📄 src/test/isolation/specscanner.c (L142-L145)

This comment is factually wrong and describes a use-after-free that does not happen. In specscanner.lex, both the QIDENT close rule (qident_close) and the SQLBLK close rule (sqlblk_close) invoke emit(user, ..., s, n) BEFORE free(s). So text is still valid for the duration of this callback; it has NOT been freed "by the time we get here." The copy-out is correct, but the stated reason is misleading and will confuse future maintainers into believing text is always dangling. Fix the comment to state that text is valid only for the duration of the callback (the lexer frees its LEX_BUF_TAKE copy right after emit returns), which is why we must copy immediately.

---

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

spec_yyAlloc result is not checked for NULL, unlike SpecLexAlloc a few lines above. If allocation returns NULL (Lime's *yyAlloc can return NULL if its mallocProc fails), the subsequent spec_yy(ctx.parser, ...) calls dereference NULL and crash. Add a NULL check mirroring the lexer allocation check.

💡 Suggested change

Before:

	ctx.parser = spec_yyAlloc(spec_palloc);

After:

	ctx.parser = spec_yyAlloc(spec_palloc);
	if (ctx.parser == NULL)
	{
		fprintf(stderr, "could not allocate spec parser\n");
		exit(1);
	}

---

📄 src/test/isolation/specscanner.c (L169-L174)

INTEGER conversion via atoi over a fixed 32-byte buffer is a silent footgun: literals longer than 31 chars are silently truncated, and atoi performs no overflow detection (undefined behavior / silent wraparound on overflow). The only user of this token is the notices N count in a permutation blocker; a bad value would silently corrupt the parsed spec rather than error out. Prefer a checked conversion (e.g. strtol with range/errno checks, or the tree's strtoint helpers) and report an error via spec_yyerror on overflow/garbage.

---

📄 src/test/isolation/specscanner.c (L54-L56)

Aspirational/future-tense comment describing unshipped work ("When Lime upstream P0-NEW-12 lands and threads the %lexer_extra_argument binding through ... this can collapse into the per-instance extra struct"). Per project convention comments must describe current behavior, not planned future refactors. Trim this to describe only what the code does now (spec_yyline is a file-scope int reset per parse and incremented by the lexer's newline rules).

---

📄 src/test/modules/dummy_grammar_ext/dummy_grammar_ext.c (L123-L126)

Memory leak of the error string on the failure path. Per parser_extension.h, pg_grammar_ext_register() "sets *err to a palloc'd string describing the problem" on failure. Here err is used in the WARNING but never freed, and _PG_init() runs during shared_preload_libraries processing in a long-lived context, so the string leaks. Free it after logging (and NULL it).

Confidence: high.

💡 Suggested change

Before:

		ereport(WARNING,
				(errmsg("dummy_grammar_ext: register() failed: %s",
						err ? err : "(no error message)")));
		pg_grammar_ext_unregister(ext);

After:

		ereport(WARNING,
				(errmsg("dummy_grammar_ext: register() failed: %s",
						err ? err : "(no error message)")));
		pg_grammar_ext_unregister(ext);
		if (err)
		{
			pfree(err);
			err = NULL;
		}

---

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

The file header describes behavior that does not exist. It documents the removed Track A subprocess pipeline ("subprocess pipeline runs the first time raw_parser() is called", "$PGDATA/pg_parser_cache", "the cached .so dlopens", "dispatches through the dlopen'd base_yyparse"). parser_extension.c's own header states this path "has been removed entirely" and the live implementation is in-process compose (pg_grammar_ext_prewarm) with no subprocess, .so cache, or dlopen. Rewrite this header to describe the current in-process behavior.

Confidence: high.

---

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

This dummy_reduce header comment contradicts the function body and the file header. It says the callback is "wired but unreachable at runtime" and "the body is documentation", while the inner comment says "The trampoline now actually fires this callback" and the body does ereport a NOTICE. Under the live Track B implementation the callback IS dispatched (pg_grammar_ext_reduce_by_ruleno -> dispatch_reduce). Remove the stale "unreachable/Track B will exercise" prose.

Confidence: high.

---

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

This module is described as a smoke test whose "primary verification" is LOG/NOTICE output, but there is no TAP test (no t/001_*.pl) and meson.build has no tests += {...} block registering one. The module is compiled but never executed, so nothing greps for the NOTICE/LOG signals and the test would still "pass" with the feature reverted -- it exercises nothing. Sibling modules in this change (e.g. grammar_ext_compose/t/001_compose.pl) do ship TAP tests. Add a TAP test and register it in meson.build (and a Makefile if used).

Confidence: high.

---

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

This header comment contradicts the actual code below, which sets .expect_failure = false (and whose inline comment gives the opposite, correct rationale). Per the TAP test (t/001_compose.pl, Test 4), foxtrot's register() currently succeeds -- the token-name conflict is only caught later at in-process compile time (prewarm/first-parse), and the harness greps the server log for the collision directly rather than relying on the helper's expected-failure NOTICE. So expect_failure=false is the intentionally-correct value, and this header is stale. Rewrite the header to describe what the code does now: standalone foxtrot registers successfully (WARNING would fire if expect_failure were true), and the alpha+foxtrot conflict is asserted by the TAP test at compile time. Also note the ASCII artifact 'expect_-\nfailure=true' -- source must be ASCII with no hyphenated line-break splits inside identifiers.

💡 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.  Loaded
 * standalone, foxtrot's register() currently succeeds, so
 * expect_failure is false (a WARNING would fire if it were true).
 * The alpha+foxtrot collision is not detected at register() time;
 * it surfaces at in-process compile, and t/001_compose.pl greps the
 * server log for that error directly.

---

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

This comment is aspirational: it describes a symbol-table check that does not exist. Confirmed there is no undefined-RHS-symbol / symbol-table validation in the registration path (register_compose_extension in compose_ext_helpers.h consumes the spec synchronously and calls pg_grammar_ext_register; parser_extension.c has no such check). Per PostgreSQL comment discipline, comments must describe current behavior, not a hypothetical future addition. Reword to state that an unknown-token reference surfaces at lime-rebuild time (undefined RHS symbol), dropping the "if we add the symbol-table check" clause. Confidence: high.

💡 Suggested change

Before:

	 * Rule references K_GRAMMAR_ALPHA which alpha must have declared first.
	 * If alpha isn't loaded, this rule's reference to an unknown token will
	 * surface either at register() (if we add the symbol-table check) or at
	 * lime-rebuild time (lime errors on undefined RHS symbol).

After:

	 * Rule references K_GRAMMAR_ALPHA which alpha must have declared first.
	 * If alpha isn't loaded, this rule's reference to an unknown token
	 * surfaces at lime-rebuild time (lime errors on undefined RHS symbol).

---

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

The header claims "reversed order should produce a clear error", but this negative/error path is never exercised. In t/001_compose.pl, Test 5 (alpha+golf) only asserts the happy path (correct load order); there is no test that loads golf before alpha, or golf without alpha, and asserts the expected error. A claim about error behavior that no test covers is unverified and can silently regress. Either add a TAP case exercising the reversed/missing-alpha ordering with an explicit error assertion, or soften the comment to not assert behavior that isn't tested. Confidence: high.

---

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

Formatting inconsistency (pgindent). The other four typedefs in this header close with a single space (} ComposeToken;, } ComposeRule;, etc.), but ComposeType uses tab alignment here and in its two usages (const ComposeType *types; on line 65 and inside register_compose_extension). This is the classic symptom of pgindent not knowing ComposeType is a type name (missing from its typedef list), causing it to right-align the identifier as if it were a variable. Add ComposeType to typedefs.list (or re-run pgindent with the module's local typedef list) so the whole tree formats consistently and git diff --check stays clean. Confidence: high.

💡 Suggested change

Before:

}			ComposeType;

After:

} ComposeType;

---

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

This comment documents a standalone-hotel scenario that the test suite never exercises. In t/001_compose.pl, the only hotel case (Test 6) loads alpha+hotel, where K_GRAMMAR_BRAVO always exists. There is no standalone-hotel test, so the described "symbol Lime doesn't know yet" path is untested. Worse, the comment admits the outcome is undetermined ("the rebuild either resolves them later or errors at compile time -- either way"). A comment must describe the single, deterministic behavior the code actually produces, not two mutually-exclusive possibilities for an untested path. Remove the speculative standalone-load narrative and describe only what the alpha+hotel case does. (high confidence)

---

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

PostgreSQL sources are ASCII-only and comments must state current, deterministic behavior. The -- here is used as an em-dash-style clause separator paired with the hedge "either resolves them later or errors at compile time -- either way", which documents an undetermined outcome. Replace with a plain description of the single behavior the alpha+hotel test asserts. (moderate confidence)

---

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

This test is a tautology and will pass even if conflict detection regresses. Compose runs at prewarm during postmaster startup (miscinit.c process_shared_preload_libraries -> pg_grammar_ext_prewarm -> ereport(FATAL)), not at first parse. The foxtrot token-name conflict surfaces inside the in-process compose, so the postmaster FATALs at startup and $started is false. That drops execution into the else branch below, which unconditionally calls pass() without ever grepping the log to confirm the conflict was the cause. Meanwhile the meaningful like() assertion is masked by local $TODO. Net effect: this subtest passes whether or not the conflict is detected, and contradicts the file header's claim that "foxtrot's register() fails with a clear error". The else branch must slurp the log and assert the conflict message (e.g. K_GRAMMAR_ALPHA / lexeme mismatch), not blindly pass().

---

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

Stale comment: compose does not fire "on first parse" for a shared_preload_libraries extension. It runs in the postmaster at prewarm (pg_grammar_ext_prewarm, FATAL on failure) before any backend forks, so a compose error aborts startup here, not at the SELECT 1 below. Also $log_after_start is assigned but never read (dead variable). Fix the comment to describe prewarm-time failure and drop the unused variable.

---

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

Internal tracker references ("lime-letter-34", "lime-letter-35 Q1 / v1.8.1") are meaningless to pgsql-hackers reviewers and should not ship in a patch. Replace with a public thread Message-Id or drop them; a TAP test asserting behavior that the code does not yet implement (register()-time validation) should either test the actual shipped behavior without TODO scaffolding or be removed until the feature lands.

---

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

log_text() reads the server log with a raw open()/die while the postmaster is still running (Tests 1,2,3,5,6,7 call it before stop()), which can race with buffered log writes and misses log rotation. The file is already inconsistent: Test 9 uses slurp_file($node->logfile). Prefer the existing PostgreSQL::Test helpers ($node->log_contains / slurp_file, and stop() before grepping) so the assertions are not flaky under CI.

---

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

This test has no missing-prerequisite guard. If the five grammar_ext_overlap_* shared modules are not built/installed, start_with() sets shared_preload_libraries to nonexistent libraries and $node->start makes the postmaster fail to start, aborting the entire test run instead of skipping cleanly. Sibling TAP tests in this tree (e.g. oauth_validator, test_autovacuum) use plan skip_all => '...' when their prerequisite isn't present. Add an equivalent guard (skip cleanly when the feature/modules are unavailable) before the first start_with. (moderate confidence)

---

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

log_text() reinvents an existing helper. PostgreSQL::Test::Utils::slurp_file (already imported via PostgreSQL::Test::Utils) reads a whole file and is the established pattern for slurping log content in TAP tests. Replace this hand-rolled open/local $//close with slurp_file($node->logfile) per the DRY/reuse discipline. (high confidence)

💡 Suggested change

Before:

sub log_text
+{
+	my ($node) = @_;
+	my $logfile = $node->logfile;
+	open(my $fh, '<', $logfile) or die "cannot read $logfile: $!";
+	local $/;
+	my $text = <$fh>;
+	close $fh;
+	return $text;
+}

After:

sub log_text
{
	my ($node) = @_;
	return slurp_file($node->logfile);
}

---

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

This multi-statement script assumes every statement executes even if an earlier one errors, but with on_error_die => 0 a mid-script error is silently swallowed and the psql return code is never checked. If, say, pivot; aborts the session, the later SELECT 2/3/4 never run yet the \b2\b/\b3\b/\b4\b matches on $stdout could still pass on unrelated output (e.g. counts, timestamps) — a false pass. Check the psql return code, or run each statement separately so a failure is actually caught. (moderate confidence)

---

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

qr/syntax error/ is locale-sensitive: PostgreSQL error messages are translated, so under a non-English lc_messages this substring will not appear and the assertion fails. The user rules require locale-independent, portable expected output. Pin the server/session to C messages (e.g. set lc_messages = 'C' in start_with's appended conf, or match the SQLSTATE via psql -v ON_ERROR_STOP / \errverbose) rather than the localized text. The preceding comment also hedges ("syntax error OR column does not exist"), yet only "syntax error" is asserted, so the match may be wrong for the actual IDENT parse path. (high confidence)

---

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

Memory leak of err on the success path. The in-tree sibling that calls this same API family (src/backend/parser/parser_pushparse.c) frees err on every path -- it calls if (err) free(err); after a successful compile too, not only on failure. Here err is only freed in the else branch, so if lime_compile_grammar_in_process() sets a non-NULL err alongside a successful rc == 0 && snap != NULL result (e.g. a diagnostic/warning string), that library-allocated (non-palloc) buffer leaks and MemoryContext teardown will not reclaim it. Free err unconditionally after consuming rc/snap.

💡 Suggested change

Before:

		lime_snapshot_release(snap);
	}

After:

		lime_snapshot_release(snap);
	}
	if (err)
		free(err);

---

📄 src/test/modules/lime_in_process_smoke/lime_in_process_smoke.c (L69-L70)

len is derived from strlen() on the NUL-terminated cstring, which silently truncates the grammar if the input text contains an embedded NUL byte. If the API treats len as the authoritative byte length, this is a silent-truncation footgun. The in-tree caller in parser_pushparse.c passes the true byte length ((size_t) merged.len). Prefer the exact payload length via VARSIZE_ANY_EXHDR(grammar_text).

💡 Suggested change

Before:

	grammar = text_to_cstring(grammar_text);
	len = strlen(grammar);

After:

	grammar = text_to_cstring(grammar_text);
	len = (size_t) VARSIZE_ANY_EXHDR(grammar_text);

---

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

Comment hygiene (PostgreSQL standards): this references .agent/notes/track-b-phase2-design.md, which does not exist in the tree, and is written as forward-looking roadmap narrative ("foundation for Phase 4 Track B Phase 2", "before we attempt the much more invasive parser.c surgery"). In-tree comments should describe what the code does now and not point at nonexistent internal notes. Remove the roadmap prose and the dangling doc reference.

---

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

Stray hyphen artifact from a line wrap: lime_compile_grammar_- text should read lime_compile_grammar_text. As written it garbles the API name being referenced.

---

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

Comment/code drift (high confidence). This entire block describes lime_compile_grammar_text, LIME_TEMPLATE/LIME_SNAPSHOT_BUILD_C env vars, and claims the call "returns a structured error rather than a snapshot." But the module under test (lime_in_process_smoke.c) actually calls lime_compile_grammar_in_process() and this test's first assertion (below) expects a successful ok: snapshot built result. The comment contradicts both the code path exercised and the positive assertion. On upstream review this reads as stale/aspirational prose describing a different function. Additionally, the references to internal planning artifacts (Phase 4 Track B Phase 2, .agent/notes/track-b-phase2-design.md) and future-tense claims (will be updated, options (a)/(b)/(c)) do not belong in an upstream PostgreSQL patch — comments must describe current behavior. Trim this to a short factual note about what the test asserts.

---

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

Non-portable pointer format assertion (high confidence). The module prints the snapshot pointer with %p (lime_in_process_smoke.c:77). The 0x[0-9a-f]+ regex assumes glibc's %p formatting (lowercase hex with a 0x prefix). On MSVC/Windows %p yields uppercase hex with no 0x prefix (e.g. 00007FF6...), and other libc implementations vary. This assertion will fail on Windows even when the code works. Match the pointer part loosely or don't assert its format at all.

💡 Suggested change

Before:

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

After:

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

---

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

Weak error assertion (medium confidence). rc=-?\d+ accepts rc=0, which is the success code (the module emits error: only when rc != 0 || snap == NULL), and msg= matches an empty message. A regression that returns e.g. rc=0 with an empty message would still pass. Require a non-zero rc and a non-empty message so the test actually catches regressions on the error path.

💡 Suggested change

Before:

like($bad_result, qr/^error: rc=-?\d+ msg=/,

After:

like($bad_result, qr/^error: rc=-?[1-9]\d* msg=\S/,

---

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

Dollar-quoting footgun (medium confidence). The grammar body is interpolated inside $$...$$ dollar quotes. Lime/lemon action blocks commonly use $$/$1 semantic-value markers; the moment any grammar under test contains the literal sequence $$, the dollar quote terminates early and the SELECT breaks with a confusing syntax error. Use a uniquely tagged dollar-quote (e.g. $grammar$...$grammar$) or pass the grammar via a psql variable/bind parameter to make the harness robust as grammars evolve.

---

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

This execl -> execlp change appears to be a no-op with no stated purpose, and slightly weakens the invocation. shellprog comes from SHELLPROG, which is an absolute path in every current build config ($(SHELL) in GNUmakefile/ecpg, hardcoded /bin/sh in meson.build). execlp only consults $PATH when the program name contains no /, so for an absolute path it behaves identically to execl. Thus this hunk changes nothing today.

The only situation where it would differ is if SHELLPROG were ever a bare name (no slash), in which case execlp would resolve the shell via $PATH -- a subtle change of behavior (and a mild footgun in a test harness) that is not justified anywhere in the change. Per minimal-diff discipline: either drop this hunk, or if there's a real motivation (e.g. supporting a non-absolute SHELL), state it and add coverage. As posted, it's an unmotivated change with no accompanying test or rationale. (moderate confidence)

💡 Suggested change

Before:

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

After:

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

---

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

Non-portable timing: clock_gettime(CLOCK_MONOTONIC) (and struct timespec) is not portable to Windows/MSVC and bypasses PostgreSQL's canonical portable timing abstraction in portability/instr_time.h, which is the only accepted in-tree pattern for interval timing (the only direct clock_gettime uses in the tree are inside instr_time.c itself and uuid.c's CLOCK_REALTIME timestamp, not interval measurement). This will fail to build on Windows and on platforms lacking CLOCK_MONOTONIC. Use instr_time: INSTR_TIME_SET_CURRENT(t0), ..., INSTR_TIME_SUBTRACT(t1, t0), then INSTR_TIME_GET_NANOSEC(t1) (returns int64 ns). Confidence: high.

💡 Suggested change

Before:

	struct timespec t0,
				t1;

After:

	instr_time	t0,
				t1;

---

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

The timed loop runs iterations (up to INT32_MAX) parses with no CHECK_FOR_INTERRUPTS(), so a large iterations or an expensive query makes the backend uninterruptible -- the query cannot be cancelled and a shutdown/SIGTERM will be ignored until the loop finishes. miscadmin.h is already included; add CHECK_FOR_INTERRUPTS() inside the loop. Note this adds a (negligible) per-iteration cost to the measured region; place it after the reset or account for it. 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)

PG_FUNCTION_INFO_V1(parser_microbench) declares the C symbol, but the module ships no way to call it from SQL: the directory contains only this .c file and meson.build -- there is no .control/--1.0.sql extension file, no CREATE FUNCTION, and no t/*.pl TAP test exercising it. The header comment claims "Exposes one SQL function ... parser_microbench(query text, iterations int) RETURNS bigint", which is aspirational, not descriptive of what this change actually provides. A new SQL-callable code path with no test that would catch a regression is WIP, not commit-ready; add the SQL registration and a TAP/regression test covering the error path (iterations <= 0) and a normal call. Confidence: high.

---

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

The clock_gettime() return value is unchecked; on failure t0/t1 are uninitialized and ns_total becomes garbage. Switching to INSTR_TIME_SET_CURRENT() avoids this entirely. Confidence: moderate.

---

📄 src/test/modules/parser_microbench/parser_microbench.c (L8-L9)

Header comment describes behavior not present in this change. It states the module "Exposes one SQL function" and is "Loaded via shared_preload_libraries", but there is no SQL registration and no preload/_PG_init wiring in this module (unlike the sibling dummy_grammar_ext which has _PG_init). Comments must describe what the code does now, not the intended usage. Update the header to match the actual contents or add the missing wiring. Confidence: high.

---

📄 src/tools/lime_format (L39-L39)

Data-loss footgun: original is read here but never used for recovery. Combined with the in-place shutil.move(formatted_path, lime_path) below, there is no backup and no rollback path. If lime -F writes a corrupt .formatted, or the move is interrupted, the tracked source file is destroyed with no way to recover. original is currently only used for the equality comparison at the end, so the in-memory copy provides zero safety. Consider writing atomically and restoring original on any failure, or at minimum only overwriting after validating the formatted output.

---

📄 src/tools/lime_format (L7-L9)

Version-coupled, self-contradictory comment. This header asserts the formatter is "single-pass idempotent" and runs a single pass, but the meson.build wiring added in this same patch states the opposite: "Lime's formatter is not idempotent on its first pass for %left/%right/%nonassoc symbol order (stabilizes after pass 2)" and that "Two passes are run." One of these is wrong. Beyond the contradiction, the correctness of the single-pass assumption is coupled to an externally pinned version (flake.lock) with no runtime version check here, so if the pin changes the formatting can silently drift. Reconcile the two comments and, if single-pass really is required, enforce/verify the lime version at runtime rather than relying on a comment.

---

📄 src/tools/lime_format (L37-L37)

Over-broad skip matching. part.startswith(p) will exclude any path component that merely starts with one of the skip tokens, e.g. installation/, builder/, tmp_install_notes/. This can silently skip legitimate .lime files, leaving them unformatted while lime_format_check later flags them as drift (hard to diagnose). If the intent is to skip specific directory names, use exact matching instead.

💡 Suggested change

Before:

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

After:

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

---

📄 src/tools/lime_format (L39-L39)

Portability: read_text() (and the re-read below) uses the platform default encoding. On Windows/MSVC (a hard portability gate for PostgreSQL) this is not guaranteed to be UTF-8, which can produce spurious diffs against lime -F output or raise UnicodeDecodeError on .lime files with non-ASCII bytes, aborting the whole run. Pass an explicit encoding='utf-8' to read_text() (and to subprocess.run).

💡 Suggested change

Before:

    original = lime_path.read_text()

After:

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

---

📄 src/tools/lime_format_check (L11-L14)

This comment is factually wrong. flake.lock pins lime to v1.10.2 (see flake.nix ?ref=refs/tags/v1.10.2 and flake.lock "ref": "refs/tags/v1.10.2"), not v0.6.0. The whole paragraph about "Pre-v0.6.0 needed two passes ... we drop that workaround now since flake.lock pins v0.6.0" describes the wrong version and is stale/misleading. Comments must describe current behavior; drop the version-history narrative and, if single-pass idempotency must be documented, state it against the actually-pinned version. The companion src/tools/lime_format carries the same wrong claim.

---

📄 src/tools/lime_format_check (L47-L47)

Using part.startswith(p) prunes any path segment merely starting with one of these prefixes, not just the intended build/install directories. A legitimate .lime under e.g. build_helpers/ or installation/ would be silently skipped, and the checker would still report success (false negative that defeats the CI gate). Prefer exact-match against the segment: part in SKIP_PATTERNS. Note the companion src/tools/lime_format uses the identical logic, so both must stay consistent.

💡 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 (L52-L53)

Flattening the relative path via replace('/', '_') collapses distinct paths into one temp-dir name, e.g. a/b_c.lime and a_b/c.lime both become a_b_c.lime. shutil.copy then overwrites the first, so lime -F runs twice on one source and never checks the other -- a silent false negative for an enforcement gate. Since the temp dir is per-run, you can safely reproduce the tree structure instead: create flat = work / rel and flat.parent.mkdir(parents=True, exist_ok=True) before copying.

---

📄 src/tools/lime_format_check (L72-L73)

Path.read_text() performs universal-newline translation, so a .lime source with CRLF endings compares equal to LF-normalized formatter output, masking real byte-level formatting drift. Given this change set also adds Windows CI, the canonical-format guarantee should be enforced at the byte level. Read both sides as bytes (read_bytes()) or pass newline='' / explicit encoding to compare exactly what lime emits versus the on-disk source.

---

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

This shebang change is inconsistent with the rest of the tree. Every other Perl script in PostgreSQL (e.g. src/tools/copyright.pl, src/backend/catalog/genbki.pl, src/tools/mark_pgdllimport.pl, and ~25 others) uses #!/usr/bin/perl. Changing only this one script to #!/usr/bin/env perl breaks project convention and is unrelated to any functional purpose, so it will draw a reject on -hackers as a non-minimal, inconsistent diff. If a switch to env perl is genuinely wanted, it needs to be a separate, tree-wide patch discussed on the list. Revert this hunk.

💡 Suggested change

Before:

#!/usr/bin/env perl

After:

#!/usr/bin/perl

---

📄 src/tools/lime_lint (L65-L65)

Fragile failure detection: '0 error(s)' in out matches as a substring inside multi-digit counts such as '10 error(s)', '20 error(s)', '100 error(s)' — any count ending in 0. When lime exits 0 but reports e.g. 10 errors, error_count_zero is True and the file is treated as clean, silently defeating the lint gate. Anchor the match instead, e.g. parse the count with a regex like re.search(r'\b(\d+) error\(s\)', out) and compare int(m.group(1)) == 0, or match ' 0 error(s)' / start-anchor it.

---

📄 src/tools/lime_lint (L59-L59)

Non-ASCII character: the U+2713 check mark violates the project's ASCII-only source requirement (git diff --check / encoding portability). This literal is also matched as a string below. Since lime -L here is documented as v0.5.0+ (which emits 'OK: no diagnostics'), the pre-v0.5.0 shape appears unused and can be dropped along with its comment. If it must be kept, encode it as an escape (e.g. '\u2713 No errors or warnings') rather than a raw glyph.

---

📄 src/tools/lime_lint (L67-L67)

Non-ASCII character (U+2713) in a string literal violates the ASCII-only source requirement. Use an escape sequence such as '\u2713 No errors or warnings', or remove this pre-v0.5.0 branch entirely if that output shape is no longer produced.

---

📄 src/tools/lime_lint (L81-L81)

When no .lime files are discovered (wrong --srcdir, or all excluded by SKIP_PATTERNS), the script prints 'lime_lint: 0 files OK' and exits 0. A gate that passes without checking anything gives false coverage. Consider failing (or at least warning) when checked == 0.

---

📄 src/tools/lime_lint (L46-L46)

Prefix-based skip is overly broad: part.startswith(p) will also skip legitimately-named directories like builder, buildfarm, installer, or installation, silently excluding their .lime files. Use exact component matching, e.g. part in SKIP_PATTERNS, so only intended directories are skipped.

---

📄 src/tools/pglime (L111-L113)

These interdependent-argument checks fire only after Lime has already run and the base .c/.h have been moved into the meson-declared outputs. If --aot/--snapshot is passed without its --*-output companion, the script does the full generation, moves the base outputs, and only then exits non-zero, leaving the declared custom_target outputs partially populated. Validate the dependency up front (before running Lime), matching the fail-fast style of the sibling wrappers.

---

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

This local annotation contradicts the value actually stored and the surrounding annotations. rules.setdefault(lhs, []).append((rhs, prec)) (below) stores tuple[list[str], str | None], and both parse_lime's return type and emit_bison's rules parameter are annotated OrderedDict[str, list[tuple[list[str], str | None]]]. Fix the local annotation to match, otherwise a type checker (or a future refactor trusting it) will be misled.

💡 Suggested change

Before:

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

After:

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

---

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

The label regex \([A-Za-z]\) only accepts a single-letter label, but the forward converter (lime_convert_gram.py _label_for_index) emits labels of the form P{idx} (e.g. (P26)) once an RHS position exceeds 25. For such a rule the optional group matches nothing here, i is left at (P26), and on the next loop iteration P26 is misread as a spurious extra RHS symbol (and the same applies to the LHS label match above at line 334). This is latent today (no current .lime grammar produces such labels) but is a real parse/emit corruption if one ever does. Consider matching [A-Za-z][A-Za-z0-9]* to mirror the forward converter.

💡 Suggested change

Before:

            sm = re.match(r"([A-Za-z_][A-Za-z0-9_]*)\s*(?:\([A-Za-z]\))?",

After:

            sm = re.match(r"([A-Za-z_][A-Za-z0-9_]*)\s*(?:\([A-Za-z][A-Za-z0-9]*\))?",

[github-actions]

The default -Repository is hardcoded to a personal fork (gburd/postgres). For a change destined for the upstream PostgreSQL tree via pgsql-hackers/commitfest, embedding a specific contributor's fork as the default is dev-only scaffolding: anyone else running this silently downloads artifacts from the wrong repository. More broadly, this entire .github/scripts + .github/workflows Windows-deps infrastructure (the same gburd/postgres slug appears throughout .github/docs/* and workflow files) is an unrelated CI/tooling addition bundled with what is otherwise a parser/scanner refactoring series, violating the 'one thing per patch' rule. This is not commit-ready for upstream.

[github-actions]

The -Token parameter (default $env:GITHUB_TOKEN) is declared and documented in the usage header but never referenced in the script body. Authentication is delegated entirely to whatever context gh runs in, so $Token is dead/misleading: callers passing -Token will expect it to affect the download, but it has no effect. Either wire it into the gh invocations (e.g. via GH_TOKEN) or remove the parameter and its usage-line mention (YAGNI).

[github-actions]

gh run download is a native executable; with $ErrorActionPreference = 'Stop' a non-zero exit code is NOT treated as a terminating error, so a failed download falls through to Copy-Item, which then throws a confusing 'path not found' on postgresql-deps-bundle-win64\* instead of a clear diagnostic. Check $LASTEXITCODE after gh run download (as the companion workflow already does after curl.exe/tar) and fail fast with an actionable message.

[github-actions]

postInlineComments reads context.payload.pull_request.head.sha unconditionally, but the script explicitly supports workflow_dispatch (line 79 falls back to INPUT_PR_NUMBER/context.payload.inputs.pr_number). On a manual dispatch, context.payload.pull_request is undefined, so this throws a TypeError for every issue. The failure is swallowed by the surrounding try/catch and only logged as Failed to post inline comment, silently dropping all inline comments. Fetch the head SHA from the PR object retrieved via octokit.rest.pulls.get (e.g. pr.head.sha) and pass it through instead.

[github-actions]

config.model is reported in the summary even when config.provider === 'bedrock'. In that case the effective model is config.bedrock_model_id; the summary will show a misleading model string. Select the model based on the provider.

Suggested change

	summary += `---\n*Cost: $${totalCost.toFixed(2)} | Model: ${config.model}*\n`;
	const effectiveModel = config.provider === 'bedrock' ? config.bedrock_model_id : config.model;
	summary += `---\n*Cost: $${totalCost.toFixed(2)} | Model: ${effectiveModel}*\n`;

[github-actions]

COMMIT_RANGE is computed as base.sha..head.sha but checkout uses fetch-depth: 10. For PRs with more than 10 commits, or when base.sha is outside the shallow history, git rev-list/git diff-tree will fail or return incomplete results, producing incorrect pristine detection (skipping needed builds or building unnecessarily). Git-history-dependent logic should use fetch-depth: 0.

Suggested change

	- uses: actions/checkout@v4
	with:
	fetch-depth: 10 # Fetch enough commits to check recent changes
	- uses: actions/checkout@v4
	with:
	fetch-depth: 0 # Full history needed to resolve base..head commit range

[github-actions]

This workflow declares no permissions: key, so it inherits the repository-default token scope, which is broader than necessary — this workflow only needs contents: read. The sibling pg-ci.yml explicitly sets permissions: contents: read; follow that least-privilege convention here to reduce blast radius.

[github-actions]

ilammy/msvc-dev-cmd@v1 is a third-party action pinned only to a mutable tag rather than a full commit SHA — a supply-chain risk, since the tag can be repointed to malicious code. Pin third-party actions to a full commit SHA. Separately, actions/cache@v3 is inconsistent with the @v4 first-party actions used elsewhere in this file; upgrade to actions/cache@v4.

[github-actions]

The download steps verify only (Get-Item x).Length -gt N size heuristics. manifest.json already provides sha256 for each dependency (e.g. openssl 88525753...), but it is never used. A truncated-but-large or tampered archive would pass the size check — an integrity/supply-chain gap for security-sensitive libraries like OpenSSL. Verify the downloaded archive's SHA256 against the manifest.

[github-actions]

nmake test for OpenSSL uses continue-on-error: true because tests 'can be flaky on Windows'. Silently ignoring crypto test failures can ship a broken/insecure OpenSSL build. If specific tests are genuinely unreliable, disable those deterministically rather than swallowing all failures.