Re: SetBufferCommitInfoNeedsSave and race conditions

Pavan Deolasee wrote:
> During one of HOT stress tests, an asserition failed at tqual.c:1178
> in HeapTupleSatisfiesVacuum(). The assertion failure looked really
> strange because the assertion checks for HEAP_XMAX_COMMITTED
> which we set just couple of lines above. I inspected the core dump
> and found that the flag is *set* properly. That was even more strange.
> I confirmed that we are holding a SHARE lock on the buffer as we
> do at several other places while checking/setting the infomask bits.
> 
> We had a theory that somebody clears the flag after the asserting
> process sets it and before it checks it. The other process also sets it
> back before core dump is generated because core shows the flag
> being set properly. The chances of this happening are very slim and
> can further be ruled out because I carefully looked at the code and found
> that the flag can only be cleared holding an exclusive lock on the buffer.
> 
> So we suspected an interaction between multiple processes each holding
> a SHARE lock and setting/checking different bits in the infomask and
> we could theoritically say that such interaction can potentially lead to
> missing hint bit updates. I can think of the following:

FWIW, this can be reproduced by single-stepping with a debugger:

First, you need a tuple that's committed dead but no hint bits have been 
set:

BEGIN; truncate foo; INSERT INTO foo values (1,'foo'); DELETE FROM Foo; 
commit;

In one backend, set a breakpoint to HeapTupleSatisfiesMVCC lin 953 where 
it sets the XMIN_COMMITED hint bit:
>         else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))>         {>>>>             tuple->t_infomask
|=HEAP_XMIN_COMMITTED;>             SetBufferCommitInfoNeedsSave(buffer);>         }
 

Issue a SELECT * FROM foo, and step a single instruction that fetches 
the infomask field from memory to a register.

Open another backend, set a breakpoint to HeapTupleSatisfiesVacuum line 
1178:
>         else if (TransactionIdDidCommit(HeapTupleHeaderGetXmax(tuple)))>         {>             tuple->t_infomask |=
HEAP_XMAX_COMMITTED;>            SetBufferCommitInfoNeedsSave(buffer);>         }>         else>         {>
/*>             * Not in Progress, Not Committed, so either Aborted or 
 
crashed>              */>             tuple->t_infomask |= HEAP_XMAX_INVALID;>
SetBufferCommitInfoNeedsSave(buffer);>            return HEAPTUPLE_LIVE;>         }>         /* Should only get here if
weset XMAX_COMMITTED */>>>>>         Assert(tuple->t_infomask & HEAP_XMAX_COMMITTED);>     }
 

And issue "VACUUM foo". It'll stop on that breakpoint.

Let the first backend continue. It will clear the XMAX_COMMITTED field.

Now let the 2nd backend to continue and you get an assertion failure.


AFAICS, we can just simply remove the assertion. But is there any 
codepaths that assume that after calling HeapTupleSatisfiesSnapshot, all 
appropriate hint bits are set?

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com