Re: Re: [PATCH] Discard aborted updaters when expanding a multixact

Hi,

Attached is v3, rebased onto master at fcc0e27f45e2d8dc6046435908a48626c91d3d84.

The v2 change in MultiXactIdExpand() fixes the two-updater ERROR, but
I found a case it cannot reach. T1 performs a non-key update while T2
takes a compatible FOR KEY SHARE lock and prepares its transaction.
The server then crashes before T1 commits. After recovery, T1's update
has not committed and the original row is still visible. However, a
REPEATABLE READ update by T3 gets "could not serialize access due to
concurrent update": TransactionIdDidAbort(T1) is false because the
crashed updater has no abort record, so heap_update() returns TM_Updated
before MultiXactIdExpand() is reached. Filtering members in Expand
cannot fix this result.

Patch 0001 addresses the original abort-window race at its earlier
conflict check: an updater is skipped only when it is no longer in
progress and did not commit. A conflicting operation therefore waits
until the abort finishes. MultiXactIdExpand()'s existing filter then
discards the aborted member, so the v2 change there is no longer needed.

heap_update() has a similar problem in its own updater-status check.
Like the conflict check changed by patch 0001, it uses
TransactionIdDidAbort(), which cannot identify an updater that crashed
without recording an abort. Such an updater is no longer running and
never committed, but TransactionIdDidAbort() still returns false.

Patch 0001 skips that updater when checking conflicts, but does not fix
heap_update()'s subsequent decision. The surviving prepared locker
makes HeapTupleSatisfiesUpdate() return TM_BeingModified, so
heap_update() still reaches its own updater-status check. Its
can_continue flag remains false and it reports TM_Updated for an update
that never committed. This causes the REPEATABLE READ error above and
can also trigger the executor's tmfd.traversed assertion at READ
COMMITTED.

Patch 0002 changes that heap_update() check to !TransactionIdDidCommit(),
after the updater has been established as no longer running.

I also checked the MultiXact-related TransactionIdDidAbort()/
TransactionIdDidCommit() calls for missing TransactionIdIsInProgress()
checks or equivalent completion guarantees, leaving
heap_lock_updated_tuple_rec() as the only remaining case to examine.

In this function, TransactionIdDidAbort() checks the successor's xmin.
An explicit abort correctly stops the chain walk. If the creator
crashed without an abort record, the walk can instead lock a dead
successor and generate extra WAL. In the cases analyzed and reproduced,
the live predecessor was still locked correctly, and subsequent updates
and pruning handled the dead successor correctly. I found no
user-visible correctness issue in those cases, so I have left this
path unchanged in v3.

Patch 0003 adds an isolation test for the abort-window race and a
recovery test for heap_update()'s crashed-updater case.

I ran the isolation test with assertions and injection points enabled on
fcc0e27f45e2d8dc6046435908a48626c91d3d84. All four permutations passed.
With only the v2 MultiXactIdExpand() change, the DELETE and key-changing
UPDATE cases reproduce assertion failures.

Regards,
Chee Wooson