Re: Usage of epoch in txid_current
От | Thomas Munro |
---|---|
Тема | Re: Usage of epoch in txid_current |
Дата | |
Msg-id | CA+hUKGJdoJCT2n83+cGCeez0TiRV8gEoJYVQbeF_uzKE1Xkq8Q@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Usage of epoch in txid_current (Andres Freund <andres@anarazel.de>) |
Ответы |
Re: Usage of epoch in txid_current
|
Список | pgsql-hackers |
On Mon, Feb 4, 2019 at 8:41 PM Andres Freund <andres@anarazel.de> wrote: > 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? We could get rid of this in future, if certain WAL records and 2PC state start carrying full xids. That would be much bigger surgery than I wanted to do in this patch. This is the only place that converts 32 bit -> 64 bit with an inferred epoch component. I have explained why I think that's correct in new comments along with a new assertion. The theory is that the xids encountered in recovery and 2PC startup can never be too far out of phase with the current nextFullXid. In contrast, the original epoch tracking from commit 35af5422 wasn't bounded in the same sort of way. Certainly no other code should be allowed to do this sort of thing without very careful consideration of how the epoch is bounded. The patch deliberately provides no general purpose make-me-a-FullTransactionId-by-guessing-the-epoch facility. While reviewing this, I also realised that the "lock_free_check" function argument was unnecessary. The only place that was setting that to false, namely RecordKnownAssignedTransactionIds(), might as well just use the same behaviour. I've now rewritten AdvanceNextFullTransactionIdPastXid() completely in light of all that; please review. > > 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)); > 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. Ok. > > +/* > > + * 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. Done. > > +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... Changed to FullTransactionIdFromEpochAndXid(). > > + dest->value++; > > + if (XidFromFullTransactionId(*dest) < FirstNormalTransactionId) > > + *dest = MakeFullTransactionId(EpochFromFullTransactionId(*dest), > 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. Done. > > - TransactionId nextXid; /* copy of ShmemVariableCache->nextXid */ > > + TransactionId nextXid; /* xid from ShmemVariableCache->nextFullXid */ > > Probably worthwhile to pgindent partially. Done. I also added FullTransactionId to typedefs.list, bumped PG_CONTROL_VERSION and did some peformance testing of tight loops of GetNewTransactionId(), which showed no measurable change (~10 million single-threaded calls per second either way on the machine I tested). New version attached. I'd like to commit this for PG12. -- Thomas Munro https://enterprisedb.com
Вложения
В списке pgsql-hackers по дате отправления: