Andres Freund <and...@anarazel.de> writes: > contain_volatile_functions_walker is duplicated, near entirely, in > contain_volatile_functions_not_nextval_walker. > Wouldn't it have been better not to duplicate, and keep a flag about > ignoring nextval in the context variable? > While at it, couldn't we also fold contain_mutable_functions_walker() > together using a similar technique?
I had what might be a better idea about refactoring in this area. Most of the bulk of contain_volatile_functions and friends consists of knowing how to locate the function OIDs, if any, embedded in a given expression node. I am envisioning a helper function that looks like typedef bool (*check_func) (Oid func_oid, void *context); bool check_functions_in_node(Node *node, check_func checker, void *context) { if (IsA(node, FuncExpr)) { FuncExpr *expr = (FuncExpr *) node; if (checker(expr->funcid, context)) return true; } else if (IsA(node, OpExpr)) { OpExpr *expr = (OpExpr *) node; set_opfuncid(expr); if (checker(expr->opfuncid, context)) return true; } ... return false; } and then for example contain_volatile_functions_walker would reduce to static bool contain_volatile_functions_checker(Oid func_oid, void *context) { return (func_volatile(func_oid) == PROVOLATILE_VOLATILE); } static bool contain_volatile_functions_walker(Node *node, void *context) { if (node == NULL) return false; if (check_functions_in_node(node, contain_volatile_functions_checker, context)) return true; if (IsA(node, Query)) { /* Recurse into subselects */ return query_tree_walker((Query *) node, contain_volatile_functions_walker, context, 0); } return expression_tree_walker(node, contain_volatile_functions_walker, context); } Note that the helper function doesn't recurse into child nodes, it only examines the given node. This is because some of the potential callers have additional checks that they need to apply, so it's better if the call site retains control of the recursion. By my count there are half a dozen places in clauses.c that could make use of this, at a savings of about 80 lines each, as well as substantial removal of risks-of-omission. There might be use-cases elsewhere, so I'm rather inclined to put the checker function in nodeFuncs.c. Thoughts? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers