Re: map_partition_varattnos() and whole-row vars

On 2017/07/31 18:56, Amit Langote wrote:
> On 2017/07/28 20:46, Amit Khandekar wrote:
>> create table foo (a int, b text) partition by list (a);
>> create table foo1 partition of foo for values in (1);
>> create table foo2(b text, a int) ;
>> alter table foo attach partition foo2 for values in (2);
>>
>> postgres=# insert into foo values (1, 'hi there');
>> INSERT 0 1
>> postgres=# insert into foo values (2, 'bi there');
>> INSERT 0 1
>> postgres=# insert into foo values (2, 'error there') returning foo;
>> ERROR: table row type and query-specified row type do not match
>> DETAIL: Table has type text at ordinal position 1, but query expects integer.

>> This is due to a mismatch between the composite type tuple descriptor
>> of the leaf partition doesn't match the RETURNING slot tuple
>> descriptor.
>>
>> We probably need to do what
>> inheritance_planner()=>adjust_appendrel_attrs() does for adjusting the
>> attrs in the child rel parse tree; i.e., handle some specific nodes
>> other than just simple var nodes. In adjust_appendrel_attrs_mutator(),
>> for whole row node, it updates var->vartype to the child rel.
>
> 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 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.

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

Best regards,
Etsuro Fujita