[pyramation]

Summary

Port of #10 (18-constructive) to 17-constructive. In PG 17's extracted sources the stack-depth code lives in src_backend_tcop_postgres.c, so pg_query_set_stack_limit() is added there; the rest is identical.

check_stack_depth() is dead code in libpg_query because set_stack_base() is never called (stack_base_ptr stays NULL), so deep recursion in the tree walkers overflows the C stack — a memory access out of bounds trap in the WebAssembly build. This PR:

• adds pg_query_set_stack_limit(size_t bytes) so an embedder can set the reference point and limit at each entry;
• calls check_stack_depth() at the top of _outNode (JSON + protobuf), _fingerprintNode, dump_stmt / dump_expr, deparseExpr (no-op for embedders that never call the setter);
• runs pg_query_nodes_to_json in pg_query_parse_opts under PG_TRY so the error is returned as result.error instead of being unhandled.

make build, make test pass.

Also: free the PL/pgSQL alias list (pre-existing valgrind failure)

The valgrind CI jobs were already red before this PR (same still-reachable block from pg_query_plpgsql_record_alias on the branch base; proven on an unpatched 18-constructive worktree, same code here): pg_query_plpgsql_reset_aliases() freed the names but never the array, and nothing called it after the last compile. Second commit: reset frees the array too and pg_query_parse_plpgsql() resets at the end. make test VALGRIND=1 passes locally (exit 0).

Review follow-up: stack-depth errors no longer exit the process

With a stack limit set, three paths call check_stack_depth() outside any PG_TRY, so a deep input ended in Terminating process due to FATAL error instead of an error result:

• pg_query_parse_protobuf_opts → pg_query_nodes_to_protobuf
• pg_query_fingerprint_with_opts → _fingerprintNode
• pg_query_parse_plpgsql → plpgsqlToJSON

Each now runs under PG_TRY/PG_CATCH and returns the error in result.error. Fingerprint XXH3 states are allocated from the memory context (palloc0, 64-byte aligned like XXH_alignedMalloc), so the error path does not leak them. pg_query_set_stack_limit() is now declared in pg_query.h.

New test/stack_depth (runs in make test and under valgrind). Before the fix all three cases exit with status 1 and Terminating process due to FATAL error; after it, all three return stack depth limit exceeded. make test and make test VALGRIND=1 pass locally.

Not changed: the review also asked for a guard on statement-level deparse recursion, and for validation of pg_query_set_stack_limit's argument. I could not build a failing test for either (the deep-deparse crash I hit was in protobuf unpacking, not the deparser), so they are left out.

Link to Devin session: https://app.devin.ai/sessions/bcd8b2f791164247acb7a659b481efc2
Open in Devin Desktop: https://app.devin.ai/desktop/session/bcd8b2f791164247acb7a659b481efc2?variant=devin
Requested by: @pyramation

[devin-ai-integration]

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

• Disable automatic comment, CI, and merge conflict monitoring

[tenki-reviewer]

Review complete. 🟠 3 high · 🟡 2 medium

💬 Inline comments (3)

• 🟠 PL/pgSQL JSON dumper's check_stack_depth throw has no handler — pg_query_json_plpgsql.c:143
• 🟡 pg_query_set_stack_limit lacks validation, declaration, per-thread docs — src_backend_tcop_postgres.c:832
• 🟡 Statement-level deparse recursion bypasses deparseExpr guard — postgres_deparse.c:787

📍 Findings outside the diff (2) — 🟠 2 high — defects on lines GitHub can't attach comments to

🟠 High — Protobuf serialization unwrapped against new stack-depth error · pg_query_parse.c:195 · unchanged line

// src/pg_query_parse.c
195		result.parse_tree = pg_query_nodes_to_protobuf(parsetree_and_error.tree);

