Fix PG 18 pg_query_parse_plpgsql() regressions + add regression samples

[pyramation]

Thanks @lfittl ! Per your last comment, I work with my agent on bringing those tests cases into the repo... :)

Summary

This brings the three pg_query_parse_plpgsql() PG 18 regressions from pganalyze/libpg_query#337 into libpg_query's own output-based test suite (test/plpgsql_samples.sql → golden test/plpgsql_samples.expected.json), plus the minimal fixes required for those samples to emit valid, meaningful JSON.

This is a follow-up iteration on the earlier reference fix constructive-io/libpg_query#1 (fix/plpgsql-json-serialization), which fixed the bugs but did not add regression coverage. In this comment on #337 Lukas asked:

I do wonder if we should have parts of your tests also run in libpg_query, so we can find this during release updates.

So this iteration does exactly that — the repros now live in the harness that the maintainer regenerates on every Postgres version bump, so a re-break shows up as a golden diff. Two things also changed vs. #1: the namespace fix is applied in the mock (scripts/mocks/) so it survives extract_source, and retvarno is emitted only for real datums instead of unconditionally.

All three regressions were re-confirmed still-present on 18-latest before fixing (they were introduced by the PG 18 PL/pgSQL compilation rework — bc734d7 "Rework how PL/pgSQL compilation is done to rely on original code more" and ca4d024 "Rework PL/pgSQL parsing to use proper type definitions", against Postgres 18.4).

---

The three fixes

1. Trigger functions emitted malformed JSON — handle PLPGSQL_DTYPE_PROMISE datums

Cause. PG 18 represents the implicit trigger variables (tg_name, tg_when, tg_level, tg_op, tg_relid, …) as promise datums (PLPGSQL_DTYPE_PROMISE) instead of eagerly-populated vars. dump_function()'s datum switch had no case for that dtype, so each promise datum fell through to default:, which emits nothing — but the surrounding loop still appended the object braces, producing {} with a mismatched trailing } for each one (…,{}},{}},{}},…). That corrupts the entire datums array and makes the whole result fail JSON.parse.

Fix. Route promise datums to dump_var():

case PLPGSQL_DTYPE_VAR:
case PLPGSQL_DTYPE_PROMISE:   // added
    dump_var(out, (PLpgSQL_var *) d);
    break;

Why this is correct and safe. A promise datum is a PLpgSQL_var — Postgres stores both in the same struct. From the struct's own doc comment (src/pl/plpgsql/src/plpgsql.h):

Scalar variable — DTYPE_VAR and DTYPE_PROMISE datums both use this struct type. A PROMISE datum works exactly like a VAR datum for most purposes, but if it is read without having previously been assigned to, then a special "promised" value is computed and assigned to the datum before the read is performed.

So the cast (PLpgSQL_var *) d is exactly what upstream itself does. See:

• struct definition / comment: https://github.com/postgres/postgres/blob/REL_18_STABLE/src/pl/plpgsql/src/plpgsql.h (struct PLpgSQL_var, PLpgSQL_datum_type, PLpgSQL_promise_type)
• where these promise datums are created for triggers: plpgsql_compile in https://github.com/postgres/postgres/blob/REL_18_STABLE/src/pl/plpgsql/src/pl_comp.c (the tg_* vars are built, then var->dtype = PLPGSQL_DTYPE_PROMISE; var->promise = PLPGSQL_PROMISE_TG_*)
• upstream treats promise as var everywhere at runtime: plpgsql_fulfill_promise / the PLPGSQL_DTYPE_PROMISE cases in https://github.com/postgres/postgres/blob/REL_18_STABLE/src/pl/plpgsql/src/pl_exec.c

dump_var() only reads the leading PLpgSQL_var fields (refname, lineno, datatype, default_val, cursor fields) — all populated for promise datums — so there is no possibility of reading uninitialized/mismatched memory. It cannot affect non-trigger functions (no promise datums), and it strictly adds previously-missing cases to a switch whose other arms are untouched.

2. Schema-qualified types outside pg_catalog/public returned an error string instead of JSON

Cause. The PG 18 rework resolves a schema-qualified variable type (e.g. "my_schema".users) through LookupExplicitNamespace(). libpg_query has no real catalog, so it stubs that function with a mock; the mock elog(ERROR, …)'d for any schema other than pg_catalog/public, which aborts the whole parse and makes pg_query_parse_plpgsql() return the literal string Not implemented (LookupExplicitNamespace only supports pg_catalog and public) instead of JSON.

Fix (scripts/mocks/LookupExplicitNamespace.c, and the extracted src/postgres/src_backend_catalog_namespace.c to match):

