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

Thanks a lot Amit for looking at this and providing some useful pointers.

On 2017/07/28 20:46, Amit Khandekar wrote:
> On 28 July 2017 at 11:17, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2017/07/26 16:58, Amit Langote wrote:
>>> Rajkumar Raghuwanshi reported [1] on the "UPDATE partition key" thread
>>> that whole-row vars don't play nicely with partitioned table operations.
>>>
>>> For example, when used to convert WITH CHECK OPTION constraint expressions
>>> and RETURNING target list expressions, it will error out if the
>>> expressions contained whole-row vars.  That's a bug, because whole-row
>>> vars are legal in those expressions.  I think the decision to output error
>>> upon encountering a whole-row in the input expression was based on the
>>> assumption that it will only ever be used to convert partition constraint
>>> expressions, which cannot contain those.  So, we can relax that
>>> requirement so that its other users are not bitten by it.
>>>
>>> Attached fixes that.
>>
>> Attached a revised version of the patch.
>>
>> Updated patch moves the if (found_whole_row) elog(ERROR) bit in
>> map_partition_varattnos to the callers.  Only the callers know if
>> whole-row vars are not expected in the expression it's getting converted
>> from map_partition_varattnos.  Given the current message text (mentioning
>> "partition key"), it didn't seem appropriate to have the elog inside this
>> function, since it's callable from places where it wouldn't make sense.
> 
> 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.

Didn't see that coming.

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

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

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)

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