Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit130beba

Browse files
committed
Fix inadequate buffer locking in FSM and VM page re-initialization.
When reading an existing FSM or VM page that was found to be corrupt by thebuffer manager, the code applied PageInit() to reinitialize the page, butdid so without any locking. There is thus a hazard that two backends mightconcurrently do PageInit, which in itself would still be OK, but the slowerone might then zero over subsequent data changes applied by the faster one.Even that is unlikely to be fatal; but it's not desirable, so add lockingto prevent it.This does not add any locking overhead in the normal code path where thepage is OK. It's not immediately obvious that that's safe, but I believeit is, for reasons explained in the added comments.Problem noted by R P Asim. It's been like this for a long time, soback-patch to all supported branches.Discussion:https://postgr.es/m/CANXE4Te4G0TGq6cr0-TvwP0H4BNiK_-hB5gHe8mF+nz0mcYfMQ@mail.gmail.com
1 parent3948115 commit130beba

File tree

2 files changed

+40
-2
lines changed

2 files changed

+40
-2
lines changed

‎src/backend/access/heap/visibilitymap.c

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -610,11 +610,30 @@ vm_readbuf(Relation rel, BlockNumber blkno, bool extend)
610610
* Use ZERO_ON_ERROR mode, and initialize the page if necessary. It's
611611
* always safe to clear bits, so it's better to clear corrupt pages than
612612
* error out.
613+
*
614+
* The initialize-the-page part is trickier than it looks, because of the
615+
* possibility of multiple backends doing this concurrently, and our
616+
* desire to not uselessly take the buffer lock in the normal path where
617+
* the page is OK. We must take the lock to initialize the page, so
618+
* recheck page newness after we have the lock, in case someone else
619+
* already did it. Also, because we initially check PageIsNew with no
620+
* lock, it's possible to fall through and return the buffer while someone
621+
* else is still initializing the page (i.e., we might see pd_upper as set
622+
* but other page header fields are still zeroes). This is harmless for
623+
* callers that will take a buffer lock themselves, but some callers
624+
* inspect the page without any lock at all. The latter is OK only so
625+
* long as it doesn't depend on the page header having correct contents.
626+
* Current usage is safe because PageGetContents() does not require that.
613627
*/
614628
buf=ReadBufferExtended(rel,VISIBILITYMAP_FORKNUM,blkno,
615629
RBM_ZERO_ON_ERROR,NULL);
616630
if (PageIsNew(BufferGetPage(buf)))
617-
PageInit(BufferGetPage(buf),BLCKSZ,0);
631+
{
632+
LockBuffer(buf,BUFFER_LOCK_EXCLUSIVE);
633+
if (PageIsNew(BufferGetPage(buf)))
634+
PageInit(BufferGetPage(buf),BLCKSZ,0);
635+
LockBuffer(buf,BUFFER_LOCK_UNLOCK);
636+
}
618637
returnbuf;
619638
}
620639

‎src/backend/storage/freespace/freespace.c

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -580,10 +580,29 @@ fsm_readbuf(Relation rel, FSMAddress addr, bool extend)
580580
* pages than error out. Since the FSM changes are not WAL-logged, the
581581
* so-called torn page problem on crash can lead to pages with corrupt
582582
* headers, for example.
583+
*
584+
* The initialize-the-page part is trickier than it looks, because of the
585+
* possibility of multiple backends doing this concurrently, and our
586+
* desire to not uselessly take the buffer lock in the normal path where
587+
* the page is OK. We must take the lock to initialize the page, so
588+
* recheck page newness after we have the lock, in case someone else
589+
* already did it. Also, because we initially check PageIsNew with no
590+
* lock, it's possible to fall through and return the buffer while someone
591+
* else is still initializing the page (i.e., we might see pd_upper as set
592+
* but other page header fields are still zeroes). This is harmless for
593+
* callers that will take a buffer lock themselves, but some callers
594+
* inspect the page without any lock at all. The latter is OK only so
595+
* long as it doesn't depend on the page header having correct contents.
596+
* Current usage is safe because PageGetContents() does not require that.
583597
*/
584598
buf=ReadBufferExtended(rel,FSM_FORKNUM,blkno,RBM_ZERO_ON_ERROR,NULL);
585599
if (PageIsNew(BufferGetPage(buf)))
586-
PageInit(BufferGetPage(buf),BLCKSZ,0);
600+
{
601+
LockBuffer(buf,BUFFER_LOCK_EXCLUSIVE);
602+
if (PageIsNew(BufferGetPage(buf)))
603+
PageInit(BufferGetPage(buf),BLCKSZ,0);
604+
LockBuffer(buf,BUFFER_LOCK_UNLOCK);
605+
}
587606
returnbuf;
588607
}
589608

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp