Re: problem with RETURNING and update row movement

On Wed, Sep 23, 2020 at 10:12 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Sun, Sep 20, 2020 at 11:41 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> > On Mon, Sep 14, 2020 at 10:45 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > > On Mon, Sep 14, 2020 at 5:56 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> > > > IIUC, I think two issues are discussed in the thread [1]: (a) there is
> > > > currently no way to define the set of meaningful system columns for a
> > > > partitioned table that contains pluggable storages other than standard
> > > > heap, and (b) even in the case where the set is defined as before,
> > > > like partitioned tables that don’t contain any pluggable storages,
> > > > system column values are not obtained from a tuple inserted into a
> > > > partitioned table in cases as reported in [1], because virtual slots
> > > > are assigned for partitioned tables [2][3]. (I think the latter is
> > > > the original issue in the thread, though.)

> > > > I think we could fix this update-tuple-routing-vs-RETURNING issue
> > > > without the fix for (a). But to transfer system column values in a
> > > > cleaner way, I think we need to fix (b) first so that we can always
> > > > obtain/copy them from the new tuple moved to another partition by
> > > > INSERT following DELETE.
> > >
> > > Yes, I think you are right. Although, I am still not sure how to
> > > "copy" system columns from one partition's slot to another's, that is,
> > > without assuming what they are.
> >
> > I just thought we assume that partitions support all the existing
> > system attributes until we have the fix for (a), i.e., the slot
> > assigned for a partition must have the getsysattr callback routine
> > from which we can fetch each existing system attribute of a underlying
> > tuple in the slot, regardless of whether that system attribute is used
> > for the partition or not.
>
> Hmm, to copy one slot's system attributes into another, we will also
> need a way to "set" the system attributes in the destination slot.
> But maybe I didn't fully understand what you said.

Sorry, my thought was vague. To store xmin/xmax/cmin/cmax into a
given slot, we need to extend the TupleTableSlot struct to contain
these attributes as well? Or we need to introduce a new callback
routine for that (say, setsysattr)? These would not be
back-patchable, though.

> > I also created a patch for PG11, which I am attaching
> > as well.
>
> In the patch for PG 11:
>
> + new_tuple->t_self = new_tuple->t_data->t_ctid =
> + old_tuple->t_self;
> ...
>
> Should we add a one-line comment above this block of code to transfer
> system attributes? Maybe: /* Also transfer the system attributes. */?

Will add that comment. Thanks for reviewing!

> BTW, do you think we should alter the test in PG 11 branch to test
> that system attributes are returned correctly?

Yeah, I think so. I didn’t come up with any good test cases for that, though.

> Once we settle the
> other issue, we can adjust the HEAD's test to do likewise?

Yeah, but for the other issue, I started thinking that we should just
forbid referencing xmin/xmax/cmin/cmax in 12, 13, and HEAD...

Best regards,
Etsuro Fujita