Re: map_partition_varattnos() and whole-row vars

Fujita-san,

Thanks for the review.

On 2017/08/03 16:01, Etsuro Fujita wrote:
> On 2017/08/02 15:21, Amit Langote wrote:
>> On 2017/08/02 1:33, Amit Khandekar wrote:
>>> -------
>>>
>>> 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.)

OK, done.

> 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 back to the ... */
> ...
> return (Node *) r;
> }
> }

Sounds good, so done.

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

I consider this a good suggestion. I guess we tend to list all the input
arguments before any output arguments. So done as you suggest.

> + * 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.)

Alright, dropped RelationGetRelType from the patch.

> +-- check that wholerow vars in the RETUNING list work with partitioned
> tables
>
> Typo: s/RETUNING/RETURNING/

Fixed.

Attached updated patches.

Thanks,
Amit