Re: Usage of epoch in txid_current

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