Horton

Re: [HACKERS] [COMMITTERS] pgsql: Fix freezing of a dead HOT-updated tuple

Michael Paquier <michael.paquier@gmail.com>

View thread View on postgr.es
From: Michael Paquier <michael.paquier@gmail.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>, "Wood, Dan" <hexpert@amazon.com>, Peter Geoghegan <pg@bowt.ie>, pgsql-committers@postgresql.org, "Wong, Yi Wen" <yiwong@amazon.com>, Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Date: 2017-12-07T03:08:38Z
Lists: pgsql-hackers 
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

Commits

  1. Backport addition of rs_old_rel to rewriteheap's state.

  2. Perform a lot more sanity checks when freezing tuples.

  3. Revert bogus fixes of HOT-freezing bug

  4. Fix traversal of half-frozen update chains

  5. Fix freezing of a dead HOT-updated tuple

  6. During index build, check and elog (not just Assert) for broken HOT chain.

  7. Fix WAL replay of locking an updated tuple

  8. Change the way we mark tuples as frozen.