Hi,
On Sat, 20 Jul 2024 at 21:14, Noah Misch <n...@leadboat.com> wrote:
>
> On Sat, Jul 20, 2024 at 03:01:31PM +0300, Nazir Bilal Yavuz wrote:
> >
> > With the separate commit (e00c45f685), does it make sense to rename
> > the smgr_persistence parameter of the ReadBuffer_common() to
> > persistence? Because, ExtendBufferedRelTo() calls ReadBuffer_common()
> > with rel's persistence now, not with smgr's persistence.
>
> BMR_REL() doesn't set relpersistence, so bmr.relpersistence is associated with
> bmr.smgr and is unset if bmr.rel is set. That is to say, bmr.relpersistence
> is an smgr_persistence. It could make sense to change ReadBuffer_common() to
> take a BufferManagerRelation instead of the three distinct arguments.
Got it.
>
> On a different naming topic, my review missed that field name
> copy_storage_using_buffer_read_stream_private.last_block doesn't fit how the
> field is used. Code uses it like an nblocks. So let's either rename the
> field or change the code to use it as a last_block (e.g. initialize it to
> nblocks-1, not nblocks).
I prefered renaming it as nblocks, since that is how it was used in
RelationCopyStorageUsingBuffer() before. Also, I realized that instead
of setting p.blocknum = 0; initializing blkno as 0 and using
p.blocknum = blkno makes sense. Because, p.blocknum and blkno should
always start with the same block number. The relevant patch is
attached.
--
Regards,
Nazir Bilal Yavuz
Microsoft
From 1110cfe915dd885c90f91014f8dbece20426efbc Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <byavu...@gmail.com>
Date: Mon, 22 Jul 2024 11:07:50 +0300
Subject: [PATCH v5] Minimal refactorings on RelationCopyStorageUsingBuffer()
- copy_storage_using_buffer_read_stream_private.last_block is renamed as
nblocks.
- blkno is initialized as 0 and used while setting
copy_storage_using_buffer_read_stream_private.blocknum.
---
src/backend/storage/buffer/bufmgr.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index a6aeecdf534..29d862ccfde 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -143,7 +143,7 @@ typedef struct SMgrSortArray
struct copy_storage_using_buffer_read_stream_private
{
BlockNumber blocknum;
- BlockNumber last_block;
+ BlockNumber nblocks;
};
/*
@@ -157,7 +157,7 @@ copy_storage_using_buffer_read_stream_next_block(ReadStream *stream,
{
struct copy_storage_using_buffer_read_stream_private *p = callback_private_data;
- if (p->blocknum < p->last_block)
+ if (p->blocknum < p->nblocks)
return p->blocknum++;
return InvalidBlockNumber;
@@ -4709,7 +4709,7 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
Page dstPage;
bool use_wal;
BlockNumber nblocks;
- BlockNumber blkno;
+ BlockNumber blkno = 0;
PGIOAlignedBlock buf;
BufferAccessStrategy bstrategy_src;
BufferAccessStrategy bstrategy_dst;
@@ -4745,8 +4745,8 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
bstrategy_dst = GetAccessStrategy(BAS_BULKWRITE);
/* Initalize streaming read */
- p.blocknum = 0;
- p.last_block = nblocks;
+ p.blocknum = blkno;
+ p.nblocks = nblocks;
src_smgr = smgropen(srclocator, INVALID_PROC_NUMBER);
src_stream = read_stream_begin_smgr_relation(READ_STREAM_FULL,
bstrategy_src,
@@ -4758,7 +4758,7 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
0);
/* Iterate over each block of the source relation file. */
- for (blkno = 0; blkno < nblocks; blkno++)
+ for (; blkno < nblocks; blkno++)
{
CHECK_FOR_INTERRUPTS();
--
2.45.2
