Re: HeapTupleSatisfiesDirty fails to test HEAP_XMAX_IS_LOCKED_ONLY for TransactionIdIsInProgress(...)

Craig Ringer wrote:

> A SELECT ... FOR SHARE will incorrectly block on another open
> transaction that ran SELECT ... FOR SHARE and still holds the locks if
> it has to follow a ctid chain from the current snapshot through a
> committed update to a share-locked tuple.
>
> This also affects uniqueness checks in btrees, where it can cause
> unnecessary waiting. It's also an issue with FOR KEY UPDATE, in that it
> can cause an update to block when it doesn't have to.

Interesting bug. Thanks for the patch. I have applied it all the way
back to 8.4 (with adjustments for 9.2 and beyond).

> The attached test case runs under isolationtester to exersise the
> problem. I've tested it against 9.2, 9.3, and HEAD, but Andres looked
> over the code and says the underlying bug goes back to the commit of
> MVCC, it's just become easier to trigger. To reliably test this with
> isolationtester I had to horribly abuse pg_advisory_lock(...) so that I
> could force the first SELECT ... FOR UPDATE to acquire its snapshot
> before the UPDATE runs.

I didn't apply the test case. I think if we want to test tqual.c
behavior we will need to introduce a large battery of tests. I would
like to see more opinions on whether that's something we want.

> A backtrace of the point where it's incorrectly blocked is attached.
> What's happening is that the test for TransactionIdIsInProgress
> unconditionally sets snapshot->xmax, even if xmax was only set for
> locking purposes. This will cause the caller to wait for the xid in xmax
> when it doesn't need to.

Yeah, after actually going over the code I think the bug is clear. (I
was initially unsure about SatisfiesDirty returning true not false for
this case; but the return value was correct, only snapshot->xmax was
being set incorrectly. If you examine the callers they would misbehave
if the return value were changed; for example EvalPlanQualFetch would
completely skip the tuple if SatisfiesDirty returned false, which is not
what we want; we want the tuple to be processed.) I think the comments
on what exactly SatisfiesDirty does about in-progress transactions ought
to be more specific.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services