Re: problem with RETURNING and update row movement

On Thu, Apr 22, 2021 at 9:37 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> > FWIW, I think we should go ahead and apply the patches for the bug
> > reported here. Anyone who tries to project an updated tuple's system
> > columns using RETURNING are likely to face problems one way or
> > another, especially if they have partitioned tables containing
> > partitions of varying table AMs, but at least they won't face the bug
> > discussed here.
>
> Agreed, we should get this fixed in time for the next minor releases.

Thanks for taking a look at this.

> The issue no longer exists on HEAD, thanks to 86dc90056 having got
> rid of per-target-relation variance in the contents of planSlot.
> But we still need a fix for the back branches.
>
> So I looked over the v13 patch, and found a couple of things
> I didn't like:
>
> * I think what you did in ExecProcessReturning is buggy. It's
> not a great idea to have completely different processes for
> getting tableoid set in normal-relation vs foreign-relation
> cases, and in this case the foreign-relation case was simply
> wrong. Maybe the bug isn't reachable for lack of support of
> cross-partition motion with FDWs, but I'm not sure about that.

The two cases I see are some callers passing a valid 'tupleSlot' to
ExecProcessReturning() and some (just one!) not. The latter case
applies only to a foreign relations that are directly-modified and
relies on the FDWs having set econtext->ecxt_scantuple to the correct
slot. In the 1st case, the callers should already have made sure that
the correct tableoid is passed through 'tableSlot', although
Fujita-san had noticed in [1] that my earlier patch failed to do that
in the cross-partition update case. Along with fixing the problem of
my patch, he also proposed that we set the tableoid of the slot
assigned to ecxt_scantuple only in the 2nd case to save cycles.

I'm fine with leaving it the way it is as your updated patch does, but
I don't really see a bug being introduced. Actually, the code was
like what Fujita-san proposed we do before b8d71745eac changed it to
the current way:

@@ -166,20 +166,15 @@ ExecProcessReturning(ResultRelInfo *resultRelInfo,
/* Make tuple and any needed join variables available to ExecProject */
if (tupleSlot)
econtext->ecxt_scantuple = tupleSlot;
- else
- {
- HeapTuple tuple;
-
- /*
- * RETURNING expressions might reference the tableoid column, so
- * initialize t_tableOid before evaluating them.
- */
- Assert(!TupIsNull(econtext->ecxt_scantuple));
- tuple = ExecFetchSlotHeapTuple(econtext->ecxt_scantuple, true, NULL);
- tuple->t_tableOid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
- }
econtext->ecxt_outertuple = planSlot;

+ /*
+ * RETURNING expressions might reference the tableoid column, so
+ * reinitialize tts_tableOid before evaluating them.
+ */
+ econtext->ecxt_scantuple->tts_tableOid =
+ RelationGetRelid(resultRelInfo->ri_RelationDesc);
+
/* Compute the RETURNING expressions */
return ExecProject(projectReturning);
}

> We really need to decouple the RETURNING expressions (which
> will belong to the source relation) from the value injected
> for tableoid (which will belong to the destination).

Yeah, that makes sense.

> * I really disliked the API change that ExecInsert is responsible
> for computing RETURNING except when it isn't. That's confusing
> and there's no good reason for it, since it's not really any
> easier to deal with the case at the call site than inside ExecInsert.
>
> In the attached revision I made ExecInsert handle RETURNING
> calculations by asking the callers to pass in the ResultRelInfo
> that should be used for the purpose.

That seems fine to me.

> We could alternatively
> have taken the responsibility for RETURNING out of ExecInsert
> altogether, making the callers call ExecProcessReturning.
> I think that might have netted out slightly simpler than this.
> But we're unlikely to apply such a change in HEAD, so it seemed
> better to keep the division of responsibilities the same as it
> is in other branches.

I agree.

> Thoughts?

The patch looks good to me, except one thing:

/*
+ * In a cross-partition UPDATE with RETURNING, we have to use the
+ * source partition's RETURNING list, because that matches the output
+ * of the planSlot, while the destination partition might have
+ * different resjunk columns. This means we have to map the
+ * destination slot back to the source's format so we can apply that
+ * RETURNING list. This is expensive, but it should be an uncommon
+ * corner case, so we won't spend much effort on making it fast.
+ */
+ if (returningRelInfo != resultRelInfo)
+ {

I think we should also add slot != srcSlot to this condition. The
uncommon corner case is the source and/or the destination partitions
having different column order than the root parent, thus requiring the
tuple to be converted during tuple routing using slots appropriate to
each relation, which causes 'slot' to end up different than 'srcSlot'.
But in the common case, where tuple descriptors match between all
tables involved, 'slot' should be the same as 'srcSlot'.

> (I've not looked at porting this to v12 or v11 yet.)

I did that; patches attached. (I haven't changed them to incorporate
the above comment though.)

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

[1] https://www.postgresql.org/message-id/CAPmGK14QXD5Te_vwGgpuVWXRcrC%2Bd8FyWse0aHSqnDDSeeCRFQ%40mail.gmail.com