Re: [PATCH v2] Avoid carrying self lock-only xmax to updated tuple

Hi,

Thank you for the patch. I tested v2 (CF entry 7253) and here are my results.

== Setup ==

- I used the cfbot branch cf/7253, commit
62f9339057248e2e323e075bb2a6cb9630afd024. It is a merge of the v2
patch commit 597d58f into 58aecdd.
- Configure options: --enable-cassert --enable-debug
--enable-tap-tests --without-icu, on Linux (WSL Ubuntu).
- For comparison, I built a second server from the same tree with only
src/backend/access/heap/heapam.c reverted to the base version, so
"unpatched" below means the same code without the heapam.c change.

== Tests that passed ==

- The build finished with 0 compiler warnings.
- "make check" passed: All 243 tests passed. This includes the changed
expected output in update.out.

== Manual tests ==

I ran these on a temporary cluster, on both the patched and the
unpatched server, and looked at the tuples with pageinspect.

1. Your PL/pgSQL loop (SELECT ... FOR UPDATE, then UPDATE, inside a
BEGIN ... EXCEPTION block), run for 3 iterations in one transaction.
- Unpatched: the tuple chain had 4 versions, and 3 of them had a
MultiXact xmax (HEAP_XMAX_IS_MULTI).
- Patched: 4 versions, none with a MultiXact xmax, and the last
version has xmax = 0 with HEAP_XMAX_INVALID.
2. A single transaction doing SELECT ... FOR UPDATE and then UPDATE on
the same row.
- Unpatched: the new version carried xmax = the same XID, with
HEAP_XMAX_KEYSHR_LOCK | HEAP_XMAX_LOCK_ONLY.
- Patched: the new version has xmax = 0 and HEAP_XMAX_INVALID. The
row value was correct after COMMIT (2).
3. INSERT ... ON CONFLICT DO UPDATE on an existing row, in one transaction.
- Unpatched: the new version carried a self lock-only xmax.
- Patched: the new version has xmax = 0.
4. A SAVEPOINT that takes FOR UPDATE, then RELEASE, then UPDATE in the
parent (different XID, so it should keep the old behavior). Patched
and unpatched gave the same flags on the new version
(HEAP_XMAX_KEYSHR_LOCK | HEAP_XMAX_LOCK_ONLY, xmax 678). The old
version showed a MultiXact xmax in both.
5. Foreign key key-share lock from another session. Session B inserted
a child row (FOR KEY SHARE on the parent row) and stayed open. Session
A did a non-key UPDATE of the parent row. Then a key UPDATE and a
DELETE of the parent row, each with lock_timeout = 2s, both failed
with "canceling statement due to lock timeout", waiting on the open
FK-insert transaction. The result was the same on the patched and
unpatched servers, so the remote key-share lock is still protected in
this case.
6. A rough timing of your loop, 3000 iterations, fsync=off, single run
each: about 1.6 s patched and about 6.2 s unpatched. The final counter
value was 3001 in both cases. This is only a quick check on an
assert-enabled build, but it is in line with the speedup you
described.

The patched server's log had no WARNING, ERROR, PANIC or TRAP lines
apart from the expected lock timeouts.

== Not tested ==

- Only "make check" was run. I did not run check-world, the TAP tests,
isolation tests or contrib tests.
- No concurrency stress test, no crash recovery or replication test,
and no pg_upgrade or logical decoding test with the new tuple state.
- No test of VACUUM, freezing or MultiXact wraparound behavior.
- No code review beyond the heapam.c change and the update.out change
shown in run.log.
- I did not test other lock modes or combinations beyond the cases
above (for example FOR SHARE or FOR NO KEY UPDATE taken first), or the
foreign key case where the same transaction both locks and updates.
- Timing was a single short run per build on a debug build, so it is
not a reliable benchmark.
- Only one platform (Linux under WSL) was used.

Regards,
João Rito