Re: problem with RETURNING and update row movement

Fujita-san,

Thanks for your time on this.

On Sun, Aug 2, 2020 at 5:57 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> Yet another thing I noticed is that the patch incorrectly produces
> values for the tableoid columns specified in the RETURNING list, like
> this:

Yeah, I noticed that too.

> +UPDATE range_parted r set a = 'c' FROM (VALUES ('a', 1), ('a', 10),
> ('b', 12)) s(x, y) WHERE s.x = r.a AND s.y = r.b RETURNING
> tableoid::regclass, *;
> + tableoid | a | b | c | d | e | x | y
> +----------------+---+----+-----+---+---------------+---+----
> + part_a_1_a_10 | c | 1 | 1 | 1 | in trigfunc() | a | 1
> + part_a_10_a_20 | c | 10 | 200 | 1 | in trigfunc() | a | 10
> + part_c_1_100 | c | 12 | 96 | 1 | in trigfunc() | b | 12
> +(3 rows)
>
> The source partitions are shown as tableoid, but the destination
> partition (ie, part_c_1_c_20) should be shown. To fix this, I
> modified the patch further so that 1) we override tts_tableOid of the
> original slot with the OID of the destination partition before calling
> ExecProcessReturning() if needed, and 2) in ExecProcessReturning(), we
> only initialize ecxt_scantuple's tts_tableOid when needed, which would
> save cycles a bit for non-foreign-table-direct-modification cases.
>
> Attached is a new version of the patch.

Thanks for the updated patch. I reviewed your changes in v3 too and
they looked fine to me.

However, I noticed that having system columns like ctid, xmin, etc. in
the RETURNING list is now broken and maybe irrepairably due to the
approach we are taking in the patch. Let me show an example:

drop table foo;
create table foo (a int, b int) partition by list (a);
create table foo1 (c int, b int, a int);
alter table foo1 drop c;
alter table foo attach partition foo1 for values in (1);
create table foo2 partition of foo for values in (2);
create table foo3 partition of foo for values in (3);
create or replace function trigfunc () returns trigger language
plpgsql as $$ begin new.b := 2; return new; end; $$;
create trigger trig before insert on foo2 for each row execute
function trigfunc();
insert into foo values (1, 1), (2, 2), (3, 3);
update foo set a = 2 from (values (1), (2), (3)) s(x) where a = s.x
returning tableoid::regclass, ctid, xmin, xmax, *;
tableoid | ctid | xmin | xmax | a | b | x
----------+----------------+------+------------+---+---+---
foo2 | (4294967295,0) | 128 | 4294967295 | 2 | 2 | 1
foo2 | (0,3) | 782 | 0 | 2 | 2 | 2
foo2 | (0,4) | 782 | 0 | 2 | 2 | 3
(3 rows)

During foo1's update, it appears that we are losing the system
information in the physical tuple initialized during ExecInsert on
foo2 during its conversion back to foo1's reltype using the new code.
I haven't been able to figure out how to preserve the system
information in HeapTuple contained in the destination slot across the
conversion. Note we want to use the latter to project the RETURNING
list.

By the way, you'll need two adjustments to even get this example
working, one of which is a reported problem that system columns in
RETURNING list during an operation on partitioned table stopped
working in PG 12 [1] for which I've proposed a workaround (attached).
Another is that we forgot in our patch to "materialize" the virtual
tuple after conversion, which means slot_getsysattr() can't find it to
access system columns like xmin, etc.

--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

[1] https://www.postgresql.org/message-id/141051591267657%40mail.yandex.ru