Re: Hard to maintain duplication in contain_volatile_functions_not_nextval_walker

On Fri, Jun 10, 2016 at 11:12 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andres Freund <andres(at)anarazel(dot)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