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: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: "Wong, Yi Wen" <yiwong@amazon.com>, Peter Geoghegan <pg@bowt.ie>, "Wood, Dan" <hexpert@amazon.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2017-10-05T01:31:43Z
Lists: pgsql-hackers 
On Wed, Oct 4, 2017 at 10:46 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Wong, Yi Wen wrote:
>> My interpretation of README.HOT is the check is just to ensure the chain is continuous; in which case the condition should be:
>>
>> >                 if (TransactionIdIsValid(priorXmax) &&
>> >                         !TransactionIdEquals(priorXmax, HeapTupleHeaderGetRawXmin(htup)))
>> >                         break;
>>
>> So the difference is GetRawXmin vs GetXmin, because otherwise we get the FreezeId instead of the Xmin when the transaction happened
>
> I independently arrived at the same conclusion.  Since I was trying with
> 9.3, the patch differs -- in the old version we must explicitely test
> for the FrozenTransactionId value, instead of using GetRawXmin.
> Attached is the patch I'm using, and my own oneliner test (pretty much
> the same I posted earlier) seems to survive dozens of iterations without
> showing any problem in REINDEX.

Confirmed, the problem goes away with this patch on 9.3.

> This patch is incomplete, since I think there are other places that need
> to be patched in the same way (EvalPlanQualFetch? heap_get_latest_tid?).
> Of course, for 9.4 and onwards we need to patch like you described.

I have just done a lookup of the source code, and here is an
exhaustive list of things in need of surgery:
- heap_hot_search_buffer
- heap_get_latest_tid
- heap_lock_updated_tuple_rec
- heap_prune_chain
- heap_get_root_tuples
- rewrite_heap_tuple
- EvalPlanQualFetch (twice)

> This bit in EvalPlanQualFetch caught my attention ... why is it saying
> xmin never changes?  It does change with freezing.
>
>                         /*
>                          * If xmin isn't what we're expecting, the slot must have been
>                          * recycled and reused for an unrelated tuple.  This implies that
>                          * the latest version of the row was deleted, so we need do
>                          * nothing.  (Should be safe to examine xmin without getting
>                          * buffer's content lock, since xmin never changes in an existing
>                          * tuple.)
>                          */
>                         if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple.t_data),
>                                                                          priorXmax))

Agreed. That's not good.
-- 
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.