Date: Sat, 20 Jul 2024 11:14:05 -0700
From: Noah Misch <noah@leadboat.com>
To: Nazir Bilal Yavuz <byavuz81@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Use read streams in CREATE DATABASE command when the strategy is
 wal_log
Message-ID: <20240720181405.5a.nmisch@google.com>
References: 
 <CAN55FZ0JKL6vk1xQp6rfOXiNFV1u1H0tJDPPGHWoiO3ea2Wc=A@mail.gmail.com>
 <20240711235209.f6.nmisch@google.com>
 <CAN55FZ0pHWZrLPwKGggnigJCj9Cp7fCXY8-e5yGNLzzp5kPvgg@mail.gmail.com>
 <20240716121935.fa.nmisch@google.com>
 <CAN55FZ33iZ3LmfqGhGH9YU8-b5SQisD_KJmmtFAEsUR0=XL-gg@mail.gmail.com>
 <20240717204147.09@rfd.leadboat.com>
 <CAN55FZ0TH_igXHFpVAicoDOhAn3S-JHutMDsa7JbrJx6HEgO3g@mail.gmail.com>
 <20240720112718.68.nmisch@google.com>
 <CAN55FZ1bOfGBszLqO_YF-wb0tCQa_PeHouf3Np0OA0N+Vy++eA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAN55FZ1bOfGBszLqO_YF-wb0tCQa_PeHouf3Np0OA0N+Vy++eA@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20240720181405.5a.nmisch%40google.com>
Precedence: bulk

On Sat, Jul 20, 2024 at 03:01:31PM +0300, Nazir Bilal Yavuz wrote:
> On Sat, 20 Jul 2024 at 14:27, Noah Misch <noah@leadboat.com> wrote:
> >
> > On Thu, Jul 18, 2024 at 02:11:13PM +0300, Nazir Bilal Yavuz wrote:
> > > v4 is attached.
> >
> > Removal of the PinBufferForBlock() comment about the "persistence =
> > RELPERSISTENCE_PERMANENT" fallback started to feel like a loss.  I looked for
> > a way to re-add a comment about the fallback.
> > https://coverage.postgresql.org/src/backend/storage/buffer/bufmgr.c.gcov.html
> > shows no test coverage of that fallback, and I think the fallback is
> > unreachable.  Hence, I've removed the fallback in a separate commit.  I've
> > pushed that and your three patches.  Thanks.
> 
> Thanks for the separate commit and push!
> 
> 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.

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).