On Tue, Jul 10, 2018 at 1:30 PM, Andres Freund <and...@anarazel.de> wrote: > On 2018-07-10 13:20:52 +1200, Thomas Munro wrote: >> I don't know what to think about the encoding or meaning of non-normal >> xids in this thing. > > I'm not following what you mean by this?
While defining FullTransactionIdPrecedes() in the image of TransactionIdPrecedes(), I wondered whether the xid component of a FullTransactionId would ever be one of the special values below FirstNormalTransactionId that require special treatment. The question doesn't actually arise in this code, however, so I ignored that thought and simply used x < y. >> diff --git a/src/backend/access/transam/subtrans.c >> b/src/backend/access/transam/subtrans.c >> index 4faa21f5aef..fa0847afc81 100644 >> --- a/src/backend/access/transam/subtrans.c >> +++ b/src/backend/access/transam/subtrans.c >> @@ -261,7 +261,7 @@ StartupSUBTRANS(TransactionId oldestActiveXID) >> LWLockAcquire(SubtransControlLock, LW_EXCLUSIVE); >> >> startPage = TransactionIdToPage(oldestActiveXID); >> - endPage = TransactionIdToPage(ShmemVariableCache->nextXid); >> + endPage = >> TransactionIdToPage(XidFromFullTransactionId(ShmemVariableCache->nextFullXid)); > > These probably need an intermediate variable. Ok, did that in a couple of places. >> diff --git a/src/backend/access/transam/varsup.c >> b/src/backend/access/transam/varsup.c >> index 394843f7e91..13020f54d98 100644 >> --- a/src/backend/access/transam/varsup.c >> +++ b/src/backend/access/transam/varsup.c >> @@ -73,7 +73,7 @@ GetNewTransactionId(bool isSubXact) >> >> LWLockAcquire(XidGenLock, LW_EXCLUSIVE); >> >> - xid = ShmemVariableCache->nextXid; >> + xid = XidFromFullTransactionId(ShmemVariableCache->nextFullXid); > > It's a bit over the top, I know, but I'd move the conversion to outside > the lock. Less because it makes a meaningful efficiency difference, and > more because I find it clearer, and easier to reason about if we ever go > to atomically incrementing nextFullXid. Erm -- I can't read nextFullXid until I have the lock, and then I need it in a 32 bit format for the code that follows immediately (I don't currently have xidVacLimit in FullTransactionId format). I'm not sure what you mean. >> @@ -6700,7 +6698,8 @@ StartupXLOG(void) >> wasShutdown ? "true" >> : "false"))); >> ereport(DEBUG1, >> (errmsg_internal("next transaction ID: %u:%u; next >> OID: %u", >> - >> checkPoint.nextXidEpoch, checkPoint.nextXid, >> + >> EpochFromFullTransactionId(checkPoint.nextFullXid), >> + >> XidFromFullTransactionId(checkPoint.nextFullXid), >> checkPoint.nextOid))); > > I don't think we should continue to expose epochs, but rather go to full > xids where appropriate. OK, done. Hmm, that's going to hurt some eyeballs, because other fields are shown in 32 bit xid format. Should we also go to hex everywhere so that we feeble humans can see the epoch component and compare the xid component by eye? Here's what I see on my test cluster: Latest checkpoint's NextXID: 184683724519 Latest checkpoint's oldestXID: 4294901760 In hexadecimal money that'd be (with extra spaces, to line up with a monospace font): Latest checkpoint's NextXID: 0000002b0001fee7 Latest checkpoint's oldestXID: ffff0000 I left pg_resetwal's arguments -x and -e alone, but I suppose someone might want a new switch that does both together... >> +/* advance a FullTransactionId lvalue, handling wraparound correctly */ >> +#define FullTransactionIdAdvance(dest) \ >> + do { \ >> + (dest).value++; \ >> + if (XidFromFullTransactionId(dest) < FirstNormalTransactionId) >> \ >> + (dest) = >> MakeFullTransactionId(EpochFromFullTransactionId(dest), \ >> + >> FirstNormalTransactionId); \ >> + } while (0) > > Yikes. Why isn't this an inline function? Because it assigns to dest? Following existing malpractice, and yeah because it requires an lvalue so can't be changed without a different convention at the call site. Fixed. -- Thomas Munro http://www.enterprisedb.com
0001-Track-the-next-xid-using-64-bits-v3.patch
Description: Binary data