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@alvh.no-ip.org>
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-07T09:32:51Z
Lists: pgsql-hackers 
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));

> 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 (!(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.

> I started thinking it'd be good to report block number whenever anything
> happened while scanning the relation.  The best way to go about this
> seems to be to add an errcontext callback to lazy_scan_heap, so I attach
> a WIP untested patch to add that.  (I'm not proposing this for
> back-patch for now, mostly because I don't have the time/energy to push
> for it right now.)

I would recommend to start a new thread and to add that patch to the
next commit fest as you would get more visibility and input from other
folks on -hackers. It looks like a good idea to me.
-- 
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.