Repository navigation
perf(postgres): read libpg_query's tree from its c nodes - #56
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
ℹ️ No critical issues. One gap in the fingerprint and one avoidable clone, both inline.
Reviewed changes
I reviewed the whole of the single commit, which replaces the protobuf round trip through pg_query::parse and pg_query::deparse with a reader and builder that work directly on libpg_query's C nodes.
- Generated converters:
engine/postgres/build.rsruns bindgen over libpg_query's node headers. It also scrapes libpg_query's out/read defs, their conds and its enum defs, plus prost'sprotobuf.rs, and from those it emitsraw_convert.rs(C →pb) andraw_build.rs(pb→ C). A field kind it doesn't know stops the build. - Reading (
raw::parse): callspg_query_raw_parseinside a memory context. ADepthcounter copies the patched_outNodebound and upb's message depth. Too-deep trees are now refused with the new text "parse tree is nested too deeply to read", andstack::too_deepstill matches it. - Deparsing (
raw::deparseandcsrc/deparse.c): the C nodes live in a zeroed arena owned by Rust, and libpg_query's deparser runs underPG_TRYwith default options, the same waypg_query_deparse_protobuf_optsdoes. I checked the pinnedpostgres_deparse.c: everylappend,pfreeandlist_freecall works on its ownDeparseStateand never on the input nodes. So nodes allocated by Rust are safe to hand it. Parsed:stack::parsenow returns the bare tree. The table listing throughpg_query::ParseResultonly happens on demand, throughParsed::listed().- Tests: every Postgres statement in the oracle is compared against
pg_query's tree and deparse output on every run. The same comparison over a whole corpus can be run on demand (REYLO_PG_CORPUS). - Artifacts: all three
engine.wasmfiles are rebuilt.
claude-opus-5-5 ｜ 𝕏
colinhacks
force-pushed
the
postgres-direct-tree
branch
2 times, most recently
from
October 23, 2026 03:50
daecc34 to
e137804
Compare
The engine had libpg_query encode each tree as protobuf for prost to decode, and the reverse to deparse. Converters generated from libpg_query's own outfuncs and readfuncs now read and build the C nodes directly: the same trees and deparsed text on all of Postgres 18's regression statements, 682 KB less engine, parsing twice as fast.
colinhacks
force-pushed
the
postgres-direct-tree
branch
from
October 23, 2026 05:19
e137804 to
61fb567
Compare
Contributor
There was a problem hiding this comment.
✅ No new issues found. Both earlier findings are fixed.
Reviewed changes
I reviewed 61fb567, which addresses the two findings from the previous review.
- Fingerprint covers
csrc/:engine/postgres/build.rsnow hashescsrcalong withbuild.rs,srcandvendor. The extrarerun-if-changed=csrcline is gone becauseemitalready prints one for each entry, andpackages/postgres/engine.wasmis rebuilt.tests/fingerprint.test.tspasses for all three artifacts. - Guard test: the new test
the_fingerprint_hashes_every_source_of_the_cratereads theemitlist frombuild.rsand fails if any top-level entry of the crate is missing from it. Dot-entries,Cargo.tomlandexamples/are skipped. The crate's top level today isCargo.toml,build.rs,csrc,examples,srcandvendor, so the test passes. - No clone in
Parsed::listed: it now takesselfand moves the tree intoParseResult::new. Its only caller,analyze.rs, doesn't useparsedafter that call.
claude-opus-5-5 ｜ 𝕏
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.
The Postgres engine no longer goes through protobuf to get libpg_query's parse tree, or to deparse one.
pg_query::parsehad libpg_query encode the C tree as protobuf, then prost decode it intopg_query::protobuf's structs;pg_query::deparseencoded those again for libpg_query to decode back into C nodes. The engine now reads the C nodes straight into the same structs, and builds C nodes from them for the deparser.packages/postgres/engine.wasmHow
engine/postgres/build.rsgenerates both converters from the files libpg_query generates its own protobuf encoder and decoder from:pg_query_outfuncs_defs.c,pg_query_readfuncs_defs.c, their_conds.cdispatch files andpg_query_enum_defs.c, matched againstpg_query's prost structs. Every node and field libpg_query encodes is read, the same way; a field kind the generator does not know stops the build. bindgen gives the C structs for each target. The few nodes libpg_query handles by hand (lists, the value nodes,A_Const) are written by hand insrc/raw.rs, as there.raw::parse):pg_query_raw_parsein a memory context, then the tree intopg_query::protobuf::ParseResult. The depth bound is counted exactly as before: past half ofPG_QUERY_PROTOBUF_MAX_DEPTHgeneric nodes, as_outNodecounts them, or a message that deep, as upb's encoder counts them, so every statement that parsed before parses and every one refused is refused.raw::deparse): the tree built into C nodes in memory the engine owns, thencsrc/deparse.c, which runs libpg_query's deparser with default options underPG_TRY, so an error never unwinds through Rust frames.stack::parsereturns aParsedholding the tree:pg_query::ParseResultalso walks the whole tree to list its tables and functions, which only the analysis's read and write sets use (Parsed::listed).Verified
src/test/regress/sqlofREL_18_STABLE, 49,975 statements) gives the same tree or the same refusal aspg_query::parse, and each tree deparses to the same text aspg_query::deparse. The test that checks it runs on demand over a corpus (REYLO_PG_CORPUS); one that always runs does the same over every Postgres statement of the oracle's cases.