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
