Re: In progress INSERT wrecks plans on table - Mailing list pgsql-performance

On 16 June 2013 16:23, Heikki Linnakangas <hlinnakangas@vmware.com> 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@2ndquadrant.com> 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