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

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