Horton

Re: [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: Peter Geoghegan <pg@bowt.ie>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, "Wood, Dan" <hexpert@amazon.com>
Date: 2017-10-04T00:15:44Z
Lists: pgsql-hackers 
On Wed, Oct 4, 2017 at 8:10 AM, Peter Geoghegan <pg@bowt.ie> wrote:
> I now think that it actually is a VACUUM problem, specifically a
> problem with VACUUM pruning. You see the HOT xmin-to-xmax check
> pattern that you mentioned within heap_prune_chain(), which looks like
> where the incorrect tuple prune (or possibly, at times, redirect?)
> takes place. (I refer to the prune/kill that you mentioned today, that
> frustrated your first attempt at a fix -- "I modified the multixact
> freeze code...".)

My lookup of the problem converges to the same conclusion. Something
is wrong with the vacuum's pruning. I have spent some time trying to
design a patch, all the solutions I tried have proved to make the
problem harder to show up, but it still showed up, sometimes after
dozens of attempts.

> The attached patch "fixes" the problem -- I cannot get amcheck to
> complain about corruption with this applied. And, "make check-world"
> passes. Hopefully it goes without saying that this isn't actually my
> proposed fix. It tells us something that this at least *masks* the
> problem, though; it's a start.

Yep.

> FYI, the repro case page contents looks like this with the patch applied:
> postgres=# select lp, lp_flags, t_xmin, t_xmax, t_ctid,
> to_hex(t_infomask) as infomask,
> to_hex(t_infomask2) as infomask2
> from heap_page_items(get_raw_page('t', 0));
>  lp | lp_flags | t_xmin  | t_xmax | t_ctid | infomask | infomask2
> ----+----------+---------+--------+--------+----------+-----------
>   1 |        1 | 1845995 |      0 | (0,1)  | b02      | 3
>   2 |        2 |         |        |        |          |
>   3 |        0 |         |        |        |          |
>   4 |        0 |         |        |        |          |
>   5 |        0 |         |        |        |          |
>   6 |        0 |         |        |        |          |
>   7 |        1 | 1846001 |      0 | (0,7)  | 2b02     | 8003
> (7 rows)

Is lp_off for tid (0,2) pointing to (0,7)? A hot chain preserved is
what would look correct to me.

-         * Check the tuple XMIN against prior XMAX, if any
-         */
-        if (TransactionIdIsValid(priorXmax) &&
-            !TransactionIdEquals(HeapTupleHeaderGetXmin(htup), priorXmax))
-            break;
If you remove this check, you could also remove completely priorXmax.

Actually, I may be missing something, but why is priorXmax updated
even for dead tuples? For example just doing that is also taking care
of the problem:
@@ -558,7 +558,8 @@ heap_prune_chain(Relation relation, Buffer buffer,
OffsetNumber rootoffnum,
        Assert(ItemPointerGetBlockNumber(&htup->t_ctid) ==
               BufferGetBlockNumber(buffer));
        offnum = ItemPointerGetOffsetNumber(&htup->t_ctid);
-       priorXmax = HeapTupleHeaderGetUpdateXid(htup);
+       if (!tupdead)
+           priorXmax = HeapTupleHeaderGetUpdateXid(htup);
    }
-- 
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.