Greg Stark <stark@mit.edu> writes:
> 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 don't like that at all. It's a lot of unnecessary churn in what is
indeed hard-to-test code, and personally I don't find it clearer.
Nor, if you're going to complain about the cost of smgrnblocks, does
it seem like a great idea to be doing that *twice* per page rather
than once.
How about the attached instead?
regards, tom lane
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 4cd82df..f1918f6 100644
*** a/src/backend/access/transam/xlogutils.c
--- b/src/backend/access/transam/xlogutils.c
*************** XLogReadBufferExtended(RelFileNode rnode
*** 338,352 ****
/* 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)
--- 338,358 ----
/* we do this in recovery only - no rel-extension lock needed */
Assert(InRecovery);
buffer = InvalidBuffer;
! do
{
if (buffer != InvalidBuffer)
ReleaseBuffer(buffer);
buffer = ReadBufferWithoutRelcache(rnode, forknum,
P_NEW, mode, NULL);
}
! while (BufferGetBlockNumber(buffer) < blkno);
! /* Handle the corner case that P_NEW returns non-consecutive pages */
! if (BufferGetBlockNumber(buffer) != blkno)
! {
! ReleaseBuffer(buffer);
! buffer = ReadBufferWithoutRelcache(rnode, forknum, blkno,
! mode, NULL);
! }
}
if (mode == RBM_NORMAL)
