On Thu, Dec 7, 2017 at 1:21 AM, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote: > Put together, I propose the attached delta for 0001.
I have been looking at Andres' 0001 and your tweaks here for some time since yesterday... I have also performed sanity checks using all the scripts that have accumulated on my archives for this stuff. This looks solid to me. I have not seen failures with broken hot chains, REINDEX, etc. > This way, an xmax that has exactly the OldestXmin value will return > RECENTLY_DEAD rather DEAD, which seems reasonable to me (since > OldestXmin value itself is supposed to be still possibly visible to > somebody). Also, this way it is consistent with the other comparison to > OldestXmin at the bottom of the function. There is no reason for the > "else" or the extra braces. +1. It would be nice to add a comment in the patched portion mentioning that the new code had better match what is at the bottom of the function. + else if (!MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple), false)) + { + /* + * Not in Progress, Not Committed, so either Aborted or crashed. + * Mark the Xmax as invalid. + */ + SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId); } - /* - * Not in Progress, Not Committed, so either Aborted or crashed. - * Remove the Xmax. - */ - SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId); return HEAPTUPLE_LIVE; I would find cleaner if the last "else if" is put into its own separate if condition, and that for a multixact still running this refers to an updating transaction aborted so hint bits are not set. > Your commit message does a poor job of acknowledging prior work on > diagnosing the problem starting from Dan's initial test case and patch. (Nit: I have extracted from the test case of Dan an isolation test, which Andres has reduced to a subset of permutations. Peter G. also complained about what is visibly the same bug we are discussing here but without a test case.) -- Michael