Re: Use read streams in CREATE DATABASE command when the strategy is wal_log

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