The PR's new check_stack_depth() in the protobuf _outNode (src/pg_query_outfuncs_protobuf.c:250) raises ereport(ERROR) when recursion exceeds the configured limit, but its caller pg_query_parse_protobuf_opts invokes pg_query_nodes_to_protobuf with no PG_TRY (src/pg_query_parse.c:195). With no active exception handler, pg_re_throw promotes the error to FATAL and terminates the embedding process, and pg_query_exit_memory_context is skipped, leaking the memory context. The sibling JSON path in the same file got a PG_TRY/PG_CATCH in this PR, so protobuf callers crash where JSON callers get a normal error result.

---

🟠 High — Fingerprint paths lack PG_TRY around new check_stack_depth · pg_query_fingerprint.c:350–351 · unchanged line

// src/pg_query_fingerprint.c
350				_fingerprintNode(&ctx, parsetree_and_error.tree, NULL, NULL, 0);
351			}

_fingerprintNode now calls check_stack_depth() (src/pg_query_fingerprint.c:267), which ereport(ERROR)s when the depth limit is exceeded, but neither caller installs a handler: pg_query_fingerprint_node calls _fingerprintNode bare at src/pg_query_fingerprint.c:320, and pg_query_fingerprint_with_opts does the same at line 350. In pg_query_fingerprint_with_opts the longjmp skips pg_query_exit_memory_context, leaking the memory context; in pg_query_fingerprint_node the error escapes the library into embedder code with no PostgreSQL exception frame, aborting the process. This is inconsistent with the parse/deparse paths this PR updated, which all convert the error into PgQueryError via PG_CATCH.

---

The change wires PostgreSQL's stack-depth mechanism into the library: a new per-thread base pointer/limit setter is added, and every recursive serialization/deparse entry point gains a recursion guard. The guards throw ereport(ERROR), but only some call sites wrap the throw in PG_TRY/PG_CATCH and translate it into a PgQueryError.

Files	Change
src/postgres/src_backend_tcop_postgres.c	Adds pg_query_set_stack_limit() and thread-local stack-base/limit state, plus stack_is_too_deep() support.
src/pg_query_parse.c	Wraps the parse/JSON path in PG_TRY/PG_CATCH and returns a structured error on stack exhaustion.
src/pg_query_outfuncs_json.c, src/pg_query_outfuncs_protobuf.c, src/pg_query_fingerprint.c, src/pg_query_json_plpgsql.c	Insert check_stack_depth() guards into the recursive node walkers.
src/postgres_deparse.c	Adds the depth guard at deparseExpr for deparsing recursion.

_{Reviewed commit: 5691182}

[tenki-reviewer]

This PR adds a configurable stack-depth limit (pg_query_set_stack_limit) and check_stack_depth() guards to libpg_query's recursive walkers (parse, JSON, protobuf, fingerprint, plpgsql dump, deparse) to convert deep-nesting crashes into errors.

Key findings

• 🟠 Protobuf serialization unwrapped against new stack-depth error — pg_query_parse.c:195
• 🟠 Fingerprint paths lack PG_TRY around new check_stack_depth — pg_query_fingerprint.c:350
• 🟠 PL/pgSQL JSON dumper's check_stack_depth throw has no handler — pg_query_json_plpgsql.c:143
• 🟡 pg_query_set_stack_limit lacks validation, declaration, per-thread docs — src_backend_tcop_postgres.c:832
• 🟡 Statement-level deparse recursion bypasses deparseExpr guard — postgres_deparse.c:787

[tenki-reviewer]

🟠 bug · high

PL/pgSQL JSON dumper's check_stack_depth throw has no handler

dump_stmt and dump_expr in src/pg_query_json_plpgsql.c:143 now call check_stack_depth(), which throws an ereport(ERROR) when nesting exceeds the stack-depth limit. The caller chain pg_query_parse_plpgsql → plpgsqlToJSON (src/pg_query_parse_plpgsql.c:492) has no PG_TRY — the enclosing PG_TRY at line 345 only covers compilation and has already exited by the time the dump runs. With no active exception handler, pg_re_throw promotes the error to FATAL and exits the host process instead of returning a PgQueryError.

