Re: Handling RestrictInfo in expression_tree_walker

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:
>
> 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