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

On 2 August 2017 at 11:51, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Thanks Fuita-san and Amit for reviewing.
>
> On 2017/08/02 1:33, Amit Khandekar wrote:
>> 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.)
>
> That's a good point, although it sounds like a bigger project that, IMHO,
> should be undertaken separately, because that would involve designing for
> considerations of expanding inheritance even in the INSERT case.
>
>> 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.
>
> Yeah, I think it'd be a good idea to do those projects together.  That is,
> doing what Fujita-san suggested and expanding partitioned tables in
> partition bound order in the planner.
>
>>>> 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.
>
> You're right, from_rowtype is unnecessary.
>
>> -------
>>
>> Few more comments :
>>
>> @@ -1240,7 +1247,7 @@ map_variable_attnos_mutator(Node *node,
>>  var->varlevelsup == context->sublevels_up)
>>  {
>>  /* Found a matching variable, 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.
>
> Done.
>
>> -------
>>
>> -       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.
>
> Actually, the first patch I posted on this thread did exactly the same
> thing (it added a wholerow_ok argument to map_partition_varattnos similar
> in spirit to your error_on_whole_row), but later dropped that idea because
> of the ambiguity I felt about the error message text in
> map_partition_varattnos().
>
> Actually, unlike other callers of map_variable_attnos(),
> map_partition_varattnos *can* in fact convert the whole-row Vars, because
> it has all the available information to do so (some of the other callers
> don't).  It's just that some callers of map_partition_varattnos() convert
> expressions containing only the partition key Vars which cannot be
> whole-row Vars, so any whole-row Var that map_variable_attnos() finds is
> an unexpected error condition.  So the current text reads: "unexpected
> whole-row reference found in partition key".  There are other callers of
> map_partition_varattnos() (more will crop up in the future) that don't
> necessarily pass expressions containing only the partition key, so having
> the above error message sounds odd.  So, I think it's only the callers of
> map_partition_varattnos() who know whether finding whole-row vars is an
> error and *what kind* of error.

Ok. Got it. Thanks for the explanation.

How about making found_whole_row as an optional parameter ?
map_partition_varattnos() would populate it only if its not NULL. This
way, callers who don't bother about whether it is found or not don't
have to declare a variable and pass its address. In current code,
ExecInitModifyTable() is the one who has to declare found_whole_row
without needing it.

Other than this, with the updated patches, I have no more comments from my end.

> I also think this reasoning is similar to
> why map_variable_attnos_mutator() itself does not output error on seeing a
> whole-row var.
>
> Attached updated patches.
>
> Thanks,
> Amit



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