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

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.

> a. Timings:
> b. strace:
> c. perf:

Thanks for including those details.  That's valuable confirmation.

> 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.

> --- 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.
     */

> --- 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?

> @@ -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.

Thanks,
nm