Thread: Hard to maintain duplication in contain_volatile_functions_not_nextval_walker

Hi,

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?

Regards,

Andres

On Fri, May 27, 2016 at 10:36 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sat, May 28, 2016 at 12:28 AM, Andres Freund <andres@anarazel.de> wrote:
>> contain_volatile_functions_walker is duplicated, near entirely, in
>> contain_volatile_functions_not_nextval_walker.
>
> Previously, I also had same observation.
>
>> Wouldn't it have been better not to duplicate, and keep a flag about
>> ignoring nextval in the context variable?
>
> makes sense.  +1 for doing it in the way as you are suggesting.

+1 from me, too.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Fri, Jun 10, 2016 at 11:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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?

I like it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company