Re: map_partition_varattnos() and whole-row vars

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

>
>>> I suspect that the other nodes that adjust_appendrel_attrs_mutator
>>> handles (like JoinExpr, CurrentOfExpr, etc ) may also require similar
>>> adjustments for our case, because WithCheckOptions (and I think even
>>> RETURNING) can have a subquery.
>>
>>
>> Actually, WITH CHECK and RETURNING expressions that are processed in the
>> executor (i.e., in ExecInitModifyTable) are finished plan tree expressions
>> (not parse or rewritten parse tree expressions), so we need not have to
>> worry about having to convert those things in this case.
>>
>> Remember that adjust_appendrel_attrs_mutator() has to handle raw Query
>> trees, because we plan the whole query separately for each partition in
>> the UPDATE and DELETE (inheritance_planner). Since we don't need to plan
>> an INSERT query for each partition separately (at least without the
>> foreign tuple-routing support), we need not worry about converting
>> anything beside Vars (with proper support for converting whole-row ones
>> which you discovered has been missing), which we can do within the
>> executor on the finished plan tree expressions.
>
>
> Yeah, sublinks in WITH CHECK OPTION and RETURNING would already have been
> converted to subplans by the planner, so we don't need to worry about
> recursing into subqueries in sublinks at the execution time.

Yes, that seems true. It seems none of the nodes handled by
adjust_appendrel_attrs_mutator() other than Var nodes exist in planned
expressions. So looks like just additionally handling whole rows
should be sufficient.

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

-------

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.

-------

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

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