Horton

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

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, "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-14T22:00:17Z
Lists: pgsql-hackers 
On 2017-12-07 18:32:51 +0900, Michael Paquier wrote:
> On Thu, Dec 7, 2017 at 5:23 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > Looking at 0002: I agree with the stuff being done here.
> 
> The level of details you are providing with a proper error code is an
> improvement over the first version proposed in my opinion.
> 
> > I think a
> > couple of these checks could be moved one block outerwards in term of
> > scope; I don't see any reason why the check should not apply in that
> > case. I didn't catch any place missing additional checks.
> 
> In FreezeMultiXactId() wouldn't it be better to issue an error as well
> for this assertion?
> Assert(!TransactionIdPrecedes(members[i].xid, cutoff_xid));

I'm not really concerned that much about pure lockers, they don't cause
permanent data corruption...


> > Despite these being "shouldn't happen" conditions, I think we should
> > turn these up all the way to ereports with an errcode and all, and also
> > report the XIDs being complained about.  No translation required,
> > though.  Other than those changes and minor copy editing a commit
> > (attached), 0002 looks good to me.

If you want to go around doing that in some more places we can do so in
master only...


> +           if (!(tuple->t_infomask & HEAP_XMAX_LOCK_ONLY) &&
> +               TransactionIdDidCommit(xid))
> +               ereport(ERROR,
> +                       (errcode(ERRCODE_DATA_CORRUPTED),
> +                        errmsg("can't freeze committed xmax %u", xid)));
> The usual wording used in errmsg is not the "can't" but "cannot".
> 
> +               ereport(ERROR,
> +                       (errcode(ERRCODE_DATA_CORRUPTED),
> +                        errmsg_internal("uncommitted Xmin %u from
> before xid cutoff %u needs to be frozen",
> +                                        xid, cutoff_xid)));
> "Xmin" I have never seen, but "xmin" I did.

Changed...

Greetings,

Andres Freund

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.