Re: Hard to maintain duplication in contain_volatile_functions_not_nextval_walker

Andres Freund <andres@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