From 002ee98962b5f89227e1fa4751a146a188e557dd Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 29 Jan 2025 18:35:36 -0500 Subject: [PATCH v1 3/8] Remove contain_context_dependent_node function-inlining restriction. Now that CaseExpr and ArrayCoerceExpr convert their CaseTestExpr sub-nodes to Params, the sort of confusion that this test meant to prevent is impossible, and indeed the search function cannot return true anymore (except perhaps in estimation mode, where it won't matter). Let's remove the useless and optimization-defeating code. Some of the regression tests added by commits f0c7b789a and 14a158f9b are now dead code, in that the errors they intend to test for are now impossible by design. I left them there for now because the fact that they still pass is good validation for this patch series. I'm wondering whether to remove them in the final version though, because it's hard to justify expending test cycles forever on dead issues. --- src/backend/optimizer/util/clauses.c | 100 --------------------------- 1 file changed, 100 deletions(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 4edbbabf59..8aa7ae8cac 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -103,8 +103,6 @@ static bool max_parallel_hazard_walker(Node *node, max_parallel_hazard_context *context); static bool contain_nonstrict_functions_walker(Node *node, void *context); static bool contain_exec_param_walker(Node *node, List *param_ids); -static bool contain_context_dependent_node(Node *clause); -static bool contain_context_dependent_node_walker(Node *node, int *flags); static bool contain_leaked_vars_walker(Node *node, void *context); static Relids find_nonnullable_rels_walker(Node *node, bool top_level); static List *find_nonnullable_vars_walker(Node *node, bool top_level); @@ -1167,95 +1165,6 @@ contain_exec_param_walker(Node *node, List *param_ids) return expression_tree_walker(node, contain_exec_param_walker, param_ids); } -/***************************************************************************** - * Check clauses for context-dependent nodes - *****************************************************************************/ - -/* - * contain_context_dependent_node - * Recursively search for context-dependent nodes within a clause. - * - * CaseTestExpr nodes must appear directly within the corresponding CaseExpr, - * not nested within another one, or they'll see the wrong test value. If one - * appears "bare" in the arguments of a SQL function, then we can't inline the - * SQL function for fear of creating such a situation. The same applies for - * CaseTestExpr used within the elemexpr of an ArrayCoerceExpr. - * - * CoerceToDomainValue would have the same issue if domain CHECK expressions - * could get inlined into larger expressions, but presently that's impossible. - * Still, it might be allowed in future, or other node types with similar - * issues might get invented. So give this function a generic name, and set - * up the recursion state to allow multiple flag bits. - */ -static bool -contain_context_dependent_node(Node *clause) -{ - int flags = 0; - - return contain_context_dependent_node_walker(clause, &flags); -} - -#define CCDN_CASETESTEXPR_OK 0x0001 /* CaseTestExpr okay here? */ - -static bool -contain_context_dependent_node_walker(Node *node, int *flags) -{ - if (node == NULL) - return false; - if (IsA(node, CaseTestExpr)) - return !(*flags & CCDN_CASETESTEXPR_OK); - else if (IsA(node, CaseExpr)) - { - CaseExpr *caseexpr = (CaseExpr *) node; - - /* - * If this CASE doesn't have a test expression, then it doesn't create - * a context in which CaseTestExprs should appear, so just fall - * through and treat it as a generic expression node. - */ - if (caseexpr->arg) - { - int save_flags = *flags; - bool res; - - /* - * Note: in principle, we could distinguish the various sub-parts - * of a CASE construct and set the flag bit only for some of them, - * since we are only expecting CaseTestExprs to appear in the - * "expr" subtree of the CaseWhen nodes. But it doesn't really - * seem worth any extra code. If there are any bare CaseTestExprs - * elsewhere in the CASE, something's wrong already. - */ - *flags |= CCDN_CASETESTEXPR_OK; - res = expression_tree_walker(node, - contain_context_dependent_node_walker, - flags); - *flags = save_flags; - return res; - } - } - else if (IsA(node, ArrayCoerceExpr)) - { - ArrayCoerceExpr *ac = (ArrayCoerceExpr *) node; - int save_flags; - bool res; - - /* Check the array expression */ - if (contain_context_dependent_node_walker((Node *) ac->arg, flags)) - return true; - - /* Check the elemexpr, which is allowed to contain CaseTestExpr */ - save_flags = *flags; - *flags |= CCDN_CASETESTEXPR_OK; - res = contain_context_dependent_node_walker((Node *) ac->elemexpr, - flags); - *flags = save_flags; - return res; - } - return expression_tree_walker(node, contain_context_dependent_node_walker, - flags); -} - /***************************************************************************** * Check clauses for Vars passed to non-leakproof functions *****************************************************************************/ @@ -4658,8 +4567,6 @@ evaluate_function(Oid funcid, Oid result_type, int32 result_typmod, * doesn't work in the general case because it discards information such * as OUT-parameter declarations. * - * Also, context-dependent expression nodes in the argument list are trouble. - * * Returns a simplified expression if successful, or NULL if cannot * simplify the function. */ @@ -4899,13 +4806,6 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid, contain_nonstrict_functions(newexpr)) goto fail; - /* - * If any parameter expression contains a context-dependent node, we can't - * inline, for fear of putting such a node into the wrong context. - */ - if (contain_context_dependent_node((Node *) args)) - goto fail; - /* * We may be able to do it; there are still checks on parameter usage to * make, but those are most easily done in combination with the actual -- 2.43.5