Thread: Possible race in UnlockBuffers() and UnpinBuffer()

We have a wait-pin-to-1 mechanism in LockBufferForCleanup() like this:
   1:  bufHdr->wait_backend_pid = MyProcPid;   2:  bufHdr->flags |= BM_PIN_COUNT_WAITER;   3:  PinCountWaitBuf =
bufHdr;  4:  UnlockBufHdr_NoHoldoff(bufHdr);   5:  LockBuffer(buffer, BUFFER_LOCK_UNLOCK);   6:  /* Wait to be signaled
byUnpinBuffer() */   7:  ProcWaitForSignal();
 

Say if the waiter encounters an error on line 5 or gets a cancel signal on
line 6, it will abort current transaction and call UnlockBuffers() to cancel
the wait. However, a possible execution sequence involving another process
doing UnpinBuffer() may look like this:

unpinner: lockHdr(); read and reset flag; unlockHdr();
waiter:  lockHdr(); reset flag; unlockHdr(); ProcCancelWaitForSignal();
unpinner: ProcSendSignal();

After this, the proc->sem will be bumped to 1 unexpectedly ... Since this
problem is rare, a possible fix is to put a critical section around line 1
to 7 and remove UnlockBuffers() accordingly.

Regards,
Qingqing

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> ... However, a possible execution sequence involving another process
> doing UnpinBuffer() may look like this:

> unpinner: lockHdr(); read and reset flag; unlockHdr();
> waiter:  lockHdr(); reset flag; unlockHdr(); ProcCancelWaitForSignal();
> unpinner: ProcSendSignal();

Hmm ... I remember having convinced myself this code was OK, but I guess
I was wrong.

> After this, the proc->sem will be bumped to 1 unexpectedly ... Since this
> problem is rare, a possible fix is to put a critical section around line 1
> to 7 and remove UnlockBuffers() accordingly.

No, that would make any attempt to control-C a VACUUM have a significant
probability for panicking the whole database.

I think a better fix might be to arrange for an extra PGSemaphoreUnlock
to not be a problem.  This is already true in lwlock.c, and in the
pin-count-waiter too (it'll just cause an extra cycle around the loop).
We'd have to modify ProcSleep to loop until it sees that someone has
actually granted or denied the lock, but that does not seem too hard.
(First thought about it is to change MyProc->waitStatus to have three
states, "waiting/ok/denied".)  ProcCancelWaitForSignal could then go
away entirely.
        regards, tom lane

"Tom Lane" <tgl@sss.pgh.pa.us> wrote
> "Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> > After this, the proc->sem will be bumped to 1 unexpectedly ... Since
this
> > problem is rare, a possible fix is to put a critical section around line
1
> > to 7 and remove UnlockBuffers() accordingly.
>
> No, that would make any attempt to control-C a VACUUM have a significant
> probability for panicking the whole database.
>

Why panicking by control-C? Is that critical section can prevent any signal
processing?

Regards,
Qingqing

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> "Tom Lane" <tgl@sss.pgh.pa.us> wrote
>> "Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
>>> After this, the proc->sem will be bumped to 1 unexpectedly ... Since
>>> this problem is rare, a possible fix is to put a critical section
>>> around line 1 to 7 and remove UnlockBuffers() accordingly.
>> 
>> No, that would make any attempt to control-C a VACUUM have a significant
>> probability for panicking the whole database.

> Why panicking by control-C?

Because the entire point of a critical section is that any error (eg
"Query cancelled") is turned into a panic.  So a query-cancel attempt
while a vacuum is blocked here would either do nothing (bad) or panic
the database (worse).
        regards, tom lane