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

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers