Handling RestrictInfo in expression_tree_walker

Hi Konstantin,

On Wed, Aug 7, 2019 at 4:24 PM Konstantin Knizhnik
<k.knizhnik@postgrespro.ru> wrote:

Hi hackers,

I wonder if there is some particular reason for not handling
T_RestrictInfo node tag in expression_tree_walker?
There are many data structure in Postgres which contains lists of
RestrictInfo or expression with RestrictInfo as parameter (for example
orclause in RestrictInfo).
To handle such cases now it is needed to write code performing list
iteration and calling expression_tree_walker for each list element and
handling RrestrictInfo in callback function:

static bool
change_varno_walker(Node *node, ChangeVarnoContext *context)
{
if (node == NULL)
return false;

if (IsA(node, Var) && ((Var *) node)->varno == context->oldRelid)
{
((Var *) node)->varno = context->newRelid;
((Var *) node)->varnoold = context->newRelid;
return false;
}
if (IsA(node, RestrictInfo))
{
change_rinfo((RestrictInfo*)node, context->oldRelid,
context->newRelid);
return false;
}
return expression_tree_walker(node, change_varno_walker, context);
}

Are there any complaints against handling RestrictInfo in
expression_tree_walker?

As I understand it, RestrictInfo is not something that appears in
query trees or plan trees, but only in the planner data structures as
means of caching some information about the clauses that they wrap. I
see this comment describing what expression_tree_walker() is supposed
to handle:

* The node types handled by expression_tree_walker include all those
* normally found in target lists and qualifier clauses during the planning
* stage.

You may also want to read this discussion:

/messages/by-id/553FC9BC.5060402@2ndquadrant.com

Thanks,
Amit

On 07.08.2019 10:42, Amit Langote wrote:

Hi Konstantin,

On Wed, Aug 7, 2019 at 4:24 PM Konstantin Knizhnik
<k.knizhnik@postgrespro.ru> wrote:

Hi hackers,

I wonder if there is some particular reason for not handling
T_RestrictInfo node tag in expression_tree_walker?
There are many data structure in Postgres which contains lists of
RestrictInfo or expression with RestrictInfo as parameter (for example
orclause in RestrictInfo).
To handle such cases now it is needed to write code performing list
iteration and calling expression_tree_walker for each list element and
handling RrestrictInfo in callback function:

static bool
change_varno_walker(Node *node, ChangeVarnoContext *context)
{
if (node == NULL)
return false;

if (IsA(node, Var) && ((Var *) node)->varno == context->oldRelid)
{
((Var *) node)->varno = context->newRelid;
((Var *) node)->varnoold = context->newRelid;
return false;
}
if (IsA(node, RestrictInfo))
{
change_rinfo((RestrictInfo*)node, context->oldRelid,
context->newRelid);
return false;
}
return expression_tree_walker(node, change_varno_walker, context);
}

Are there any complaints against handling RestrictInfo in
expression_tree_walker?

As I understand it, RestrictInfo is not something that appears in
query trees or plan trees, but only in the planner data structures as
means of caching some information about the clauses that they wrap. I
see this comment describing what expression_tree_walker() is supposed
to handle:

* The node types handled by expression_tree_walker include all those
* normally found in target lists and qualifier clauses during the planning
* stage.

You may also want to read this discussion:

/messages/by-id/553FC9BC.5060402@2ndquadrant.com

Thanks,
Amit

Thank you very much for response and pointing me to this thread.
Unfortunately I do not understand from this thread how the problem was
solved with pullvars - right now  pull_varnos_walker and
pull_varattnos_walker
are not handling RestrictInfo.

Also I do not completely understand the argument "RestrictInfo is not a
general expression node and support for it has
been deliberately omitted from expression_tree_walker()". If there is
BoolOp expression which contains RestrictInfo expression as it
arguments, then either this expression is not
correct, either RestrictInfo should be considered as "expression node".

Frankly speaking I do not see some good reasons for not handling
RestrictInfo in expression_tree_worker. It can really simplify writing
of mutators/walkers.
And I do not think that reporting error instead of handling this tag
adds some extra safety or error protection.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi,

On Wed, Aug 7, 2019 at 5:07 PM Konstantin Knizhnik
<k.knizhnik@postgrespro.ru> wrote:

On 07.08.2019 10:42, Amit Langote wrote:

You may also want to read this discussion:

/messages/by-id/553FC9BC.5060402@2ndquadrant.com

Thank you very much for response and pointing me to this thread.
Unfortunately I do not understand from this thread how the problem was
solved with pullvars - right now pull_varnos_walker and
pull_varattnos_walker
are not handling RestrictInfo.

Also I do not completely understand the argument "RestrictInfo is not a
general expression node and support for it has
been deliberately omitted from expression_tree_walker()". If there is
BoolOp expression which contains RestrictInfo expression as it
arguments, then either this expression is not
correct, either RestrictInfo should be considered as "expression node".

Frankly speaking I do not see some good reasons for not handling
RestrictInfo in expression_tree_worker. It can really simplify writing
of mutators/walkers.
And I do not think that reporting error instead of handling this tag
adds some extra safety or error protection.

Well, Tom has expressed in various words in that thread that expecting
to successfully run expression_tree_walker() on something containing
RestrictInfos may be a sign of bad design somewhere in the code that
you're trying to add. I have recollections of submitting such code,
but later realizing that there's some other way to do things
differently that doesn't require walking expressions containing
RestrictInfos.

Btw, looking at the example walker function you've shown in the first
email, maybe you want to use a mutator, not a walker. The latter
class of functions is only supposed to inspect the input tree, not
modify it.

Thanks,
Amit

Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:

Frankly speaking I do not see some good reasons for not handling
RestrictInfo in expression_tree_worker. It can really simplify writing
of mutators/walkers.

I don't buy this; what seems more likely is that you're trying to apply
an expression tree mutator to something you shouldn't. The caching
aspects of RestrictInfo, and the fact that the planner often assumes
that RestrictInfos don't get copied (so that pointer equality is a
useful test), are both good reasons to be wary of applying general
mutations to those nodes.

Or in other words, if you want a walker/mutator to descend through
those nodes, you almost certainly need special logic at those nodes
anyway. Omitting them from the nodeFuncs support guarantees you
don't forget that.

regards, tom lane