Re: In progress INSERT wrecks plans on table

On 16 June 2013 16:23, Heikki Linnakangas <hlinnakangas(at)vmware(dot)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(at)2ndquadrant(dot)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