Re: problem with RETURNING and update row movement

Fujita-san,

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.)
> >
> > Right, (b) can be solved by using a leaf partition's tuple slot as
> > proposed.
>
> You mean what is proposed in [3]?

Yes. Although, I am for assigning a dedicated slot to partitions
*unconditionally*, whereas the PoC patch Andres shared makes it
conditional on either needing tuple conversion between the root and
the partition or having a RETURNING projection present in the query.

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

> > > (I think we could address this issue for v11 independently of [1], off course.)
> >
> > Yeah.
>
> I noticed that I modified your patch incorrectly. Sorry for that.
> Attached is an updated patch fixing that.

Ah, no problem. Thanks for updating.

> I also added a bit of code
> to copy CTID from the new tuple moved to another partition, not just
> table OID, and did some code/comment adjustments, mainly to match
> other places. 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. */?

BTW, do you think we should alter the test in PG 11 branch to test
that system attributes are returned correctly? Once we settle the
other issue, we can adjust the HEAD's test to do likewise?

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