Hi,
On Wed, 17 Jul 2024 at 23:41, Noah Misch <noah@leadboat.com> wrote:
>
> 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.
I see, thanks for the explanation. I removed that part of the comment.
>
> 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;
Nice catch, I do not think it is needed now. WaitReadBuffers() is
called only from ReadBuffer_common() and read_stream_next_buffer().
For the ReadBuffer_common(), persistence is calculated before calling
WaitReadBuffers(). And for the read_stream_next_buffer(), it is
calculated while creating a read stream object in the
read_stream_begin_impl().
v4 is attached.
--
Regards,
Nazir Bilal Yavuz
Microsoft