On Thu, Dec 7, 2017 at 1:21 AM, Alvaro Herrera <alvherre@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
