Skip to content

Implement our own version of normalize - #41

Open
sgrif wants to merge 1 commit into
mainfrom
sg-new-normalize
Open

Implement our own version of normalize#41
sgrif wants to merge 1 commit into
mainfrom
sg-new-normalize

Conversation

@sgrif

@sgrif sgrif commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

This introduces a reimplementation of normalize using our primitives instead of the implementation in libpg_query. In addition to reducing our reliance on that library, this implementation takes advantage of the fact that for our use case we will already have a parsed AST to pass in.

libpg_query takes a string, parses it, records the location of any constants, then copies and manipulates that string to replace the constants.

In contrast, this implementation just copies the AST, walks it, replaces any constants with parameters, and changes the numbers on any parameters it encounters. In an effort to do this change in a single iteration over the parse tree, we don't preserve any bind parameters that were in the original query, which you can see reflected in the test that was updated.

On an extremely large query, the new implementation is 4.5x faster. If you include deparse time it is about twice as fast, and including parse

  • deparse time it is roughly 10% slower. While it isn't ideal that the full parse + normalize + deparse flow is slightly slower, in pgdog we'd only be using the normalize + deparse flow, so this results in a significant performance win as well as a reduction in unsafe code.

@sgrif
sgrif requested review from levkk and meskill July 16, 2026 15:44
This introduces a reimplementation of normalize using our primitives
instead of the implementation in libpg_query. In addition to reducing
our reliance on that library, this implementation takes advantage of the
fact that for our use case we will already have a parsed AST to pass in.

libpg_query takes a string, parses it, records the location of any
constants, then copies and manipulates that string to replace the
constants.

In contrast, this implementation just copies the AST, walks it, replaces
any constants with parameters, and changes the numbers on any parameters
it encounters. In an effort to do this change in a single iteration over
the parse tree, we don't preserve any bind parameters that were in the
original query, which you can see reflected in the test that was
updated.

On an extremely large query, the new implementation is 4.5x faster. If
you include deparse time it is about twice as fast, and including parse
+ deparse time it is roughly 10% slower. While it isn't ideal that the
full parse + normalize + deparse flow is slightly slower, in pgdog we'd
only be using the normalize + deparse flow, so this results in a
significant performance win as well as a reduction in unsafe code.
@sgrif
sgrif force-pushed the sg-new-normalize branch from 2a2f23c to 5b4d7ab Compare July 16, 2026 15:47

@levkk levkk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 🔥

Comment thread Cargo.toml
[dev-dependencies]
criterion = "0.8.2"
pg_query = { version = "6.1.1", git = "https://github.com/pgdogdev/pg_query.rs.git" }
# pg_query = { version = "6.1.1", git = "https://github.com/pgdogdev/pg_query.rs.git" }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BABYE

@levkk

levkk commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Could you also include a normalize_str version? We will use it in EE because ppl can be running without sharding/LB (parser off) but we still need it to uniquely identify queries. 🙇

@sgrif

sgrif commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

Could you also include a normalize_str version? We will use it in EE because ppl can be running without sharding/LB (parser off) but we still need it to uniquely identify queries. 🙇

If you'd reviewed the code you would have seen that it's already there ;) https://github.com/pgdogdev/pg_raw_parse/pull/41/changes#diff-7d7ec379288d03f370e79b3210ce515b7e24f37e5e17248d1e9ce6fd557e1bfbR26

@levkk

levkk commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

@sgrif is this good to go? I think we would need this for EE when we rip out the old parser.

@sgrif

sgrif commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Shouldn't need this to rip out the old parser since normalize existed prior to this PR. I'd hold off while we've got other stuff in flight since this is strictly a performance regression unless the calling code uses the new API and not normalize_str

@sgrif

sgrif commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

It should be good to go it just needs changes on the pgdog side

@levkk

levkk commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Was just checking since pg_query won't be linked anymore, just need to be sure we have some implementation of normalize. If the pg_query one is still available through this crate, then we should be good / not blocked by this.

@sgrif

sgrif commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

libpg_query's impl is exposed, yes. pg_query is no longer linked anywhere

@levkk

levkk commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Excellent! We are rock solid then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants