-
Notifications
You must be signed in to change notification settings - Fork 254
fix: don't sum row counts across shards for EXECUTE on omnisharded tables #1179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,127 @@ | ||||||||||||||||||||||||||
| //! Routing for SQL-level `PREPARE` and `EXECUTE` statements. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| use tracing::warn; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| use crate::frontend::BufferedQuery; | ||||||||||||||||||||||||||
| use crate::net::Parse; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| use super::*; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| impl QueryParser { | ||||||||||||||||||||||||||
| /// Route a SQL-level `PREPARE` statement. | ||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||
| /// It's broadcast to all shards. The statement behind the name is | ||||||||||||||||||||||||||
| /// stored in the prepared statements cache, so `EXECUTE` can be | ||||||||||||||||||||||||||
| /// routed based on it. | ||||||||||||||||||||||||||
| pub(super) fn prepare_statement( | ||||||||||||||||||||||||||
| stmt: &nodes::PrepareStmt, | ||||||||||||||||||||||||||
| context: &mut QueryParserContext, | ||||||||||||||||||||||||||
| ) -> Result<Command, Error> { | ||||||||||||||||||||||||||
| if let Some(name) = stmt.name() | ||||||||||||||||||||||||||
| && let Some(prepared_statements) = | ||||||||||||||||||||||||||
| context.router_context.prepared_statements.as_deref_mut() | ||||||||||||||||||||||||||
| // First PREPARE wins: a duplicate fails on the server, | ||||||||||||||||||||||||||
| // which keeps the original statement. | ||||||||||||||||||||||||||
| && prepared_statements.name(name).is_none() | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| match pg_raw_parse::deparse(stmt.query()) { | ||||||||||||||||||||||||||
| Ok(query) => { | ||||||||||||||||||||||||||
| let mut parse = Parse::named(name, query.as_str()); | ||||||||||||||||||||||||||
| prepared_statements.insert(&mut parse); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| Err(err) => { | ||||||||||||||||||||||||||
| warn!("failed to record PREPARE statement: {}", err); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| context | ||||||||||||||||||||||||||
| .shards_calculator | ||||||||||||||||||||||||||
| .push(ShardWithPriority::new_table(Shard::All)); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Ok(Command::Query(Route::write( | ||||||||||||||||||||||||||
| context.shards_calculator.shard(), | ||||||||||||||||||||||||||
| ))) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// Route `EXECUTE <name>` of a server-side prepared statement. | ||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||
| /// `PREPARE` is broadcast to all shards, so `EXECUTE` is broadcast as | ||||||||||||||||||||||||||
| /// well. If the statement behind the name is a write that only touches | ||||||||||||||||||||||||||
| /// omnisharded tables, mark the route, so results are deduplicated | ||||||||||||||||||||||||||
| /// across shards instead of aggregated, e.g. `UPDATE <rows>` reports | ||||||||||||||||||||||||||
| /// the row count from one shard, not the sum of all of them. | ||||||||||||||||||||||||||
| pub(super) fn execute_prepared( | ||||||||||||||||||||||||||
| stmt: &nodes::ExecuteStmt, | ||||||||||||||||||||||||||
| context: &mut QueryParserContext, | ||||||||||||||||||||||||||
| ) -> Result<Command, Error> { | ||||||||||||||||||||||||||
| let omnisharded = Self::executed_statement_omnisharded(stmt, context); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let shard = if omnisharded { | ||||||||||||||||||||||||||
| ShardWithPriority::new_table_omni(Shard::All) | ||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||
| ShardWithPriority::new_table(Shard::All) | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| context.shards_calculator.push(shard); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Ok(Command::Query( | ||||||||||||||||||||||||||
| Route::write(context.shards_calculator.shard()).with_omnisharded(omnisharded), | ||||||||||||||||||||||||||
| )) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// Check if the statement behind an `EXECUTE` name is a write that | ||||||||||||||||||||||||||
| /// only touches omnisharded tables. | ||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||
| /// `PREPARE` accepts SELECT, INSERT, UPDATE, DELETE, MERGE and VALUES. | ||||||||||||||||||||||||||
| /// Only writes are flagged: `EXECUTE` always routes as a write, and the | ||||||||||||||||||||||||||
| /// omnisharded flag on a write requires full shard coverage, which | ||||||||||||||||||||||||||
| /// would reject shard directives on read-only statements. MERGE is | ||||||||||||||||||||||||||
| /// left out conservatively; its row counts keep aggregating. | ||||||||||||||||||||||||||
| fn executed_statement_omnisharded( | ||||||||||||||||||||||||||
| stmt: &nodes::ExecuteStmt, | ||||||||||||||||||||||||||
| context: &mut QueryParserContext, | ||||||||||||||||||||||||||
| ) -> bool { | ||||||||||||||||||||||||||
| if context.sharding_schema.tables.omnishards().is_empty() { | ||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let Some(name) = stmt.name() else { | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be |
||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| let Some(prepared_statements) = context.router_context.prepared_statements.as_deref_mut() | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this returning |
||||||||||||||||||||||||||
| else { | ||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| let Some(parse) = prepared_statements.parse(name) else { | ||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // The statement cache parses each unique statement once, | ||||||||||||||||||||||||||
| // not on every EXECUTE. | ||||||||||||||||||||||||||
| let ast_context = AstContext { | ||||||||||||||||||||||||||
| sharding_schema: context.sharding_schema.clone(), | ||||||||||||||||||||||||||
| db_schema: context.router_context.schema.clone(), | ||||||||||||||||||||||||||
| user: context.router_context.cluster.user(), | ||||||||||||||||||||||||||
| search_path: context.router_context.parameter_hints.search_path, | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| let Ok(ast) = Cache::get().query( | ||||||||||||||||||||||||||
| &BufferedQuery::Prepared(parse), | ||||||||||||||||||||||||||
| &ast_context, | ||||||||||||||||||||||||||
| prepared_statements, | ||||||||||||||||||||||||||
| ) else { | ||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
Comment on lines
+107
to
+113
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be returning the error? Should we at least log it? |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let Some(root) = ast.ast.stmts().next() else { | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can also be |
||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| if !matches!( | ||||||||||||||||||||||||||
| root, | ||||||||||||||||||||||||||
| Node::InsertStmt(_) | Node::UpdateStmt(_) | Node::DeleteStmt(_) | ||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| StatementParser::new(root, None, &context.sharding_schema, None).is_all_omnisharded() | ||||||||||||||||||||||||||
|
Comment on lines
+118
to
+125
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| //! Routing tests for SQL-level `PREPARE`/`EXECUTE` statements. | ||
| //! | ||
| //! `EXECUTE` must be routed based on the statement behind the name. If that | ||
| //! statement is a write that only touches omnisharded tables, the results | ||
| //! are identical on all shards, so the response (e.g. `UPDATE <rows>`) must | ||
| //! be deduplicated across shards instead of aggregated. | ||
|
|
||
| use crate::frontend::router::parser::{Error, Shard}; | ||
|
|
||
| use super::setup::{QueryParserTest, *}; | ||
|
|
||
| #[test] | ||
| fn test_execute_omni_update_is_omnisharded() { | ||
| let mut test = QueryParserTest::new(); | ||
| test.execute(vec![ | ||
| Query::new("PREPARE upd AS UPDATE sharded_omni SET value = $1").into(), | ||
| ]); | ||
|
|
||
| let command = test.execute(vec![Query::new("EXECUTE upd('x')").into()]); | ||
|
|
||
| let route = command.route(); | ||
| assert!(route.is_write()); | ||
| assert_eq!(route.shard(), &Shard::All); | ||
| assert!( | ||
| route.is_omnisharded(), | ||
| "EXECUTE of an omnisharded UPDATE must carry the omnisharded flag, got {:?}", | ||
| route | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_execute_omni_delete_is_omnisharded() { | ||
| let mut test = QueryParserTest::new(); | ||
| test.execute(vec![ | ||
| Query::new("PREPARE del AS DELETE FROM sharded_omni WHERE id = $1").into(), | ||
| ]); | ||
|
|
||
| let command = test.execute(vec![Query::new("EXECUTE del(1)").into()]); | ||
|
|
||
| let route = command.route(); | ||
| assert!(route.is_write()); | ||
| assert_eq!(route.shard(), &Shard::All); | ||
| assert!( | ||
| route.is_omnisharded(), | ||
| "EXECUTE of an omnisharded DELETE must carry the omnisharded flag, got {:?}", | ||
| route | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_execute_omni_insert_is_omnisharded() { | ||
| let mut test = QueryParserTest::new(); | ||
| test.execute(vec![ | ||
| Query::new("PREPARE ins AS INSERT INTO sharded_omni (id, value) VALUES ($1, $2)").into(), | ||
| ]); | ||
|
|
||
| let command = test.execute(vec![Query::new("EXECUTE ins(1, 'a')").into()]); | ||
|
|
||
| let route = command.route(); | ||
| assert_eq!(route.shard(), &Shard::All); | ||
| assert!(route.is_omnisharded()); | ||
| } | ||
|
|
||
| /// Reads are not flagged: `EXECUTE` always routes as a write, and an | ||
| /// omnisharded write requires full shard coverage, which would reject | ||
| /// shard directives on statements that can't diverge the shards. | ||
| #[test] | ||
| fn test_execute_omni_select_not_omnisharded() { | ||
| let mut test = QueryParserTest::new(); | ||
| test.execute(vec![ | ||
| Query::new("PREPARE sel AS SELECT * FROM sharded_omni WHERE id = $1").into(), | ||
| ]); | ||
|
|
||
| let command = test.execute(vec![Query::new("EXECUTE sel(1)").into()]); | ||
|
|
||
| let route = command.route(); | ||
| assert_eq!(route.shard(), &Shard::All); | ||
| assert!(!route.is_omnisharded()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_execute_values_not_omnisharded() { | ||
| let mut test = QueryParserTest::new(); | ||
| test.execute(vec![Query::new("PREPARE vals AS VALUES (1), (2)").into()]); | ||
|
|
||
| let command = test.execute(vec![Query::new("EXECUTE vals").into()]); | ||
|
|
||
| assert!(!command.route().is_omnisharded()); | ||
| } | ||
|
|
||
| /// A shard directive on `EXECUTE` of a read-only statement is allowed; | ||
| /// the statement can't diverge the shards. | ||
| #[test] | ||
| fn test_execute_omni_select_with_shard_directive() { | ||
| let mut test = QueryParserTest::new(); | ||
| test.execute(vec![ | ||
| Query::new("PREPARE sel AS SELECT * FROM sharded_omni WHERE id = $1").into(), | ||
| ]); | ||
|
|
||
| let command = test.execute(vec![ | ||
| Query::new("/* pgdog_shard: 0 */ EXECUTE sel(1)").into(), | ||
| ]); | ||
|
|
||
| assert_eq!(command.route().shard(), &Shard::Direct(0)); | ||
| } | ||
|
|
||
| /// A shard directive on `EXECUTE` of an omnisharded write is rejected, | ||
| /// like on the equivalent direct statement: reaching only one shard | ||
| /// would silently diverge the table. | ||
| #[test] | ||
| fn test_execute_omni_write_with_shard_directive_rejected() { | ||
| let mut test = QueryParserTest::new(); | ||
| test.execute(vec![ | ||
| Query::new("PREPARE upd AS UPDATE sharded_omni SET value = $1").into(), | ||
| ]); | ||
|
|
||
| let result = test.try_execute(vec![ | ||
| Query::new("/* pgdog_shard: 0 */ EXECUTE upd('x')").into(), | ||
| ]); | ||
|
|
||
| assert!(matches!(result, Err(Error::OmniWriteWithDirective))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_prepare_routes_to_all_shards() { | ||
| let mut test = QueryParserTest::new(); | ||
| let command = test.execute(vec![ | ||
| Query::new("PREPARE upd AS UPDATE sharded_omni SET value = $1").into(), | ||
| ]); | ||
|
|
||
| let route = command.route(); | ||
| assert!(route.is_write()); | ||
| assert_eq!(route.shard(), &Shard::All); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_execute_sharded_table_not_omnisharded() { | ||
| let mut test = QueryParserTest::new(); | ||
| test.execute(vec![ | ||
| Query::new("PREPARE upd AS UPDATE sharded SET value = $1").into(), | ||
| ]); | ||
|
|
||
| let command = test.execute(vec![Query::new("EXECUTE upd('x')").into()]); | ||
|
|
||
| let route = command.route(); | ||
| assert!(route.is_write()); | ||
| assert_eq!(route.shard(), &Shard::All); | ||
| assert!(!route.is_omnisharded()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_execute_unknown_statement_not_omnisharded() { | ||
| let mut test = QueryParserTest::new(); | ||
| let command = test.execute(vec![Query::new("EXECUTE not_prepared(1)").into()]); | ||
|
|
||
| let route = command.route(); | ||
| assert!(route.is_write()); | ||
| assert_eq!(route.shard(), &Shard::All); | ||
| assert!(!route.is_omnisharded()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about setting this to
Default::default()innewand removing theOptionso callers can rely on this always being set?