📋 Prompt for AI Agents

In src/pg_query_parse_plpgsql.c, inside the loop of pg_query_parse_plpgsql (around lines 487-508), wrap func_json = plpgsqlToJSON(func_and_error.func); in a PG_TRY/PG_CATCH. In the PG_CATCH, MemoryContextSwitchTo the outer context ctx, CopyErrorData, build a malloc'd PgQueryError with strdup'd message/filename/funcname and NULL context, assign it to result.error, and FlushErrorState — mirroring the existing PG_CATCH at lines 369-388 — then skip appending the function and break out of the loop. This is needed because dump_stmt and dump_expr in src/pg_query_json_plpgsql.c now call check_stack_depth(), which throws an ereport(ERROR); with no handler the error becomes FATAL and exits the process on deeply nested PL/pgSQL input.

[tenki-reviewer]

🟡 bug · medium

pg_query_set_stack_limit lacks validation, declaration, per-thread docs

The new exported pg_query_set_stack_limit accepts any size_t without validation (src/postgres/src_backend_tcop_postgres.c:832): with bytes == 0 it sets a non-NULL stack_base_ptr and a zero limit, so stack_is_too_deep() returns true immediately and every subsequent recursive call — even the shallowest query — fails with "stack depth limit exceeded"; with very large values, (int)(bytes / 1024) and (long) bytes overflow or wrap, silently disabling protection. The function is also not declared in the public header pg_query.h, so embedders have no ABI contract, and since all three state variables are __thread, the setting applies only to the calling thread — an embedder configuring from a setup thread gets silently different behavior on worker threads. Impact: an embedder passing an unvalidated config value gets spurious errors or silently no stack protection, with no documented contract to guide correct use.

📋 Prompt for AI Agents

1. In pg_query.h, add a declaration void pg_query_set_stack_limit(size_t bytes); near the other public entry points (around line 119), with a comment explaining that the stack base and limit are thread-local (__thread state in src/postgres/src_backend_tcop_postgres.c), so the function must be called on every thread that will run pg_query parsing, and that stack-depth checks remain inert until it is called. 2) In src/postgres/src_backend_tcop_postgres.c lines 832-839, guard the body: if bytes is below a safe minimum (e.g. 64 * 1024), either ignore the call or clamp to the existing default (100 * 1024) so that bytes = 0 cannot make stack_is_too_deep return true for every call, and clamp large values to (size_t)INT_MAX * 1024 so the int/long casts cannot overflow.

[tenki-reviewer]

🟡 bug · medium

Statement-level deparse recursion bypasses deparseExpr guard

The PR's check_stack_depth() guard was added only at deparseExpr (src/postgres_deparse.c:787), but statement-level recursion does not funnel through it: deparseSubLink calls deparseSelectStmt directly (src/postgres_deparse.c:3548), and nested subqueries via deparseRangeSubselect (line 4165), set-operation operands (lines 2849, 2874), and rule/view actions form recursion cycles that never cross the guarded frame. A parse tree with deeply nested subqueries can therefore overflow the real native stack and crash the process with SIGSEGV instead of raising the intended "stack depth limit exceeded" error.

📋 Prompt for AI Agents

In src/postgres_deparse.c, add check_stack_depth(); as the first statement of deparseSelectStmt and deparseStmt — the functions that close statement-level recursion cycles (deparseSubLink at line 3548, deparseRangeSubselect at line 4165, and set-operation recursion at lines 2849/2874 call deparseSelectStmt directly without passing through the deparseExpr guard at line 787). Verify other direct self-recursive cycles (e.g. deparseConstraint, deparseAlterTableCmd via deparseSchemaStmt) are covered by these two insertion points, and add guards to any cycle still not crossing a guarded frame.