On 2013-07-19 13:46:44 -0400, Alvaro Herrera wrote:
> levertond@googlemail.com wrote:
>
> > START TRANSACTION ISOLATION LEVEL SERIALIZABLE;
> > CREATE TABLE testing(
> > x INTEGER PRIMARY KEY
> > );
> > INSERT INTO testing VALUES(1);
> > SELECT * FROM testing WHERE x = 1 FOR UPDATE;
> > SAVEPOINT test;
> > UPDATE testing SET x = 2 WHERE x = 1;
> > ROLLBACK TO test;
> > UPDATE testing SET x = 3 WHERE x = 1;
> > ROLLBACK;
>
> Thanks for the test case. This one-liner patch fixes it:
>
> diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
> index 55563ea..fa9c9d7 100644
> --- a/src/backend/utils/time/tqual.c
> +++ b/src/backend/utils/time/tqual.c
> @@ -1287,7 +1287,7 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin,
> {
> if (tuple->t_infomask & HEAP_XMAX_INVALID) /* xid invalid */
> return HEAPTUPLE_INSERT_IN_PROGRESS;
> - if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
> + if (HeapTupleHeaderIsOnlyLocked(tuple))
> return HEAPTUPLE_INSERT_IN_PROGRESS;
> /* inserted and then deleted by same xact */
> return HEAPTUPLE_DELETE_IN_PROGRESS;
>
>
> That is, I was taking a shortcut here by checking only the infomask bits
> about locking, and not resolving the MultiXact to see if the updater
> (sub)transaction was aborted; but this was wrong, because a tuple which
> was created here and updated by an aborted subxact needs to be seen as
> in-progress insertion, not an in-progress delete. This causes trouble
> to the predicate.c code because it tries to obtain the update XID for
> the tuple, but since the update has aborted, that returned zero, causing
> the assert failure.
> Sadly, this has performance implications, because what previously was
> just an in-place check of bit flags has now become a function call.
Well, the impact imo primarily comes from actually resolving the
multixact, not from the function call itself... But I don't think we
need to overly worried. That path is only entered if xmin is
in-progress, so that shouldn't have too big implications.
If you're worried you could do a SetHintBits(HEAP_XMAX_INVALID) like
it's done if xmin isn't in progress like it's done when xmin has
committed.
> Perhaps it'd be smart to optimize it a bit so that we first check the
> flags, and only call the function if that fails.
Sounds like a good idea to me. The duplicated amount of work by that
should by fairly minimal.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
