Andres Freund <and...@2ndquadrant.com> wrote: > On 2015-02-14 17:25:00 +0000, Kevin Grittner wrote:
>>> I think we should simply move the >>> buf->flags &= ~BM_PIN_COUNT_WAITER (Inside LockBuffer) >> >> I think you meant inside UnpinBuffer? > > No, LockBufferHdr. What I meant was that the pincount can only be > manipulated while the buffer header spinlock is held. Oh, I see what you were saying -- I had read that a different way entirely. Got it. >> Even though it appears to be a long-standing bug, there don't >> appear to have been any field reports, so it doesn't seem like >> something to back-patch. > > I was wondering about that as well. But I don't think I agree. > The most likely scenario for this to fail is in full table > vacuums that have to freeze rows - those are primarily triggered > by autovacuum. I don't think it's likely that such a error > message would be discovered in the logs unless it happens very > regularly. I guess we have some time before the next minor release to find any problems with this; perhaps the benefit would outweigh the risk. Anyone else want to weigh in on that? > You can't manipulate flags without holding the spinlock. > Otherwise you (or the other writer) can easily cancel the other > sides effects. So is the attached more like what you had in mind? If not, feel free to post a patch. :-) -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index e1e6240..6640172 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -1548,7 +1548,6 @@ UnpinBuffer(volatile BufferDesc *buf, bool fixOwner) /* we just released the last pin other than the waiter's */ int wait_backend_pid = buf->wait_backend_pid; - buf->flags &= ~BM_PIN_COUNT_WAITER; UnlockBufHdr(buf); ProcSendSignal(wait_backend_pid); } @@ -3273,6 +3272,13 @@ LockBufferForCleanup(Buffer buffer) else ProcWaitForSignal(); + /* + * Clear the flag unconditionally here, because otherwise a spurious + * signal (which is allowed) could make it look like an error. + */ + LockBufHdr(bufHdr); + bufHdr->flags &= ~BM_PIN_COUNT_WAITER; + UnlockBufHdr(bufHdr); PinCountWaitBuf = NULL; /* Loop back and try again */ }
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
