Add Go wrappers for PL/pgSQL expression and assignment raw-parse modes - #149
Open
dullkingsman wants to merge 5 commits into
Open
Add Go wrappers for PL/pgSQL expression and assignment raw-parse modes#149dullkingsman wants to merge 5 commits into
dullkingsman wants to merge 5 commits into
Conversation
pg_query_parse_protobuf_opts already supports RAW_PARSE_PLPGSQL_EXPR and RAW_PARSE_PLPGSQL_ASSIGN1/2/3 through the parser_options bitmask, but there was no Go entry point for either mode. ParsePlPgSqlExpr parses a bare PL/pgSQL expression fragment (a condition, RETURN value, RAISE param, etc.) rather than a full SQL statement. ParsePlPgSqlAssign1/2/3 parse a "target := expr" assignment statement for a single-part, two-part, or three-part dotted target respectively; the resulting PLAssignStmt carries the right-hand side as an ordinary *SelectStmt in its Val field, so it can be deparsed with the existing Deparse function. Both follow the same parser.go/pg_query.go split as the existing Parse/ParseToProtobuf pair.
…rammar) Bumps LIB_PG_QUERY_TAG and re-runs the existing update_source vendoring process against pganalyze/libpg_query's 18.0.0 release, regenerating parser/ and the protobuf bindings (pg_query.pb.go) from that source. Fixes two environment-only build issues in the Makefile's recipe (unrelated to the version bump itself, just surfaced while running it here): TMPDIR must be set for the libpg_query tarball download path, and the brace-expansion in the pg_query.h/postgres_deparse.h copy step requires bash, not the default /bin/sh (dash on this system). Fixes a real correctness bug in the vendored C source, found while verifying the new parser's output: dump_return/dump_return_next in parser/pg_query_json_plpgsql.c had retvarno's WRITE_INT_FIELD commented out, so PostgreSQL 18's new "simple variable RETURN" fast path (which leaves expr NULL and sets retvarno instead) produced a PL/pgSQL RETURN statement with no information about which variable was returned at all. Confirmed via a direct PG17-vs-PG18 comparison (checking out the prior commit in a worktree) that this is new to PG18, not pre-existing: two functions differing only in which already-declared variable they return (e.g. "RETURN a;" vs "RETURN b;") now hash identically without this fix. retvarno defaults to -1 (unset) and can legitimately be 0 (the first declared variable), so the fix checks ">= 0" rather than reusing this file's usual "!= 0" WRITE_INT_FIELD convention, which would have silently dropped that case too. Updates parse_test.go, split_test.go, fingerprint_test.go, and makefuncs.go's MakeNotNullConstraintNode to match legitimate new PG18 behavior: new A_Expr.rexpr_list_start/rexpr_list_end fields, new Constraint.is_enforced/initially_valid fields (defaulted true on a plain NOT NULL constraint, matching every other constraint kind's existing default), the PG18 version number in JSON/protobuf fixtures, a PL/pgSQL type-name normalization change (e.g. "pg_catalog.\"varchar\"" -> "varchar"), and a stmt_location change in SplitWithParser specifically (no longer includes leading whitespace before the first statement; SplitWithScanner's separate implementation is unaffected). Also makes the fingerprint.json test-fixture reader tolerant of a JSON5/JSONC-style trailing "// comment" now present in one line of libpg_query's own testdata/fingerprint.json, which Go's strict encoding/json rejected outright. TestFingerprint has 6 remaining failures (of 78 cases: 3 alias- invariance queries, one ON COMMIT DROP temp table, two MERGE statements) against upstream's own golden hashes in testdata/fingerprint.json. Left deliberately red and documented in a comment rather than adjusted or skipped: Fingerprint/FingerprintToUInt64 aren't used anywhere in dullkingsman/dpg, so there's no independent way to tell whether the new parser or the shipped fixture is wrong, and chasing it further means debugging PostgreSQL's own query-normalization internals for no benefit to that consumer. Full test suite otherwise green: go build ./... && go vet ./... && go test ./... (verified via `go test -run` targeting each of TestParse, TestParsePlPgSQL, TestSplit, TestScan, TestSummary, TestParseConcurrency, and the ParsePlPgSqlExpr/Assign family individually, plus the full `go test ./...` run).
Upgrade vendored libpg_query from 17-6.2.2 to 18.0.0 (PostgreSQL 18 grammar)
…dtype
dump_function's switch over each datum's dtype in
parser/pg_query_json_plpgsql.c had no case for PLPGSQL_DTYPE_PROMISE, only
VAR/ROW/REC/RECFIELD. Every PL/pgSQL trigger function unconditionally
compiles in 10 built-in TG_* variables (tg_name, tg_when, tg_level, tg_op,
tg_relid, tg_relname, tg_table_name, tg_table_schema, tg_nargs, tg_argv)
with dtype PROMISE, built via plpgsql_build_variable and then retagged
(pl_comp.c), regardless of whether the function body ever references them
by name.
Confirmed via a direct PG17-vs-PG18 comparison (checking out the
pre-upgrade commit in a worktree) that PG17 only added a TG_* datum to
the table lazily, on first reference by name, so this exact code path
was never exercised before: a plain trigger function referencing only
NEW/OLD never touched a PROMISE datum under the old parser. PG18 adds
all 10 unconditionally at compile time, so this is now the common case,
not an edge case.
The missing switch case fell to the default branch: an elog(WARNING)
and no output, while the enclosing loop had already written the
datum's opening '{' and unconditionally appends "}}," after the switch
returns regardless of what (if anything) was written inside — the net
effect for each PROMISE datum was literally "{}}," emitted into the
"datums" array, corrupting the JSON structure for the entire rest of
the array, not just that one entry. ParsePlPgSqlToJSON's output for
any trigger function was therefore invalid JSON, full stop.
Fixed by adding a PLPGSQL_DTYPE_PROMISE case that reuses dump_var (a
promise datum is memory-layout-identical to a regular PLpgSQL_var, just
dtype-tagged differently) and appends the promise type code afterward
via a new dump_promise wrapper, so the specific built-in is still
identifiable in the output rather than merely not crashing.
Added a regression case to parsePlPgSQLTests (TestParsePlPgSQL) using a
minimal trigger function, asserting the exact valid JSON output
including all 10 promise-typed datums.
Fix invalid JSON for every trigger function: unhandled PROMISE datum dtype
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds four small Go functions exposing raw-parse modes that already exist
as public C entry points in the vendored libpg_query, but were never
wrapped in Go:
ParsePlPgSqlExpr/parser.ParsePlPgSqlExprToProtobuf— parses a barePL/pgSQL expression fragment (a condition, a RETURN value, a RAISE
param, etc.), using
PG_QUERY_PARSE_PLPGSQL_EXPR.ParsePlPgSqlAssign1/2/3/parser.ParsePlPgSqlAssign{1,2,3}ToProtobuf— parses a PL/pgSQL assignment statement ("target := expr") for a one-,
two-, or three-part dotted target, using
PG_QUERY_PARSE_PLPGSQL_ASSIGN{1,2,3}.Both follow the exact same two-layer split as the existing
Parse/ParseToProtobufpair. No existing function changes at all — this is apure addition, and no grammar, deparser, or other C source changes are
included.
Why
ParsePlPgSqlToJSONalready records aparseModeon everyPLpgSQL_exprnode it emits, telling you exactly which raw-parse mode PostgreSQL itself
used to compile that fragment — but there's currently no way to act on
that information. If you want to independently re-parse one of those
fragments (to canonicalize it, fingerprint it, lint it, or otherwise
analyze it apart from the rest of the function body), you're stuck: a bare
expression like
a IS NULLisn't valid input toParse(it's not acomplete statement), and an assignment fragment like
n := a || bisn'teither, since
:=isn't SQL syntax at all outside PL/pgSQL. The C libraryalready solves this correctly —
pg_query_parse_protobuf_optsand thePG_QUERY_PARSE_PLPGSQL_EXPR/ASSIGN1/2/3constants are public, stable,and already used internally — the gap is purely that the Go bindings never
exposed them.
We ran into this concretely while trying to fix spurious diffs in a tool
that hashes PL/pgSQL function bodies for change detection: cosmetic
reformatting inside an individual expression (e.g.
a||bvsa || b)was indistinguishable from a real change, because there was no way to
re-parse and re-canonicalize that one fragment on its own. These four
functions closed that gap cleanly, using PostgreSQL's own parser modes for
the job rather than an approximation (e.g. wrapping fragments in a
synthetic
SELECT).Notes for reviewers
PLAssignStmt(the nodeParsePlPgSqlAssign1/2/3return) has nodeparse support anywhere in this library — that's out of scope for this
PR. Its
Valfield is already an ordinary*SelectStmt, so callersneeding to deparse the right-hand side can do so directly with the
existing
Deparse; the target (Name/Indirection) can bereconstructed via the library's own existing
MakeColumnRefNode. Happyto follow up with real
PLAssignStmtdeparse support as a separate PRif that's of interest.
plpgsql_raw_parse_test.gocover both success andexpected-failure cases (e.g.
ParsePlPgSqlExprcorrectly rejecting afull statement). Full existing test suite passes unchanged.