On 2017-12-07 18:32:51 +0900, Michael Paquier wrote: > 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));
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