On Thu, Dec 7, 2017 at 5:23 AM, Alvaro Herrera <alvhe...@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