Re: Usage of epoch in txid_current - Mailing list pgsql-hackers

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