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.