Re: Handling RestrictInfo in expression_tree_walker

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?

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

Hi Konstantin,

On Wed, Aug 7, 2019 at 4:24 PM Konstantin Knizhnik
<k(dot)knizhnik(at)postgrespro(dot)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:

https://www.postgresql.org/message-id/553FC9BC.5060402%402ndquadrant.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(dot)knizhnik(at)postgrespro(dot)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:
>
> https://www.postgresql.org/message-id/553FC9BC.5060402%402ndquadrant.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(dot)knizhnik(at)postgrespro(dot)ru> wrote:
> On 07.08.2019 10:42, Amit Langote wrote:
> > You may also want to read this discussion:
> >
> > https://www.postgresql.org/message-id/553FC9BC.5060402%402ndquadrant.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(dot)knizhnik(at)postgrespro(dot)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