if (strcmp(nspname, "pg_catalog") == 0)
    return PG_CATALOG_NAMESPACE;
return PG_PUBLIC_NAMESPACE;   // was: elog(ERROR, "Not implemented ...")

Why this is correct and safe. This does not invent a new code path — it funnels every non-catalog schema into the public path that libpg_query already supports. The caller is LookupTypeNameExtended() (scripts/mocks/LookupTypeNameExtended.c), which then calls GetSysCacheOid2(TYPENAMENSP, …, namespaceId). The GetSysCacheOid mock is already written to expect exactly this and to resolve unknown public-namespace types to RECORDOID (its own comment):

// scripts/mocks/GetSysCacheOid.c
if (IsCatalogNamespace(key2))         return pg_query_builtin_type_oid_by_name(...);  // pg_catalog: builtins
else if (key2 == PG_PUBLIC_NAMESPACE) return RECORDOID;                               // public: treat as row type
else                                  elog(ERROR, ...);                               // <- previously reachable

So the change is behavior-preserving for the cases that already worked, and only converts former hard-errors into the pre-existing public/RECORDOID behavior:

Input schema	Before	After
pg_catalog	PG_CATALOG_NAMESPACE → builtin type oid	unchanged
public	PG_PUBLIC_NAMESPACE → RECORDOID for unknown types	unchanged
any other (my_schema, …)	elog(ERROR), whole parse fails	PG_PUBLIC_NAMESPACE → RECORDOID, same as public

Empirically verified — public.users and "my_schema".users now produce byte-identical AST (PLpgSQL_rec → RECORDOID), and pg_catalog.int4 still resolves to the builtin int4:

=== public.users ===      {"PLpgSQL_rec":{"refname":"v","dno":1,"lineno":1}}
=== my_schema.users ===   {"PLpgSQL_rec":{"refname":"v","dno":1,"lineno":1}}   (identical)
=== pg_catalog.int4 ===   {"PLpgSQL_var":{"refname":"v",...,"typname":"int4"}}  (builtin, unchanged)

Because elog(ERROR) produced no JSON at all, broadening it cannot regress any input that previously parsed successfully — the only inputs whose behavior changes are ones that used to fail outright. In a parse-only context there is no real namespace to be "wrong" about; the mock exists solely to let type resolution proceed far enough to build the AST. Relevant upstream reference for the real function's contract: LookupExplicitNamespace / LookupTypeNameExtended in https://github.com/postgres/postgres/blob/REL_18_STABLE/src/backend/catalog/namespace.c and https://github.com/postgres/postgres/blob/REL_18_STABLE/src/backend/parser/parse_type.c.

3. RETURN <variable> dropped the return target

Cause. For a bare variable/record return (RETURN v, RETURN NEW), PG 18 records the target in PLpgSQL_stmt_return.retvarno rather than in expr. But retvarno was commented out in dump_return()/dump_return_next(), so the entire return payload disappeared from the JSON ({"PLpgSQL_stmt_return":{"lineno":4}}), making the source unreconstructable.

Fix.

WRITE_EXPR_FIELD(expr);
if (node->retvarno >= 0)                                   // -1 == "returns an expression"
    appendStringInfo(out, "\"retvarno\":%d,", node->retvarno);

Why this is correct and safe. retvarno is an int whose sentinel -1 means "this return uses expr, not a datum" (see PLpgSQL_stmt_return in https://github.com/postgres/postgres/blob/REL_18_STABLE/src/pl/plpgsql/src/plpgsql.h and how it is set in pl_gram.y / consumed in exec_stmt_return in https://github.com/postgres/postgres/blob/REL_18_STABLE/src/pl/plpgsql/src/pl_exec.c). The two representations are mutually exclusive: expression-returns keep expr and carry retvarno == -1; variable-returns carry a real datum index and no expr. Guarding on >= 0 therefore emits retvarno exactly for variable-returns (the ones that were losing data) and never adds a misleading retvarno":-1 to the many expression-returns. This is a deliberate refinement over PR #1, which uncommented WRITE_INT_FIELD(retvarno,…) unconditionally and thus stamped -1 onto every expression-return.

---

Test changes

• Adds three cases to test/plpgsql_samples.sql (trigger, schema-qualified type, RETURN <var>), matching the repros in pg_query_parse_plpgsql() regressions in 18.0.0 #337.
• Regenerates test/plpgsql_samples.expected.json via the existing test/parse_plpgsql harness.
• Because the golden file is regenerated, fix A_CONST and NULL/empty string #3 also adds retvarno to a handful of pre-existing samples that do RETURN <var> / RETURN ref — expected and correct, not a behavior change beyond the fix itself.

Note on the golden-file mechanism: a golden test only catches a regression if the committed baseline is correct — a snapshot of malformed JSON would simply lock the bug in. That's why the samples are paired with the fixes rather than added alone.

Verification

• make test (valgrind disabled locally) — all suites pass, including the plpgsql_samples diff.
• Each of the three sample outputs is valid JSON.parse-able JSON (was: throw / error-string / lossy before).
• Namespace equivalence + pg_catalog builtins confirmed as shown above.

[pyramation]

FYI, I've been making more fixes, which I have been battle testing rigorously, potentially we should consider looking at the full diff together.

I can definitely say it's working on our 23k test cases and even found a few other issues, but was working across so many repos I decided to hold off on making new PRs until the bug squashing cooled down a bit, that said, here is my diff: https://github.com/pganalyze/libpg_query/compare/18-latest...constructive-io:libpg_query:18-constructive?expand=1

[evoludigit]

I hit Bug 1 downstream and arrived at the same PLPGSQL_DTYPE_PROMISE case before finding this PR. Here is some independent verification in case it helps the review:

• I ran every statement in test/sql/postgres_regress and test/sql/plpgsql_regress through 18-latest with and without this PR. That's 49,481 statements, 1,251 of which produce PL/pgSQL output.
  • On 18-latest, the 162 trigger and event-trigger functions among them produce invalid JSON.
  • With this PR, all 1,251 outputs are valid. The only differences are those functions' 1,476 tg_* datums, plus the new retvarno fields.
  • No statement that parsed before now errors.
• The promise datums come from plpgsql_build_variable(), which palloc0s a PLpgSQL_var, so every field dump_var() reads is initialized.
  • ASan/UBSan and MSan builds with assertions enabled are clean over the same statements.
• The branch is 4 commits behind 18-latest but merges cleanly, and make test passes on the result.

One related thing. test/plpgsql_samples.sql already contains three trigger functions, so test/plpgsql_samples.expected.json on 18-latest is itself not valid JSON. The byte-for-byte diff can't notice that, so regenerating the file for 18.0.0 locked the malformed output in:

$ python3 -c "import json; json.load(open('test/plpgsql_samples.expected.json'))"
json.decoder.JSONDecodeError: Expecting ',' delimiter: line 16 column 124 (char 23965)

This PR fixes the file, but a future regeneration could adopt a similar break just as quietly. I have a small test-only change that makes test/parse_plpgsql check that its output is structurally well-formed (brackets properly paired, strings terminated) before the diff: 18-latest...evoludigit:libpg_query:test/plpgsql-json-structure

• On 18-latest, it fails with ERROR: malformed JSON, unexpected '}' at offset 23988, which is the first stray brace.
• On top of this PR, make test passes.

I'm happy to open it as a separate PR once this lands, or feel free to fold it in here, whichever is easier for you.

For anyone working around Bug 1 downstream in the meantime: replacing {}} with {} is not safe. {"PLpgSQL_stmt_return":{}} is a legitimate {}}: it's how the implicit RETURN at the end of a body is serialised. So that replacement corrupts output that was valid, for example any RETURNS void function without an explicit RETURN.

[lfittl]

I feel what we do about retvarno and similar fields where the zero value is significant should be discussed separately (see my inline comment) - can you split that change out or adjust it to a plain WRITE_INT_FIELD for now?

The other two changes are good to go - sorry for the delay on this! I'd also be happy to see more PRs for other PL/pgSQL fixes you have queued up.

[lfittl]

Thinking through this more, I'm not a fan of this change having a special if (vs just WRITE_INT_FIELD), because it means we deviate with how the rest of the parse tree output is handled.

FWIW, I see the point that not emitting 0 is a bit confusing (really its a Protobuf-ism we're applying to JSON), and that -1 doesn't actually signal anything useful - but I'm not sure its the right trade-off to add code complexity here for an easier to read JSON output (and slightly easier logic for someone working with the JSON programmatically).

I think if we wanted to have those zeroes be emitted, we should instead always output zero values for JSON for both PL/pgSQL and regular parse trees. That's a bigger change that we should discuss separately, but I think its worth considering. If we did that, I was initially worried that this would break our protobuf-cpp compatibility test for regular parsetrees, but turns out that there is a flag for that to have it emit zeroes for default values - so we're good in that regard.

For context, I'd like us to move PL/pgSQL output to Protobuf+JSON in the future (along the lines of #323) and customization like this is inconvenient to maintain. It also looks like we have similar problems for another case, specifically new_varno on trigger functions, and possibly more - so we'd have to special case all of them, and keep that up going forward.