Skip to content

feat: multi-shard LIMIT+OFFSET in-flight re-write in PREPARE+EXECUTE - #1504

Merged
jkaczman merged 3 commits into
mainfrom
jk-prepare-execute-offset-limit
Sep 10, 2026
Merged

feat: multi-shard LIMIT+OFFSET in-flight re-write in PREPARE+EXECUTE#1504
jkaczman merged 3 commits into
mainfrom
jk-prepare-execute-offset-limit

Conversation

@jkaczman

@jkaczman jkaczman commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Implements functionality that handles multi-shard queries that use LIMIT + OFFSET in PREPARE + EXECUTE to re-write the LIMIT and OFFSET values (LIMIT = LIMIT + OFFSET, OFFSET = 0) and update the Route to expect them later to handle the appropriate multi-shard response for the client.

Specifically,

  • When we first receive a PreparedStatement with a SELECT query that has a LIMIT + OFFSET, it re-writes any A_Const nodes (constant numbers) to be ParamRef nodes. This allows us the ability to always be able to modify the limit/offset parameters later on, in a helper method stemming off of OffsetPlan::apply_after_parser. This later-on modification (during execution) only happens if it's multi-shard.
  • When we get the matching ExecuteStatement, if the Client previously used A_Const nodes, which means we had to store them in cache, we append the relevant cached numbers to the parameters list for the ExecuteStatement (as the Client does not know we internally re-wrote the PreparedStatement in our cache). If they previously used ParamRef nodes, the (cached, cloned) OffsetPlan is updated to the newly passed in values to complete it, so that we can use it in the OffsetPlan::apply_after_parser helper method.
  • Added Option<OffsetPlan> to prepared statement. Did not modify CacheKey::Simple; I modified it to use the original query (before rewrite) to store in the cache, and store the rewritten query (if any) in the cache. This allows us to later (1) which parameter is offset / which is limit, as well as (2) if the client pre-specified A_Const nodes, we must save them in our cache, as they will not be given to us in the ExecuteStatement.

Added some integration tests to test various cases: both limit/offset being ParamRefs, one being a ParamRef, both being A_Const, switching up the ordering (LIMIT $2 OFFSET $1), adding in an unrelated ParamRef (WHERE id < $2)... used ORDER BY for determinism in ensuring that the response we got is accurate.

Integration test and unit test also checks to make sure that two Queries that are the same due to a re-write won't conflict in GlobalCache.

Fixes #1383.

@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.89796% with 15 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...router/parser/rewrite/statement/simple_prepared.rs 93.59% 13 Missing ⚠️
...gdog/src/frontend/prepared_statements/cache_key.rs 0.00% 1 Missing ⚠️
...frontend/router/parser/rewrite/statement/offset.rs 97.91% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@jkaczman
jkaczman force-pushed the jk-prepare-execute-offset-limit branch from cd95d6c to 0e03efa Compare September 7, 2026 00:01
Comment thread pgdog/src/frontend/router/parser/rewrite/statement/offset.rs
// Count the `ParamRef` nodes in the query, so that we know what number to start at
// if we need to add some more.
let mut param_refs_count: usize = 0;
pg_raw_parse::walk::walk(stmt_query.into(), |node| {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We do this already here:

let mut next_param = plan.params as i32 + 1;
and here
if let Some(parameterized_stmt) = parameterized_stmt {
walk::walk(parameterized_stmt, |node| {
if let Node::ParamRef(param) = node {
plan.params = plan.params.max(param.number as u16)
}
});
}

Not a big deal, but at some point in the near/far future, we should consolidate all of our walks into one. It's cheap but it's not free.

@levkk levkk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Very nice!

@jkaczman
jkaczman merged commit 0ae57ca into main Sep 10, 2026
29 checks passed
@jkaczman
jkaczman deleted the jk-prepare-execute-offset-limit branch September 10, 2026 23:48
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.

PREPARE and OFFSET doesn't work for cross-shard queries

2 participants