On 16 June 2013 16:23, Heikki Linnakangas <hlinnakangas@xxxxxxxxxx> wrote: > On 06.05.2013 04:51, Mark Kirkwood wrote: >> >> On 05/05/13 00:49, Simon Riggs wrote: >>> >>> On 3 May 2013 13:41, Simon Riggs <simon@xxxxxxxxxxxxxxx> wrote: >>> >>>> (3) to make the check on TransactionIdIsInProgress() into a heuristic, >>>> since we don't *need* to check that, so if we keep checking the same >>>> xid repeatedly we can reduce the number of checks or avoid xids that >>>> seem to be long running. That's slightly more coding than my quick >>>> hack here but seems worth it. >>>> >>>> I think we need both (1) and (3) but the attached patch does just (1). >>>> >>>> This is a similar optimisation to the one I introduced for >>>> TransactionIdIsKnownCompleted(), except this applies to repeated >>>> checking of as yet-incomplete xids, and to bulk concurrent >>>> transactions. >>> >>> >>> ISTM we can improve performance of TransactionIdIsInProgress() by >>> caching the procno of our last xid. >>> >>> Mark, could you retest with both these patches? Thanks. >>> >> >> Thanks Simon, will do and report back. > > > Did anyone ever try (3) ? No, because my other patch meant I didn't need to. In other words, my other patch speeded up repeated access enough I didn't care about (3) anymore. > I'm not sure if this the same idea as (3) above, but ISTM that > HeapTupleSatisfiesMVCC doesn't actually need to call > TransactionIdIsInProgress(), because it checks XidInMVCCSnapshot(). The > comment at the top of tqual.c says: > >> * NOTE: must check TransactionIdIsInProgress (which looks in PGXACT >> array) >> * before TransactionIdDidCommit/TransactionIdDidAbort (which look in >> * pg_clog). Otherwise we have a race condition: we might decide that a >> * just-committed transaction crashed, because none of the tests succeed. >> * xact.c is careful to record commit/abort in pg_clog before it unsets >> * MyPgXact->xid in PGXACT array. That fixes that problem, but it also >> * means there is a window where TransactionIdIsInProgress and >> * TransactionIdDidCommit will both return true. If we check only >> * TransactionIdDidCommit, we could consider a tuple committed when a >> * later GetSnapshotData call will still think the originating transaction >> * is in progress, which leads to application-level inconsistency. >> The >> * upshot is that we gotta check TransactionIdIsInProgress first in all >> * code paths, except for a few cases where we are looking at >> * subtransactions of our own main transaction and so there can't be any >> * race condition. > > > If TransactionIdIsInProgress() returns true for a given XID, then surely it > was also running when the snapshot was taken (or had not even began yet). In > which case the XidInMVCCSnapshot() call will also return true. Am I missing > something? > > There's one little problem: we currently only set the hint bits when > TransactionIdIsInProgress() returns false. If we do that earlier, then even > though HeapTupleSatisfiesMVCC works correctly thanks to the > XidInMVCCSnapshot call, other HeapTupleSatisfies* functions that don't call > XIdInMVCCSnapshot might see the tuple as committed or aborted too early, if > they see the hint bit as set while the transaction is still in-progress > according to the proc array. Would have to check all the callers of those > other HeapTupleSatisfies* functions to verify if that's OK. Well, I looked at that and its too complex and fiddly to be worth it, IMHO. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-performance mailing list (pgsql-performance@xxxxxxxxxxxxxx) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-performance
