From 415ebedbaf3862c6fb9b88f75e298f7620844218 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Villemain?= Date: Mon, 3 Mar 2025 08:07:18 +0100 Subject: [PATCH] fix issue #7676: wrong handler around MULTIEXPR Citus was checking for presence of sublink, but forgot to manage multiexpr while evaluating clauses during planning. At this stage (citus planner), it's not always possible to call PostgreSQL code because the tree is not yet ready for PostgreSQL pure executor. https://github.com/citusdata/citus/issues/7676 Fixed by adding a new function to check sublink or multiexp in the tree. --- src/backend/distributed/utils/citus_clauses.c | 55 ++++++++++++++++--- src/test/regress/expected/issue_7676.out | 35 ++++++++++++ src/test/regress/multi_schedule | 2 +- src/test/regress/sql/issue_7676.sql | 24 ++++++++ 4 files changed, 108 insertions(+), 8 deletions(-) create mode 100644 src/test/regress/expected/issue_7676.out create mode 100644 src/test/regress/sql/issue_7676.sql diff --git a/src/backend/distributed/utils/citus_clauses.c b/src/backend/distributed/utils/citus_clauses.c index f88b173af..4f43411c4 100644 --- a/src/backend/distributed/utils/citus_clauses.c +++ b/src/backend/distributed/utils/citus_clauses.c @@ -41,6 +41,7 @@ static bool ShouldEvaluateExpression(Expr *expression); static bool ShouldEvaluateFunctions(CoordinatorEvaluationContext *evaluationContext); static void FixFunctionArguments(Node *expr); static bool FixFunctionArgumentsWalker(Node *expr, void *context); +static bool CheckContainsMultiexprOrSublink(Node *expr); /* @@ -99,15 +100,15 @@ PartiallyEvaluateExpression(Node *expression, } NodeTag nodeTag = nodeTag(expression); - if (nodeTag == T_Param) + if (CheckContainsMultiexprOrSublink(expression)) { - Param *param = (Param *) expression; - if (param->paramkind == PARAM_SUBLINK) - { - /* ExecInitExpr cannot handle PARAM_SUBLINK */ - return expression; - } + /* ExecInitExpr cannot handle PARAM_MULTIEXPR and PARAM_SUBLINK */ + return expression; + } + /* ExecInitExpr cannot handle PARAM_MULTIEXPR and PARAM_SUBLINK but we have guards */ + else if (nodeTag == T_Param) + { return (Node *) citus_evaluate_expr((Expr *) expression, exprType(expression), exprTypmod(expression), @@ -537,3 +538,43 @@ FixFunctionArgumentsWalker(Node *expr, void *context) return expression_tree_walker(expr, FixFunctionArgumentsWalker, NULL); } + + +/* + * Recursively search an expression for a Param with sublink + */ +static bool +CheckContainsMultiexprOrSublink(Node *expr) +{ + if (expr == NULL) + { + return false; + } + + /* If it's a Param, return its attnum */ + else if (IsA(expr, Param)) + { + Param *param = (Param *) expr; + if (param->paramkind == PARAM_MULTIEXPR || + param->paramkind == PARAM_SUBLINK) + { + return true; + } + } + + /* If it's a FuncExpr, search in arguments */ + else if (IsA(expr, FuncExpr)) + { + FuncExpr *func = (FuncExpr *) expr; + ListCell *lc; + + foreach(lc, func->args) + { + if (CheckContainsMultiexprOrSublink((Node *) lfirst(lc))) + { + return true; + } + } + } + return false; +} diff --git a/src/test/regress/expected/issue_7676.out b/src/test/regress/expected/issue_7676.out new file mode 100644 index 000000000..08651816a --- /dev/null +++ b/src/test/regress/expected/issue_7676.out @@ -0,0 +1,35 @@ +-- https://github.com/citusdata/citus/issues/7676 +CREATE TABLE test_ref_multiexpr ( + id bigint primary key + , col_int integer + , col_bool bool + , col_text text + , col_timestamp timestamp + ); +select create_reference_table('test_ref_multiexpr'); + create_reference_table +--------------------------------------------------------------------- + +(1 row) + +/* TODO how to ensure in test that 'now()' is correctly pre-executed */ +insert into test_ref_multiexpr values (1, 1, true, 'one', now()); +update test_ref_multiexpr +SET (col_timestamp) + = (SELECT now()) +returning id, col_int, col_bool; + id | col_int | col_bool +--------------------------------------------------------------------- + 1 | 1 | t +(1 row) + +update test_ref_multiexpr +SET (col_bool, col_timestamp) + = (SELECT true, now()) +returning id, col_int, col_bool; + id | col_int | col_bool +--------------------------------------------------------------------- + 1 | 1 | t +(1 row) + +DROP TABLE test_ref_multiexpr; diff --git a/src/test/regress/multi_schedule b/src/test/regress/multi_schedule index 0fa54bb38..42355ef8c 100644 --- a/src/test/regress/multi_schedule +++ b/src/test/regress/multi_schedule @@ -103,7 +103,7 @@ test: multi_dropped_column_aliases foreign_key_restriction_enforcement test: binary_protocol test: alter_table_set_access_method test: alter_distributed_table -test: issue_5248 issue_5099 issue_5763 issue_6543 issue_6758 issue_7477 +test: issue_5248 issue_5099 issue_5763 issue_6543 issue_6758 issue_7477 issue_7676 test: object_propagation_debug test: undistribute_table test: run_command_on_all_nodes diff --git a/src/test/regress/sql/issue_7676.sql b/src/test/regress/sql/issue_7676.sql new file mode 100644 index 000000000..5ff912491 --- /dev/null +++ b/src/test/regress/sql/issue_7676.sql @@ -0,0 +1,24 @@ +-- https://github.com/citusdata/citus/issues/7676 +CREATE TABLE test_ref_multiexpr ( + id bigint primary key + , col_int integer + , col_bool bool + , col_text text + , col_timestamp timestamp + ); +select create_reference_table('test_ref_multiexpr'); + +/* TODO how to ensure in test that 'now()' is correctly pre-executed */ +insert into test_ref_multiexpr values (1, 1, true, 'one', now()); + +update test_ref_multiexpr +SET (col_timestamp) + = (SELECT now()) +returning id, col_int, col_bool; + +update test_ref_multiexpr +SET (col_bool, col_timestamp) + = (SELECT true, now()) +returning id, col_int, col_bool; + +DROP TABLE test_ref_multiexpr;