From c4b6fb1dd075f96d1f423ef89f47cf3c2653e940 Mon Sep 17 00:00:00 2001 From: EmelSimsek Date: Mon, 13 May 2024 15:19:25 +0300 Subject: [PATCH 1/4] Attempt to fix parameter passing with auto_explain --- src/backend/distributed/planner/multi_explain.c | 15 +++++---------- src/test/regress/sql/multi_explain.sql | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/backend/distributed/planner/multi_explain.c b/src/backend/distributed/planner/multi_explain.c index 4584e774024..cb8029e94ad 100644 --- a/src/backend/distributed/planner/multi_explain.c +++ b/src/backend/distributed/planner/multi_explain.c @@ -808,7 +808,11 @@ FetchRemoteExplainFromWorkers(Task *task, ExplainState *es, ParamListInfo params if (params) { - ExtractParametersFromParamList(params, ¶mTypes, ¶mValues, false); + /* force evaluation of bound params */ + params = copyParamList(params); + + ExtractParametersForRemoteExecution(params, ¶mTypes, + ¶mValues); } int sendStatus = SendRemoteCommandParams(connection, explainQuery->data, @@ -1585,15 +1589,6 @@ FetchPlanQueryForExplainAnalyze(const char *queryString, ParamListInfo params) { StringInfo fetchQuery = makeStringInfo(); - if (params != NULL) - { - /* - * Add a dummy CTE to ensure all parameters are referenced, such that their - * types can be resolved. - */ - appendStringInfo(fetchQuery, "WITH unused AS (%s) ", - ParameterResolutionSubquery(params)); - } appendStringInfoString(fetchQuery, "SELECT explain_analyze_output, execution_duration " diff --git a/src/test/regress/sql/multi_explain.sql b/src/test/regress/sql/multi_explain.sql index 7fa75c8be9b..6b430935a86 100644 --- a/src/test/regress/sql/multi_explain.sql +++ b/src/test/regress/sql/multi_explain.sql @@ -1168,6 +1168,22 @@ set auto_explain.log_analyze to true; -- the following should not be locally executed since explain analyze is on select * from test_ref_table; +CREATE TABLE test_auto_explain.test_params +( coll1 int8 NULL, + coll2 int4 NOT NULL); + +SELECT create_distributed_table('test_auto_explain.test_params', 'coll1'); + +CREATE OR REPLACE PROCEDURE test_auto_explain.test_delete_from(p_coll2 int) +LANGUAGE plpgsql +AS $$ +BEGIN +DELETE FROM test_auto_explain.test_params +WHERE coll2 = p_coll2; +END;$$; + +CALL test_auto_explain.test_delete_from(20240401); + DROP SCHEMA test_auto_explain CASCADE; SET client_min_messages TO ERROR; From 05c88392fd5b61d6d3390a6cee25607f607eb015 Mon Sep 17 00:00:00 2001 From: EmelSimsek Date: Tue, 14 May 2024 15:39:50 +0300 Subject: [PATCH 2/4] Fix --- .../distributed/executor/adaptive_executor.c | 5 ++++- src/backend/distributed/planner/multi_explain.c | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/backend/distributed/executor/adaptive_executor.c b/src/backend/distributed/executor/adaptive_executor.c index e912f418d6f..f24e5b30ac5 100644 --- a/src/backend/distributed/executor/adaptive_executor.c +++ b/src/backend/distributed/executor/adaptive_executor.c @@ -816,8 +816,11 @@ AdaptiveExecutor(CitusScanState *scanState) * be part of the same transaction. */ UseCoordinatedTransaction(); + + ParamListInfo boundParams = copyParamList(paramListInfo); + taskList = ExplainAnalyzeTaskList(taskList, defaultTupleDest, tupleDescriptor, - paramListInfo); + boundParams); /* * Multiple queries per task is not supported with local execution. See the Assert in diff --git a/src/backend/distributed/planner/multi_explain.c b/src/backend/distributed/planner/multi_explain.c index cb8029e94ad..6049fdcfafa 100644 --- a/src/backend/distributed/planner/multi_explain.c +++ b/src/backend/distributed/planner/multi_explain.c @@ -1589,6 +1589,16 @@ FetchPlanQueryForExplainAnalyze(const char *queryString, ParamListInfo params) { StringInfo fetchQuery = makeStringInfo(); + if (params != NULL) + { + /* + * Add a dummy CTE to ensure all parameters are referenced, such that their + * types can be resolved. + */ + appendStringInfo(fetchQuery, "WITH unused AS (%s) ", + ParameterResolutionSubquery(params)); + } + appendStringInfoString(fetchQuery, "SELECT explain_analyze_output, execution_duration " @@ -1613,6 +1623,12 @@ ParameterResolutionSubquery(ParamListInfo params) for (int paramIndex = 0; paramIndex < params->numParams; paramIndex++) { ParamExternData *param = ¶ms->params[paramIndex]; + + if (param->ptype == 0) + { + continue; + } + char *typeName = format_type_extended(param->ptype, -1, FORMAT_TYPE_FORCE_QUALIFY); From 301224d91fe0a3f430262b9ec64616e2c8a7cd70 Mon Sep 17 00:00:00 2001 From: EmelSimsek Date: Wed, 15 May 2024 09:22:22 +0300 Subject: [PATCH 3/4] Fix test --- src/test/regress/expected/multi_explain.out | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/test/regress/expected/multi_explain.out b/src/test/regress/expected/multi_explain.out index 17b67360774..bbc2e44ee1f 100644 --- a/src/test/regress/expected/multi_explain.out +++ b/src/test/regress/expected/multi_explain.out @@ -3225,6 +3225,19 @@ SET auto_explain.log_min_duration = 0; set auto_explain.log_analyze to true; -- the following should not be locally executed since explain analyze is on select * from test_ref_table; +CREATE TABLE test_auto_explain.test_params +( coll1 int8 NULL, + coll2 int4 NOT NULL); +SELECT create_distributed_table('test_auto_explain.test_params', 'coll1'); + +CREATE OR REPLACE PROCEDURE test_auto_explain.test_delete_from(p_coll2 int) +LANGUAGE plpgsql +AS $$ +BEGIN +DELETE FROM test_auto_explain.test_params +WHERE coll2 = p_coll2; +END;$$; +CALL test_auto_explain.test_delete_from(20240401); DROP SCHEMA test_auto_explain CASCADE; SET client_min_messages TO ERROR; DROP SCHEMA multi_explain CASCADE; From 2b520fdc4ab4f3a0aa78d5b91bfb51aab621ccc1 Mon Sep 17 00:00:00 2001 From: EmelSimsek Date: Fri, 17 May 2024 09:42:09 +0300 Subject: [PATCH 4/4] Fix expected for pg 14 15 --- src/test/regress/expected/multi_explain_0.out | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/test/regress/expected/multi_explain_0.out b/src/test/regress/expected/multi_explain_0.out index 9534cefb8c1..89b9c6ac89f 100644 --- a/src/test/regress/expected/multi_explain_0.out +++ b/src/test/regress/expected/multi_explain_0.out @@ -3214,6 +3214,19 @@ SET auto_explain.log_min_duration = 0; set auto_explain.log_analyze to true; -- the following should not be locally executed since explain analyze is on select * from test_ref_table; +CREATE TABLE test_auto_explain.test_params +( coll1 int8 NULL, + coll2 int4 NOT NULL); +SELECT create_distributed_table('test_auto_explain.test_params', 'coll1'); + +CREATE OR REPLACE PROCEDURE test_auto_explain.test_delete_from(p_coll2 int) +LANGUAGE plpgsql +AS $$ +BEGIN +DELETE FROM test_auto_explain.test_params +WHERE coll2 = p_coll2; +END;$$; +CALL test_auto_explain.test_delete_from(20240401); DROP SCHEMA test_auto_explain CASCADE; SET client_min_messages TO ERROR; DROP SCHEMA multi_explain CASCADE;