Re: [HACKERS] map_partition_varattnos() and whole-row vars

On 1 August 2017 at 15:11, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
> On 2017/07/31 18:56, Amit Langote wrote:
>> Yes, that's what's needed here.  So we need to teach
>> map_variable_attnos_mutator() to convert whole-row vars just like it's
>> done in adjust_appendrel_attrs_mutator().
>
>
> Seems reasonable.  (Though I think it might be better to do this kind of
> conversion in the planner, not the executor, because that would increase the
> efficiency of cached plans.)
I think the work of shifting to planner should be taken as a different
task when we shift the preparation of DispatchInfo to the planner.

>
>>> I suspect that the other nodes that adjust_appendrel_attrs_mutator
>>> handles (like JoinExpr, CurrentOfExpr, etc ) may also require similar
>>> adjustments for our case, because WithCheckOptions (and I think even
>>> RETURNING) can have a subquery.
>>
>>
>> Actually, WITH CHECK and RETURNING expressions that are processed in the
>> executor (i.e., in ExecInitModifyTable) are finished plan tree expressions
>> (not parse or rewritten parse tree expressions), so we need not have to
>> worry about having to convert those things in this case.
>>
>> Remember that adjust_appendrel_attrs_mutator() has to handle raw Query
>> trees, because we plan the whole query separately for each partition in
>> the UPDATE and DELETE (inheritance_planner).  Since we don't need to plan
>> an INSERT query for each partition separately (at least without the
>> foreign tuple-routing support), we need not worry about converting
>> anything beside Vars (with proper support for converting whole-row ones
>> which you discovered has been missing), which we can do within the
>> executor on the finished plan tree expressions.
>
>
> Yeah, sublinks in WITH CHECK OPTION and RETURNING would already have been
> converted to subplans by the planner, so we don't need to worry about
> recursing into subqueries in sublinks at the execution time.

Yes, that seems true. It seems none of the nodes handled by
adjust_appendrel_attrs_mutator() other than Var nodes exist in planned
expressions. So looks like just additionally handling whole rows
should be sufficient.

>
>> Attached 2 patches:
>>
>> 0001: Addresses the bug that Rajkumar reported (handling whole-row vars in
>>        WITH CHECK and RETURNING expressions at all)
>>
>> 0002: Addressed the bug that Amit reported (converting whole-row vars
>>        that could occur in WITH CHECK and RETURNING expressions)
>
>
> I took a quick look at the patches.  One thing I noticed is this:
>
>  map_variable_attnos(Node *node,
>                                         int target_varno, int sublevels_up,
>                                         const AttrNumber *attno_map, int
> map_length,
> +                                       Oid from_rowtype, Oid to_rowtype,
>                                         bool *found_whole_row)
>  {
>         map_variable_attnos_context context;
> @@ -1291,6 +1318,8 @@ map_variable_attnos(Node *node,
>         context.sublevels_up = sublevels_up;
>         context.attno_map = attno_map;
>         context.map_length = map_length;
> +       context.from_rowtype = from_rowtype;
> +       context.to_rowtype = to_rowtype;
>         context.found_whole_row = found_whole_row;
>
> You added two arguments to pass to map_variable_attnos(): from_rowtype and
> to_rowtype.  But I'm not sure that we really need the from_rowtype argument
> because it's possible to get the Oid from the vartype of target whole-row
> Vars.  And in this:
>
> +                               /*
> +                                * If the callers expects us to convert the
> same, do so if
> +                                * necessary.
> +                                */
> +                               if (OidIsValid(context->to_rowtype) &&
> +                                       OidIsValid(context->from_rowtype) &&
> +                                       context->to_rowtype !=
> context->from_rowtype)
> +                               {
> +                                       ConvertRowtypeExpr *r =
> makeNode(ConvertRowtypeExpr);
> +
> +                                       r->arg = (Expr *) newvar;
> +                                       r->resulttype =
> context->from_rowtype;
> +                                       r->convertformat =
> COERCE_IMPLICIT_CAST;
> +                                       r->location = -1;
> +                                       /* Make sure the Var node has the
> right type ID, too */
> +                                       newvar->vartype =
> context->to_rowtype;
> +                                       return (Node *) r;
> +                               }
>
> I think we could set r->resulttype to the vartype (ie, "r->resulttype =
> newvar->vartype" instead of "r->resulttype = context->from_rowtype").

I agree.

-------

Few more comments :

@@ -1240,7 +1247,7 @@ map_variable_attnos_mutator(Node *node,var->varlevelsup == context->sublevels_up){/* Found a
matchingvariable, make the substitution */
 

- Var                *newvar = (Var *) palloc(sizeof(Var));
+ Var                *newvar = copyObject(var); int                     attno = var->varattno;
*newvar = *var;

Here, "*newvar = *var" should be removed.


-------

-       result = map_partition_varattnos(result, 1, rel, parent);
+       result = map_partition_varattnos(result, 1, rel, parent,
+ &found_whole_row);
+       /* There can never be a whole-row reference here */
+       if (found_whole_row)
+               elog(ERROR, "unexpected whole-row reference found in
partition key");

Instead of callers of map_partition_varattnos() reporting error, we
can have map_partition_varattnos() itself report error. Instead of the
found_whole_row parameter of map_partition_varattnos(), we can have
error_on_whole_row parameter. So callers who don't expect whole row,
would pass error_on_whole_row=true to map_partition_varattnos(). This
will simplify the resultant code a bit.

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company