Re: problem with RETURNING and update row movement

On Mon, Aug 3, 2020 at 2:54 PM Amit Langote <amitlangote09@gmail.com> wrote:
> 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.

The only solution I could think of for this so far is this:

+                   if (map)
+                   {
+                       orig_slot = execute_attr_map_slot(map,
+                                                         res_slot,
+                                                         orig_slot);
+
+                       /*
+                        * A HACK to install system information into the just
+                        * converted tuple so that RETURNING computes any
+                        * system columns correctly. This would be the same
+                        * information that would be present in the HeapTuple
+                        * version of the tuple in res_slot.
+                        */
+                       tuple = ExecFetchSlotHeapTuple(orig_slot, true,
+                                                      &should_free);
+                       tuple->t_data->t_infomask &= ~(HEAP_XACT_MASK);
+                       tuple->t_data->t_infomask2 &= ~(HEAP2_XACT_MASK);
+                       tuple->t_data->t_infomask |= HEAP_XMAX_INVALID;
+                       HeapTupleHeaderSetXmin(tuple->t_data,
+                                              GetCurrentTransactionId());
+                       HeapTupleHeaderSetCmin(tuple->t_data,
+                                              estate->es_output_cid);
+                       HeapTupleHeaderSetXmax(tuple->t_data, 0); /*
for cleanliness */
+                   }
+                   /*
+                    * Override tts_tableOid with the OID of the destination
+                    * partition.
+                    */
+                   orig_slot->tts_tableOid =
RelationGetRelid(destRel->ri_RelationDesc);
+                   /* Also the TID. */
+                   orig_slot->tts_tid = res_slot->tts_tid;

..but it might be too ugly :(.

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