Re: Usage of epoch in txid_current

Hi,

On 2018-09-19 13:58:36 +1200, Thomas Munro wrote:

> +/*
> + * Advance nextFullXid to the value after a given xid. The epoch is inferred.
> + * If lock_free_check is true, then the caller must be sure that it's safe to
> + * read nextFullXid without holding XidGenLock (ie during recovery).
> + */
> +void
> +AdvanceNextFullTransactionIdPastXid(TransactionId xid, bool lock_free_check)
> +{
> + TransactionId current_xid;
> + uint32 epoch;
> +
> + if (lock_free_check &&
> + !TransactionIdFollowsOrEquals(xid,
> + XidFromFullTransactionId(ShmemVariableCache->nextFullXid)))
> + return;
> +
> + LWLockAcquire(XidGenLock, LW_EXCLUSIVE);
> + current_xid = XidFromFullTransactionId(ShmemVariableCache->nextFullXid);
> + if (TransactionIdFollowsOrEquals(xid, current_xid))
> + {
> + epoch = EpochFromFullTransactionId(ShmemVariableCache->nextFullXid);
> + if (xid < current_xid)
> + ++epoch; /* epoch wrapped */
> + ShmemVariableCache->nextFullXid = MakeFullTransactionId(epoch, xid);
> + FullTransactionIdAdvance(&ShmemVariableCache->nextFullXid);
> + }
> + LWLockRelease(XidGenLock);
> }

Is this really a good idea? Seems like it's going to perpetuate the
problematic epoch logic we had before?

> --- a/src/bin/pg_resetwal/pg_resetwal.c
> +++ b/src/bin/pg_resetwal/pg_resetwal.c
> @@ -431,11 +431,15 @@ main(int argc, char *argv[])
> * if any, includes these values.)
> */
> if (set_xid_epoch != -1)
> - ControlFile.checkPointCopy.nextXidEpoch = set_xid_epoch;
> + ControlFile.checkPointCopy.nextFullXid =
> + MakeFullTransactionId(set_xid_epoch,
> + XidFromFullTransactionId(ControlFile.checkPointCopy.nextFullXid));
>
> if (set_xid != 0)
> {
> - ControlFile.checkPointCopy.nextXid = set_xid;
> + ControlFile.checkPointCopy.nextFullXid =
> + MakeFullTransactionId(EpochFromFullTransactionId(ControlFile.checkPointCopy.nextFullXid),
> + set_xid);
>

> /*
> * For the moment, just set oldestXid to a value that will force
> @@ -705,8 +709,7 @@ GuessControlValues(void)
> ControlFile.checkPointCopy.ThisTimeLineID = 1;
> ControlFile.checkPointCopy.PrevTimeLineID = 1;
> ControlFile.checkPointCopy.fullPageWrites = false;
> - ControlFile.checkPointCopy.nextXidEpoch = 0;
> - ControlFile.checkPointCopy.nextXid = FirstNormalTransactionId;
> + ControlFile.checkPointCopy.nextFullXid = MakeFullTransactionId(0, FirstNormalTransactionId);
> ControlFile.checkPointCopy.nextOid = FirstBootstrapObjectId;
> ControlFile.checkPointCopy.nextMulti = FirstMultiXactId;
> ControlFile.checkPointCopy.nextMultiOffset = 0;
> @@ -786,8 +789,8 @@ PrintControlValues(bool guessed)
> 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"),
> @@ -879,7 +882,7 @@ PrintNewControlValues(void)
> if (set_xid != 0)
> {
> printf(_("NextXID: %u\n"),
> - ControlFile.checkPointCopy.nextXid);
> + XidFromFullTransactionId(ControlFile.checkPointCopy.nextFullXid));
> printf(_("OldestXID: %u\n"),
> ControlFile.checkPointCopy.oldestXid);
> printf(_("OldestXID's DB: %u\n"),
> @@ -889,7 +892,7 @@ PrintNewControlValues(void)
> if (set_xid_epoch != -1)
> {
> printf(_("NextXID epoch: %u\n"),
> - ControlFile.checkPointCopy.nextXidEpoch);
> + EpochFromFullTransactionId(ControlFile.checkPointCopy.nextFullXid));
> }

I still think it's a mistake to not display the full xid here, and
rather perpetuate the epoch stuff. I'm ok with splitting that into a
separate commit, but this ought to be fixed in the same release imo.

> diff --git a/src/include/access/transam.h b/src/include/access/transam.h
> index 83ec3f19797..814becf96d7 100644
> --- 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;

Probably good to note here that not all values are valid normal xids.

> +static inline FullTransactionId
> +MakeFullTransactionId(uint32 epoch, TransactionId xid)
> +{
> + FullTransactionId result;
> +
> + result.value = ((uint64) epoch) << 32 | xid;
> +
> + return result;
> +}

Make sounds a bit like it's allocating...

> /* advance a transaction ID variable, handling wraparound correctly */
> #define TransactionIdAdvance(dest) \
> do { \
> @@ -52,6 +78,16 @@
> (dest) = FirstNormalTransactionId; \
> } while(0)
>
> +/* advance a FullTransactionId variable, stepping over special XIDs */
> +static inline void
> +FullTransactionIdAdvance(FullTransactionId *dest)
> +{
> + dest->value++;
> + if (XidFromFullTransactionId(*dest) < FirstNormalTransactionId)
> + *dest = MakeFullTransactionId(EpochFromFullTransactionId(*dest),
> + FirstNormalTransactionId);
> +}

Hm, this seems pretty odd coding to me. Why not do something like

dest->value++;
while (XidFromFullTransactionId(*dest) < FirstNormalTransactionId)
dest->value++;

That'd a) be a bit more readable, b) possible to do in a lockfree way at
a later point.

> diff --git a/src/include/storage/standby.h b/src/include/storage/standby.h
> index 1fcd8cf1b59..d1116454095 100644
> --- a/src/include/storage/standby.h
> +++ b/src/include/storage/standby.h
> @@ -72,7 +72,7 @@ typedef struct RunningTransactionsData
> int xcnt; /* # of xact ids in xids[] */
> int subxcnt; /* # of subxact ids in xids[] */
> bool subxid_overflow; /* snapshot overflowed, subxids missing */
> - TransactionId nextXid; /* copy of ShmemVariableCache->nextXid */
> + TransactionId nextXid; /* xid from ShmemVariableCache->nextFullXid */

Probably worthwhile to pgindent partially.

Greetings,

Andres Freund