Date: Thu, 11 Jul 2024 16:52:09 -0700
From: Noah Misch <noah@leadboat.com>
To: Nazir Bilal Yavuz <byavuz81@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Use read streams in CREATE DATABASE command when the strategy is
 wal_log
Message-ID: <20240711235209.f6.nmisch@google.com>
References: 
 <CAN55FZ0JKL6vk1xQp6rfOXiNFV1u1H0tJDPPGHWoiO3ea2Wc=A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAN55FZ0JKL6vk1xQp6rfOXiNFV1u1H0tJDPPGHWoiO3ea2Wc=A@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20240711235209.f6.nmisch%40google.com>
Precedence: bulk

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