Re: problem with RETURNING and update row movement

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

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

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

regards, tom lane