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 
>
> +#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