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

On 2017/08/02 15:21, Amit Langote 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.

Agreed.  I think that would be definitely a material for PG11.

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

OK

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

I'm not sure this change is a good idea, because the copy by "*newvar = 
*var" would be more efficient than the copyObject().  (We have this 
optimization in other places as well.  See eg, copyVar() in setrefs.c.)

Here are some other comments:

+                /* If the callers expects us to convert the same, do so. */
+                if (OidIsValid(context->to_rowtype))
+                {
+                    ConvertRowtypeExpr *r;
+
+                    /* Var itself is converted to the requested rowtype. */
+                    newvar->vartype = context->to_rowtype;
+
+                    /*
+                     * And a conversion step on top to convert back to the
+                     * original type.
+                     */
+                    r = makeNode(ConvertRowtypeExpr);
+                    r->arg = (Expr *) newvar;
+                    r->resulttype = var->vartype;
+                    r->convertformat = COERCE_IMPLICIT_CAST;
+                    r->location = -1;
+
+                    return (Node *) r;
+                }

Why not do this conversion if to_rowtype is valid and it's different 
from the rowtype of the original whole-row Var like the previous patch? 
Also, I think it's better to add an assertion that the rowtype of the 
original whole-row Var is a named one.  So, what I have in mind is:
  if (OidIsValid(context->to_rowtype))  {    Assert(var->vartype != RECORDOID)    if (var->vartype !=
context->to_rowtype)   {      ConvertRowtypeExpr *r;
 
      /* Var itself is converted to the requested rowtype. */      ...      /* And a conversion step on top to convert
backto the ... */      ...      return (Node *) r;    }  }
 

Here is the modification to the map_variable_attnos()'s API:
 map_variable_attnos(Node *node,                                        int target_varno, int sublevels_up,
                          const AttrNumber *attno_map, 
 
int map_length,
-                                       bool *found_whole_row)
+                                       bool *found_whole_row, Oid 
to_rowtype)

This is nitpicking, but I think it would be better to put the new 
argument to_rowtype right before the output argument found_whole_row.

+ * RelationGetRelType
+ *        Returns the rel's pg_type OID.
+ */
+#define RelationGetRelType(relation) \
+    ((relation)->rd_rel->reltype)

This macro is used in only one place.  Do we really need that?  (This 
macro would be useful for other places such as expand_inherited_rtentry, 
but I think it's better to introduce this in a separate patch, maybe for 
PG11.)

+-- check that wholerow vars in the RETUNING list work with partitioned 
tables

Typo: s/RETUNING/RETURNING/

Sorry for the delay.

Best regards,
Etsuro Fujita