Date: Wed, 17 Jul 2024 13:41:47 -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: <20240717204147.09@rfd.leadboat.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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAN55FZ33iZ3LmfqGhGH9YU8-b5SQisD_KJmmtFAEsUR0=XL-gg@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20240717204147.09%40rfd.leadboat.com>
Precedence: bulk

On Wed, Jul 17, 2024 at 12:22:49PM +0300, Nazir Bilal Yavuz wrote:
> On Tue, 16 Jul 2024 at 15:19, Noah Misch <noah@leadboat.com> wrote:
> > On Tue, Jul 16, 2024 at 02:11:20PM +0300, Nazir Bilal Yavuz wrote:
> > > On Fri, 12 Jul 2024 at 02:52, Noah Misch <noah@leadboat.com> wrote:
> > > > On Tue, Apr 16, 2024 at 02:12:19PM +0300, Nazir Bilal Yavuz wrote:
> > > > > --- a/src/backend/storage/aio/read_stream.c
> > > > > +++ b/src/backend/storage/aio/read_stream.c
> > > > > @@ -549,7 +549,7 @@ read_stream_begin_relation(int flags,
> > > > >       {
> > > > >               stream->ios[i].op.rel = rel;
> > > > >               stream->ios[i].op.smgr = RelationGetSmgr(rel);
> > > > > -             stream->ios[i].op.smgr_persistence = 0;
> > > > > +             stream->ios[i].op.smgr_persistence = rel->rd_rel->relpersistence;
> > > >
> > > > Does the following comment in ReadBuffersOperation need an update?
> > > >
> > > >         /*
> > > >          * The following members should be set by the caller.  If only smgr is
> > > >          * provided without rel, then smgr_persistence can be set to override the
> > > >          * default assumption of RELPERSISTENCE_PERMANENT.
> > > >          */
> > >
> > > I believe it does not need to be updated but I renamed
> > > 'ReadBuffersOperation.smgr_persistence' as
> > > 'ReadBuffersOperation.persistence'. So, this comment is updated as
> > > well. I think that rename suits better because persistence does not
> > > need to come from smgr, it could come from relation, too. Do you think
> > > it is a good idea? If it is, does it need a separate commit?
> >
> > The rename is good.  I think the comment implies "persistence" is unused when
> > rel!=NULL.  That implication is true before the patch but false after the
> > patch.
> 
> What makes it false after the patch? I think the logic did not change.
> If there is rel, the value of persistence is obtained from
> 'rel->rd_rel->relpersistence'. If there is no rel, then smgr is used
> to obtain its value.

First, the patch removes the "default assumption of RELPERSISTENCE_PERMANENT".
It's now an assertion failure.

The second point is about "If only smgr is provided without rel".  Before the
patch, the extern functions that take a ReadBuffersOperation argument examine
smgr_persistence if and only if rel==NULL.  That's consistent with the
comment.  After the patch, StartReadBuffersImpl() calling PinBufferForBlock()
uses the field unconditionally.

On that note, does WaitReadBuffers() still have a reason to calculate its
persistence as follows, or should this patch make it "persistence =
operation->persistence"?

	persistence = operation->rel
		? operation->rel->rd_rel->relpersistence
		: RELPERSISTENCE_PERMANENT;

> I think what you said in the counter argument makes sense.

Okay.