On 2018-07-10 13:20:52 +1200, Thomas Munro wrote: > defined in transam.h because c.h didn't feel right.
Yea, that looks better. > Client code lost the ability to use operator < directly. I needed to > use a static inline function as a constructor. I lost the > interchangeability with the wide xids in txid.c, so I provided > U64FromFullTransactionId() (I think that'll be useful for > serialisation over the write too). Should probably, at a later point, introduce a SQL type for it too. > 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? > 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. > 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. > @@ -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. > diff --git a/src/bin/pg_controldata/pg_controldata.c > b/src/bin/pg_controldata/pg_controldata.c > index 895a51f89d5..f6731bfd28d 100644 > --- a/src/bin/pg_controldata/pg_controldata.c > +++ b/src/bin/pg_controldata/pg_controldata.c > @@ -20,6 +20,7 @@ > > #include <time.h> > > +#include "access/transam.h" > #include "access/xlog.h" > #include "access/xlog_internal.h" > #include "catalog/pg_control.h" > @@ -256,8 +257,8 @@ main(int argc, char *argv[]) > printf(_("Latest checkpoint's full_page_writes: %s\n"), > ControlFile->checkPointCopy.fullPageWrites ? _("on") : > _("off")); > printf(_("Latest checkpoint's NextXID: %u:%u\n"), > - ControlFile->checkPointCopy.nextXidEpoch, > - ControlFile->checkPointCopy.nextXid); > + > EpochFromFullTransactionId(ControlFile->checkPointCopy.nextFullXid), > + > XidFromFullTransactionId(ControlFile->checkPointCopy.nextFullXid)); > printf(_("Latest checkpoint's NextOID: %u\n"), > ControlFile->checkPointCopy.nextOid); > printf(_("Latest checkpoint's NextMultiXactId: %u\n"), See above re exposing epoch. > --- a/src/include/access/transam.h > +++ b/src/include/access/transam.h > @@ -44,6 +44,32 @@ > #define TransactionIdStore(xid, dest) (*(dest) = (xid)) > #define StoreInvalidTransactionId(dest) (*(dest) = InvalidTransactionId) > > +#define EpochFromFullTransactionId(x) ((uint32) ((x).value >> 32)) > +#define XidFromFullTransactionId(x) ((uint32) (x).value) > +#define U64FromFullTransactionId(x) ((x).value) > +#define FullTransactionIdPrecedes(a, b) ((a).value < (b).value) > +#define FullTransactionIdPrecedesOrEquals(a, b) ((a).value <= (b).value) > + > +/* > + * A 64 bit value that contains an epoch and a TransactionId. This is > + * wrapped in a struct to prevent implicit conversion to/from TransactionId. > + * Allowing such conversions seems likely to be error-prone. > + */ > +typedef struct FullTransactionId > +{ > + uint64 value; > +} FullTransactionId; > + > +static inline FullTransactionId > +MakeFullTransactionId(uint32 epoch, TransactionId xid) > +{ > + FullTransactionId result; > + > + result.value = ((uint64) epoch) << 32 | xid; > + > + return result; > +} > + > /* advance a transaction ID variable, handling wraparound correctly */ > #define TransactionIdAdvance(dest) \ > do { \ > @@ -52,6 +78,15 @@ > (dest) = FirstNormalTransactionId; \ > } while(0) > > +/* 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? Greetings, Andres Freund