So here's my attempt to rewrite this logic. I ended up refactoring a
bit because I found it unnecessarily confusing having the mode
branches in several places. I think it's much clearer just having two
separate pieces of logic for RBM_NEW and the extension cases since all
they have in common is the ReadBuffer call.
I have to say, it scares the hell out of me that there are no
regression tests for this code. I'm certainly not comfortable
committing it without a few other people reading it if I haven't even
run the code once. At least I know it compiles...
*** a/src/backend/access/transam/xlogutils.c
--- b/src/backend/access/transam/xlogutils.c
***************
*** 293,300 **** Buffer
XLogReadBufferExtended(RelFileNode rnode, ForkNumber forknum,
BlockNumber blkno, ReadBufferMode mode)
{
- BlockNumber lastblock;
Buffer buffer;
SMgrRelation smgr;
Assert(blkno != P_NEW);
--- 293,300 ----
XLogReadBufferExtended(RelFileNode rnode, ForkNumber forknum,
BlockNumber blkno, ReadBufferMode mode)
{
Buffer buffer;
+ Page page;
SMgrRelation smgr;
Assert(blkno != P_NEW);
***************
*** 312,353 **** XLogReadBufferExtended(RelFileNode rnode, ForkNumber forknum,
*/
smgrcreate(smgr, forknum, true);
! lastblock = smgrnblocks(smgr, forknum);
!
! if (blkno < lastblock)
! {
! /* page exists in file */
! buffer = ReadBufferWithoutRelcache(rnode, forknum, blkno,
! mode, NULL);
! }
! else
{
/* hm, page doesn't exist in file */
! if (mode == RBM_NORMAL)
{
log_invalid_page(rnode, forknum, blkno, false);
return InvalidBuffer;
}
- /* OK to extend the file */
- /* we do this in recovery only - no rel-extension lock needed */
- Assert(InRecovery);
- buffer = InvalidBuffer;
- while (blkno >= lastblock)
- {
- if (buffer != InvalidBuffer)
- ReleaseBuffer(buffer);
- buffer = ReadBufferWithoutRelcache(rnode, forknum,
- P_NEW, mode, NULL);
- lastblock++;
- }
- Assert(BufferGetBlockNumber(buffer) == blkno);
- }
! if (mode == RBM_NORMAL)
! {
! /* check that page has been initialized */
! Page page = (Page) BufferGetPage(buffer);
/*
* We assume that PageIsNew is safe without a lock. During recovery,
* there should be no other backends that could modify the buffer at
--- 312,332 ----
*/
smgrcreate(smgr, forknum, true);
! if (mode == RBM_NORMAL)
{
/* hm, page doesn't exist in file */
! if (blkno >= smgrnblocks(smgr, forknum))
{
log_invalid_page(rnode, forknum, blkno, false);
return InvalidBuffer;
}
! buffer = ReadBufferWithoutRelcache(rnode, forknum, blkno,
! mode, NULL);
+ /* check that page has been initialized */
+ page = (Page) BufferGetPage(buffer);
+
/*
* We assume that PageIsNew is safe without a lock. During recovery,
* there should be no other backends that could modify the buffer at
***************
*** 359,366 **** XLogReadBufferExtended(RelFileNode rnode, ForkNumber forknum,
log_invalid_page(rnode, forknum, blkno, true);
return InvalidBuffer;
}
- }
return buffer;
}
--- 338,364 ----
log_invalid_page(rnode, forknum, blkno, true);
return InvalidBuffer;
}
+ } else {
+
+ /* If page doesn't exist extend the file */
+ while (blkno >= smgrnblocks(smgr, forknum))
+ {
+ /* we do this in recovery only - no rel-extension lock needed */
+ Assert(InRecovery);
+ buffer = ReadBufferWithoutRelcache(rnode, forknum,
+ P_NEW, mode, NULL);
+ /* Take care not to extend the relation past where it's needed */
+ if (BufferGetBlockNumber(buffer) == blkno)
+ return buffer;
+ ReleaseBuffer(buffer);
+ }
+
+ /* page now exists in file */
+ buffer = ReadBufferWithoutRelcache(rnode, forknum, blkno,
+ mode, NULL);
+ }
+
return buffer;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
