On Fri, Jun 10, 2016 at 11:12 AM, Tom Lane <t...@sss.pgh.pa.us> wrote: > 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?
I like it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers