Re: [PATCH] Corruption Issue: Fix missing tts_tid in ExecForceStoreHeapTuple

Hello.

v8 attached, code/fix is unchanged from v7 but just some cleanup of things
I'd overlooked or misunderstood so a new commit message and some additional
tests.

First, the tid2.sql case *is* fixed by this patch. I said otherwise, and
I'm sorry for the confusion. I'd tested against the wrong build, and then
misread EXPLAIN VERBOSE: the routed INSERT shows no "Remote SQL ...
RETURNING", which I took to mean ctid wasn't being requested. That remote
statement is built at execution time in postgresBeginForeignInsert(),
which passes ri_returningList through to deparseReturningList(), so ctid
is requested and make_tuple_from_result_row() does fill in t_self. What
lost it was the else (virtual-slot) branch of ExecForceStoreHeapTuple()
not setting tts_tid, which is exactly what moving the assignment to the
end (as you had suggested Andres) fixes.

Testing against a fresh build:

INSERT INTO parent_rows VALUES (2) RETURNING a, ctid;
v8: 2 | (0,2)fixes
execTuples.c reverted: 2 | (4294967295,0)

v8 drops the paragraph in v7's commit message that claimed this was a
separate postgres_fdw problem, and adds your case as a regression test
in postgres_fdw.sql, next to the existing insert tuple routing tests.
It inserts into itrtest and checks every returned ctid against
loct1/loct2, so as not to depend on page layout.

While writing it I found that only one of the two partitions in that
test shows the bug: remp1, whose columns match the parent, gets a virtual
result slot. remp2 declares (b, a), so its insert goes through a tuple
conversion map and a heap slot, which was already correct. The test
therefore reports one missing ctid rather than two without the fix.

Second, in my last mail I answered your cross-partition update comment
with "The postgres_fdw side looks like a separate patch". That attached
the FDW case to the wrong issue. As far as I can tell they're unrelated:
the FDW case was this bug, while

> cross-partition updates involving ctids just silently cause data to be
> inconsistent with the partition constraints :(.

is something else. I tried a few obvious shapes, a ctid-keyed UPDATE that
moves the row, a ctid-only predicate that matches (0,1) in two
partitions, and a ctid taken from a subquery against another partition,
and they all came out consistent with the partition constraints. So I
haven't found your case. Could you share what you ran? I'm happy to work
on it in a new thread.

On attribution, I'll answer my own question from last time, I've used
Co-authored-by: me and Virender, and Reported-by: you for the FDW
path. Tell me if you'd rather it were different.

Backpatching: v8 applies as-is to master and 15 through 18. On 14 the
execTuples.c hunk applies, but both test hunks need placing by hand
because gist.sql and postgres_fdw.sql have grown since. Both blocks are
self-contained and go in unchanged. I built 14 with --enable-cassert and
the placed postgres_fdw test passes with the fix and fails without it
(ctid_not_found 0 -> 1), the same as on master.

Apologies for rushing out the last email.

best.

-greg