Re: Use read streams in CREATE DATABASE command when the strategy is wal_log - Mailing list pgsql-hackers
From | Nazir Bilal Yavuz |
---|---|
Subject | Re: Use read streams in CREATE DATABASE command when the strategy is wal_log |
Date | |
Msg-id | CAN55FZ0pHWZrLPwKGggnigJCj9Cp7fCXY8-e5yGNLzzp5kPvgg@mail.gmail.com Whole thread Raw |
In response to | Re: Use read streams in CREATE DATABASE command when the strategy is wal_log (Noah Misch <noah@leadboat.com>) |
Responses |
Re: Use read streams in CREATE DATABASE command when the strategy is wal_log
|
List | pgsql-hackers |
Hi, Thank you for the review! 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: > > I am working on using read streams in the CREATE DATABASE command when the > > strategy is wal_log. RelationCopyStorageUsingBuffer() function is used in > > this context. This function reads source buffers then copies them to the > > Please rebase. I applied this to 40126ac for review purposes. Rebased. > > Subject: [PATCH v1 1/3] Refactor PinBufferForBlock() to remove if checks about > > persistence > > > > There are if checks in PinBufferForBlock() function to set persistence > > of the relation and this function is called for the each block in the > > relation. Instead of that, set persistence of the relation before > > PinBufferForBlock() function. > > I tried with the following additional patch to see if PinBufferForBlock() ever > gets invalid smgr_relpersistence: > > ==== > --- a/src/backend/storage/buffer/bufmgr.c > +++ b/src/backend/storage/buffer/bufmgr.c > @@ -1098,6 +1098,11 @@ PinBufferForBlock(Relation rel, > > Assert(blockNum != P_NEW); > > + if (!(smgr_persistence == RELPERSISTENCE_TEMP || > + smgr_persistence == RELPERSISTENCE_PERMANENT || > + smgr_persistence == RELPERSISTENCE_UNLOGGED)) > + elog(WARNING, "unexpected relpersistence %d", smgr_persistence); > + > if (smgr_persistence == RELPERSISTENCE_TEMP) > { > io_context = IOCONTEXT_NORMAL; > ==== > > That still gets relpersistence==0 in various src/test/regress cases. I think > the intent was to prevent that. If not, please add a comment about when > relpersistence==0 is still allowed. I fixed it, it is caused by (mode == RBM_ZERO_AND_CLEANUP_LOCK | mode == RBM_ZERO_AND_LOCK) case in the ReadBuffer_common(). The persistence was not updated for this path before. I also added an assert check for this problem to PinBufferForBlock(). > > --- 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? > > --- a/src/backend/storage/buffer/bufmgr.c > > +++ b/src/backend/storage/buffer/bufmgr.c > > > +/* > > + * Helper struct for read stream object used in > > + * RelationCopyStorageUsingBuffer() function. > > + */ > > +struct copy_storage_using_buffer_read_stream_private > > +{ > > + BlockNumber blocknum; > > + int64 last_block; > > +}; > > Why is last_block an int64, not a BlockNumber? You are right, the type of last_block should be BlockNumber; done. I copied it from pg_prewarm_read_stream_private struct and I guess the same should be applied to it as well but it is not the topic of this thread, so I did not update it yet. > > @@ -4667,19 +4698,31 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator, > > > /* Iterate over each block of the source relation file. */ > > for (blkno = 0; blkno < nblocks; blkno++) > > { > > CHECK_FOR_INTERRUPTS(); > > > > /* Read block from source relation. */ > > - srcBuf = ReadBufferWithoutRelcache(srclocator, forkNum, blkno, > > - RBM_NORMAL, bstrategy_src, > > - permanent); > > + srcBuf = read_stream_next_buffer(src_stream, NULL); > > LockBuffer(srcBuf, BUFFER_LOCK_SHARE); > > I think this should check for read_stream_next_buffer() returning > InvalidBuffer. pg_prewarm doesn't, but the other callers do, and I think the > other callers are a better model. LockBuffer() doesn't check the > InvalidBuffer case, so let's avoid the style of using a > read_stream_next_buffer() return value without checking. There is an assert in the LockBuffer which checks for the InvalidBuffer. If that is not enough, we may add an if check for InvalidBuffer but what should we do in this case? It should not happen, so erroring out may be a good idea. Updated patches are attached (without InvalidBuffer check for now). -- Regards, Nazir Bilal Yavuz Microsoft
Attachment
pgsql-hackers by date: