Re: map_partition_varattnos() and whole-row vars

On 2 August 2017 at 11:51, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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