From 1d82d6f50f1988a092c3b832398d9c830221644d Mon Sep 17 00:00:00 2001 From: mrl5 <31549762+mrl5@users.noreply.github.com> Date: Mon, 17 Jul 2023 18:25:38 +0200 Subject: [PATCH 1/5] fix(postgres): sqlx prepare fails if shared_preload_libraries=pg_stat_statements closes #2622 refs: * https://serde.rs/enum-representations.html#untagged * https://serde.rs/field-attrs.html#skip * https://www.postgresql.org/docs/current/pgstatstatements.html * https://www.postgresql.org/docs/current/runtime-config-statistics.html#GUC-COMPUTE-QUERY-ID --- sqlx-postgres/src/connection/describe.rs | 28 +++++++++++++++--------- tests/docker-compose.yml | 2 +- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/sqlx-postgres/src/connection/describe.rs b/sqlx-postgres/src/connection/describe.rs index bcfbf3a9cf..9375bce693 100644 --- a/sqlx-postgres/src/connection/describe.rs +++ b/sqlx-postgres/src/connection/describe.rs @@ -451,15 +451,11 @@ WHERE rngtypid = $1 let mut nullables = Vec::new(); - if let Explain::Plan( - plan @ Plan { - output: Some(outputs), - .. - }, - ) = &explain - { - nullables.resize(outputs.len(), None); - visit_plan(&plan, outputs, &mut nullables); + if let Explain::QueryPlan(query_plan @ QueryPlan { plan, .. }) = &explain { + if let Some(outputs) = &query_plan.plan.output { + nullables.resize(outputs.len(), None); + visit_plan(&plan, outputs, &mut nullables); + } } Ok(nullables) @@ -492,15 +488,27 @@ fn visit_plan(plan: &Plan, outputs: &[String], nullables: &mut Vec> } #[derive(serde::Deserialize)] +#[serde(untagged)] enum Explain { /// {"Plan": ...} -- returned for most statements - Plan(Plan), + QueryPlan(QueryPlan), /// The string "Utility Statement" -- returned for /// a CALL statement #[serde(rename = "Utility Statement")] UtilityStatement, } +#[derive(serde::Deserialize)] +struct QueryPlan { + #[serde(rename = "Plan")] + plan: Plan, + /// present when either pg_stat_statements is loaded and/or compute_query_id is enabled + /// https://www.postgresql.org/docs/current/pgstatstatements.html + /// https://www.postgresql.org/docs/current/runtime-config-statistics.html#GUC-COMPUTE-QUERY-ID + #[serde(rename = "Query Identifier", skip)] + _query_identifier: Option, +} + #[derive(serde::Deserialize)] struct Plan { #[serde(rename = "Join Type")] diff --git a/tests/docker-compose.yml b/tests/docker-compose.yml index f521efb3a4..d50e431a49 100644 --- a/tests/docker-compose.yml +++ b/tests/docker-compose.yml @@ -183,7 +183,7 @@ services: volumes: - "./postgres/setup.sql:/docker-entrypoint-initdb.d/setup.sql" command: > - -c ssl=on -c ssl_cert_file=/var/lib/postgresql/server.crt -c ssl_key_file=/var/lib/postgresql/server.key + -c ssl=on -c ssl_cert_file=/var/lib/postgresql/server.crt -c ssl_key_file=/var/lib/postgresql/server.key -c shared_preload_libraries=pg_stat_statements postgres_15_client_ssl: build: From f0696ea2a2b5eee723da7e39ff3ee3dcfd5cc091 Mon Sep 17 00:00:00 2001 From: mrl5 <31549762+mrl5@users.noreply.github.com> Date: Mon, 17 Jul 2023 21:48:24 +0200 Subject: [PATCH 2/5] fix(postgres): regression of #1449 ``` error: error occurred while decoding column 0: data did not match any variant of untagged enum Explain at line 3 column 1 Error: --> tests/postgres/macros.rs:103:15 | 103 | let row = sqlx::query!(r#"CALL forty_two(null)"#) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in the macro `$crate::sqlx_macros::expand_query` which comes from the expansion of the macro `sqlx::query` (in Nightly builds, run with -Z macro-backtrace for more info) error: could not compile `sqlx` (test "postgres-macros") due to previous error ``` --- sqlx-postgres/src/connection/describe.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sqlx-postgres/src/connection/describe.rs b/sqlx-postgres/src/connection/describe.rs index 9375bce693..eb0a6d0827 100644 --- a/sqlx-postgres/src/connection/describe.rs +++ b/sqlx-postgres/src/connection/describe.rs @@ -494,8 +494,7 @@ enum Explain { QueryPlan(QueryPlan), /// The string "Utility Statement" -- returned for /// a CALL statement - #[serde(rename = "Utility Statement")] - UtilityStatement, + UtilityStatement(String), } #[derive(serde::Deserialize)] From 9102d5d6984f96ce681ffc7abcba29426fcd5395 Mon Sep 17 00:00:00 2001 From: mrl5 <31549762+mrl5@users.noreply.github.com> Date: Thu, 20 Jul 2023 18:54:50 +0200 Subject: [PATCH 3/5] refactor(postgres): don't define unused fields in QueryPlan --- sqlx-postgres/src/connection/describe.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/sqlx-postgres/src/connection/describe.rs b/sqlx-postgres/src/connection/describe.rs index eb0a6d0827..53b47f8c8a 100644 --- a/sqlx-postgres/src/connection/describe.rs +++ b/sqlx-postgres/src/connection/describe.rs @@ -501,11 +501,6 @@ enum Explain { struct QueryPlan { #[serde(rename = "Plan")] plan: Plan, - /// present when either pg_stat_statements is loaded and/or compute_query_id is enabled - /// https://www.postgresql.org/docs/current/pgstatstatements.html - /// https://www.postgresql.org/docs/current/runtime-config-statistics.html#GUC-COMPUTE-QUERY-ID - #[serde(rename = "Query Identifier", skip)] - _query_identifier: Option, } #[derive(serde::Deserialize)] From 23a0fd35b7751c4577113fdb4ce5917763c87fe9 Mon Sep 17 00:00:00 2001 From: Austin Bonander Date: Mon, 31 Jul 2023 13:23:31 -0700 Subject: [PATCH 4/5] refactor(postgres): simplify query plan handling, add unit test --- sqlx-postgres/src/connection/describe.rs | 105 +++++++++++++++++++---- 1 file changed, 87 insertions(+), 18 deletions(-) diff --git a/sqlx-postgres/src/connection/describe.rs b/sqlx-postgres/src/connection/describe.rs index 53b47f8c8a..1cd28a8b54 100644 --- a/sqlx-postgres/src/connection/describe.rs +++ b/sqlx-postgres/src/connection/describe.rs @@ -451,11 +451,16 @@ WHERE rngtypid = $1 let mut nullables = Vec::new(); - if let Explain::QueryPlan(query_plan @ QueryPlan { plan, .. }) = &explain { - if let Some(outputs) = &query_plan.plan.output { - nullables.resize(outputs.len(), None); - visit_plan(&plan, outputs, &mut nullables); - } + if let Explain::Plan { + plan: + plan @ Plan { + output: Some(ref outputs), + .. + }, + } = &explain + { + nullables.resize(outputs.len(), None); + visit_plan(plan, outputs, &mut nullables); } Ok(nullables) @@ -487,23 +492,30 @@ fn visit_plan(plan: &Plan, outputs: &[String], nullables: &mut Vec> } } -#[derive(serde::Deserialize)] +#[derive(serde::Deserialize, Debug)] #[serde(untagged)] enum Explain { - /// {"Plan": ...} -- returned for most statements - QueryPlan(QueryPlan), - /// The string "Utility Statement" -- returned for - /// a CALL statement - UtilityStatement(String), -} - -#[derive(serde::Deserialize)] -struct QueryPlan { - #[serde(rename = "Plan")] - plan: Plan, + // NOTE: the returned JSON may not contain a `plan` field, for example, with `CALL` statements: + // https://github.com/launchbadge/sqlx/issues/1449 + // + // In this case, we should just fall back to assuming all is nullable. + // + // It may also contain additional fields we don't care about, which should not break parsing: + // https://github.com/launchbadge/sqlx/issues/2587 + // https://github.com/launchbadge/sqlx/issues/2622 + Plan { + #[serde(rename = "Plan")] + plan: Plan, + }, + + // This ensures that parsing never technically fails. + // + // We don't want to specifically expect `"Utility Statement"` because there might be other cases + // and we don't care unless it contains a query plan anyway. + Other(serde::de::IgnoredAny), } -#[derive(serde::Deserialize)] +#[derive(serde::Deserialize, Debug)] struct Plan { #[serde(rename = "Join Type")] join_type: Option, @@ -514,3 +526,60 @@ struct Plan { #[serde(rename = "Plans")] plans: Option>, } + +#[test] +fn explain_parsing() { + let normal_plan = r#"[ + { + "Plan": { + "Node Type": "Result", + "Parallel Aware": false, + "Async Capable": false, + "Startup Cost": 0.00, + "Total Cost": 0.01, + "Plan Rows": 1, + "Plan Width": 4, + "Output": ["1"] + } + } +]"#; + + // https://github.com/launchbadge/sqlx/issues/2622 + let extra_field = r#"[ + { + "Plan": { + "Node Type": "Result", + "Parallel Aware": false, + "Async Capable": false, + "Startup Cost": 0.00, + "Total Cost": 0.01, + "Plan Rows": 1, + "Plan Width": 4, + "Output": ["1"] + }, + "Query Identifier": 1147616880456321454 + } +]"#; + + // https://github.com/launchbadge/sqlx/issues/1449 + let utility_statement = r#"["Utility Statement"]"#; + + let normal_plan_parsed = serde_json::from_str::<[Explain; 1]>(normal_plan).unwrap(); + let extra_field_parsed = serde_json::from_str::<[Explain; 1]>(extra_field).unwrap(); + let utility_statement_parsed = serde_json::from_str::<[Explain; 1]>(utility_statement).unwrap(); + + assert!( + matches!(normal_plan_parsed, [Explain::Plan { plan: Plan { .. } }]), + "unexpected parse from {normal_plan:?}: {normal_plan_parsed:?}" + ); + + assert!( + matches!(extra_field_parsed, [Explain::Plan { plan: Plan { .. } }]), + "unexpected parse from {extra_field:?}: {extra_field_parsed:?}" + ); + + assert!( + matches!(utility_statement_parsed, [Explain::Other(_)]), + "unexpected parse from {utility_statement:?}: {utility_statement_parsed:?}" + ) +} From ddcd69c2e68bf4d869c5331d8006d781bd3bf97d Mon Sep 17 00:00:00 2001 From: Austin Bonander Date: Mon, 31 Jul 2023 14:17:44 -0700 Subject: [PATCH 5/5] chore: document why we load `pg_stat_statements` in tests --- tests/docker-compose.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/docker-compose.yml b/tests/docker-compose.yml index d50e431a49..4c59dc5034 100644 --- a/tests/docker-compose.yml +++ b/tests/docker-compose.yml @@ -182,6 +182,8 @@ services: POSTGRES_INITDB_ARGS: --auth-host=scram-sha-256 volumes: - "./postgres/setup.sql:/docker-entrypoint-initdb.d/setup.sql" + # Loading `pg_stat_statements` should serve as a regression test for: + # https://github.com/launchbadge/sqlx/issues/2622 command: > -c ssl=on -c ssl_cert_file=/var/lib/postgresql/server.crt -c ssl_key_file=/var/lib/postgresql/server.key -c shared_preload_libraries=pg_stat_statements