Re: Use read streams in CREATE DATABASE command when the strategy is wal_log - Mailing list pgsql-hackers
From | Noah Misch |
---|---|
Subject | Re: Use read streams in CREATE DATABASE command when the strategy is wal_log |
Date | |
Msg-id | 20240717204147.09@rfd.leadboat.com Whole thread Raw |
In response to | Re: Use read streams in CREATE DATABASE command when the strategy is wal_log (Nazir Bilal Yavuz <byavuz81@gmail.com>) |
Responses |
Re: Use read streams in CREATE DATABASE command when the strategy is wal_log
|
List | pgsql-hackers |
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.
pgsql-hackers by date: