Re: Usage of epoch in txid_current

On Tue, Jul 10, 2018 at 1:30 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2018-07-10 13:20:52 +1200, Thomas Munro wrote:
>> 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?

While defining FullTransactionIdPrecedes() in the image of
TransactionIdPrecedes(), I wondered whether the xid component of a
FullTransactionId would ever be one of the special values below
FirstNormalTransactionId that require special treatment. The question
doesn't actually arise in this code, however, so I ignored that
thought and simply used x < y.

>> 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.

Ok, did that in a couple of places.

>> 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.

Erm -- I can't read nextFullXid until I have the lock, and then I need
it in a 32 bit format for the code that follows immediately (I don't
currently have xidVacLimit in FullTransactionId format). I'm not sure
what you mean.

>> @@ -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.

OK, done.

Hmm, that's going to hurt some eyeballs, because other
fields are shown in 32 bit xid format. Should we also go to hex
everywhere so that we feeble humans can see the epoch component and
compare the xid component by eye?

Here's what I see on my test cluster:

Latest checkpoint's NextXID: 184683724519
Latest checkpoint's oldestXID: 4294901760

In hexadecimal money that'd be (with extra spaces, to line up with a
monospace font):

Latest checkpoint's NextXID: 0000002b0001fee7
Latest checkpoint's oldestXID: ffff0000

I left pg_resetwal's arguments -x and -e alone, but I suppose someone
might want a new switch that does both together...

>> +/* 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?

Following existing malpractice, and yeah because it requires an lvalue
so can't be changed without a different convention at the call site.
Fixed.

--
Thomas Munro
http://www.enterprisedb.com