Hi,
On Mon, 8 Apr 2024 at 00:01, Nazir Bilal Yavuz <byavu...@gmail.com> wrote:
>
> Hi,
>
> On Sun, 7 Apr 2024 at 20:33, Nazir Bilal Yavuz <byavu...@gmail.com> wrote:
> >
> > Hi,
> >
> > On Tue, 2 Apr 2024 at 11:40, Thomas Munro <thomas.mu...@gmail.com> wrote:
> > >
> > > I had been planning to commit v14 this morning but got cold feet with
> > > the BMR-based interface. Heikki didn't like it much, and in the end,
> > > neither did I. I have now removed it, and it seems much better. No
> > > other significant changes, just parameter types and inlining details.
> > > For example:
> > >
> > > * read_stream_begin_relation() now takes a Relation, likes its name says
> > > * StartReadBuffers()'s operation takes smgr and optional rel
> > > * ReadBuffer_common() takes smgr and optional rel
> >
> > Read stream objects can be created only using Relations now. There
> > could be read stream users which do not have a Relation but
> > SMgrRelations. So, I created another constructor for the read streams
> > which use SMgrRelations instead of Relations. Related patch is
> > attached.
>
> After sending this, I realized that I forgot to add persistence value
> to the new constructor. While working on it I also realized that
> current code sets persistence in PinBufferForBlock() function and this
> function is called for each block, which can be costly. So, I moved
> setting persistence to the out of PinBufferForBlock() function.
>
> Setting persistence outside of the PinBufferForBlock() function (0001)
> and creating the new constructor that uses SMgrRelations (0002) are
> attached.
Melanie noticed there was a 'sgmr -> smgr' typo in 0002. Fixed in attached.
--
Regards,
Nazir Bilal Yavuz
Microsoft
From 04fd860ce8c4c57830930cb362799fd155c92613 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <byavu...@gmail.com>
Date: Sun, 7 Apr 2024 22:33:36 +0300
Subject: [PATCH 1/2] 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.
---
src/backend/storage/aio/read_stream.c | 2 +-
src/backend/storage/buffer/bufmgr.c | 31 +++++++++++----------------
2 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index f54dacdd914..d155dde5ce3 100644
--- 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;
stream->ios[i].op.forknum = forknum;
stream->ios[i].op.strategy = strategy;
}
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 06e9ffd2b00..b4fcefed78a 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1067,24 +1067,10 @@ PinBufferForBlock(Relation rel,
BufferDesc *bufHdr;
IOContext io_context;
IOObject io_object;
- char persistence;
Assert(blockNum != P_NEW);
- /*
- * If there is no Relation it usually implies recovery and thus permanent,
- * but we take an argmument because CreateAndCopyRelationData can reach us
- * with only an SMgrRelation for an unlogged relation that we don't want
- * to flag with BM_PERMANENT.
- */
- if (rel)
- persistence = rel->rd_rel->relpersistence;
- else if (smgr_persistence == 0)
- persistence = RELPERSISTENCE_PERMANENT;
- else
- persistence = smgr_persistence;
-
- if (persistence == RELPERSISTENCE_TEMP)
+ if (smgr_persistence == RELPERSISTENCE_TEMP)
{
io_context = IOCONTEXT_NORMAL;
io_object = IOOBJECT_TEMP_RELATION;
@@ -1101,7 +1087,7 @@ PinBufferForBlock(Relation rel,
smgr->smgr_rlocator.locator.relNumber,
smgr->smgr_rlocator.backend);
- if (persistence == RELPERSISTENCE_TEMP)
+ if (smgr_persistence == RELPERSISTENCE_TEMP)
{
bufHdr = LocalBufferAlloc(smgr, forkNum, blockNum, foundPtr);
if (*foundPtr)
@@ -1109,7 +1095,7 @@ PinBufferForBlock(Relation rel,
}
else
{
- bufHdr = BufferAlloc(smgr, persistence, forkNum, blockNum,
+ bufHdr = BufferAlloc(smgr, smgr_persistence, forkNum, blockNum,
strategy, foundPtr, io_context);
if (*foundPtr)
pgBufferUsage.shared_blks_hit++;
@@ -1157,6 +1143,7 @@ ReadBuffer_common(Relation rel, SMgrRelation smgr, char smgr_persistence,
ReadBuffersOperation operation;
Buffer buffer;
int flags;
+ char persistence;
/*
* Backward compatibility path, most code should use ExtendBufferedRel()
@@ -1195,7 +1182,15 @@ ReadBuffer_common(Relation rel, SMgrRelation smgr, char smgr_persistence,
flags = 0;
operation.smgr = smgr;
operation.rel = rel;
- operation.smgr_persistence = smgr_persistence;
+
+ if (rel)
+ persistence = rel->rd_rel->relpersistence;
+ else if (smgr_persistence == 0)
+ persistence = RELPERSISTENCE_PERMANENT;
+ else
+ persistence = smgr_persistence;
+ operation.smgr_persistence = persistence;
+
operation.forknum = forkNum;
operation.strategy = strategy;
if (StartReadBuffer(&operation,
--
2.43.0
From edb23556e7fdfbbb5e237704b210f219e143f771 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <byavu...@gmail.com>
Date: Mon, 8 Apr 2024 00:14:52 +0300
Subject: [PATCH 2/2] Add a way to create read stream object by using
SMgrRelation
Currently read stream object can be created only by using Relation,
there is no way to create it by using SMgrRelation.
To achieve that, read_stream_begin_impl() function is created and
contents of the read_stream_begin_relation() is moved into this
function.
Both read_stream_begin_relation() and read_stream_begin_smgr_relation()
calls read_stream_begin_impl() by Relation and SMgrRelation
respectively.
---
src/include/storage/read_stream.h | 9 +++
src/backend/storage/aio/read_stream.c | 83 ++++++++++++++++++++++-----
2 files changed, 78 insertions(+), 14 deletions(-)
diff --git a/src/include/storage/read_stream.h b/src/include/storage/read_stream.h
index fae09d2b4cc..c7d688d8c9c 100644
--- a/src/include/storage/read_stream.h
+++ b/src/include/storage/read_stream.h
@@ -15,6 +15,7 @@
#define READ_STREAM_H
#include "storage/bufmgr.h"
+#include "storage/smgr.h"
/* Default tuning, reasonable for many users. */
#define READ_STREAM_DEFAULT 0x00
@@ -56,6 +57,14 @@ extern ReadStream *read_stream_begin_relation(int flags,
ReadStreamBlockNumberCB callback,
void *callback_private_data,
size_t per_buffer_data_size);
+extern ReadStream *read_stream_begin_smgr_relation(int flags,
+ BufferAccessStrategy strategy,
+ SMgrRelation smgr,
+ char smgr_persistence,
+ ForkNumber forknum,
+ ReadStreamBlockNumberCB callback,
+ void *callback_private_data,
+ size_t per_buffer_data_size);
extern Buffer read_stream_next_buffer(ReadStream *stream, void **per_buffer_private);
extern void read_stream_reset(ReadStream *stream);
extern void read_stream_end(ReadStream *stream);
diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index d155dde5ce3..8dd100f016c 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -406,14 +406,16 @@ read_stream_look_ahead(ReadStream *stream, bool suppress_advice)
* write extra data for each block into the space provided to it. It will
* also receive callback_private_data for its own purposes.
*/
-ReadStream *
-read_stream_begin_relation(int flags,
- BufferAccessStrategy strategy,
- Relation rel,
- ForkNumber forknum,
- ReadStreamBlockNumberCB callback,
- void *callback_private_data,
- size_t per_buffer_data_size)
+static ReadStream *
+read_stream_begin_impl(int flags,
+ BufferAccessStrategy strategy,
+ Relation rel,
+ SMgrRelation smgr,
+ char smgr_persistence,
+ ForkNumber forknum,
+ ReadStreamBlockNumberCB callback,
+ void *callback_private_data,
+ size_t per_buffer_data_size)
{
ReadStream *stream;
size_t size;
@@ -422,9 +424,6 @@ read_stream_begin_relation(int flags,
int strategy_pin_limit;
uint32 max_pinned_buffers;
Oid tablespace_id;
- SMgrRelation smgr;
-
- smgr = RelationGetSmgr(rel);
/*
* Decide how many I/Os we will allow to run at the same time. That
@@ -434,7 +433,7 @@ read_stream_begin_relation(int flags,
*/
tablespace_id = smgr->smgr_rlocator.locator.spcOid;
if (!OidIsValid(MyDatabaseId) ||
- IsCatalogRelation(rel) ||
+ (rel && IsCatalogRelation(rel)) ||
IsCatalogRelationOid(smgr->smgr_rlocator.locator.relNumber))
{
/*
@@ -548,8 +547,8 @@ read_stream_begin_relation(int flags,
for (int i = 0; i < max_ios; ++i)
{
stream->ios[i].op.rel = rel;
- stream->ios[i].op.smgr = RelationGetSmgr(rel);
- stream->ios[i].op.smgr_persistence = rel->rd_rel->relpersistence;
+ stream->ios[i].op.smgr = smgr;
+ stream->ios[i].op.smgr_persistence = smgr_persistence;
stream->ios[i].op.forknum = forknum;
stream->ios[i].op.strategy = strategy;
}
@@ -557,6 +556,62 @@ read_stream_begin_relation(int flags,
return stream;
}
+/*
+ * Create a new read stream for reading a relation.
+ * See read_stream_begin_impl() for the detailed explanation.
+ */
+ReadStream *
+read_stream_begin_relation(int flags,
+ BufferAccessStrategy strategy,
+ Relation rel,
+ ForkNumber forknum,
+ ReadStreamBlockNumberCB callback,
+ void *callback_private_data,
+ size_t per_buffer_data_size)
+{
+ return read_stream_begin_impl(flags,
+ strategy,
+ rel,
+ RelationGetSmgr(rel),
+ rel->rd_rel->relpersistence,
+ forknum,
+ callback,
+ callback_private_data,
+ per_buffer_data_size);
+}
+
+/*
+ * Create a new read stream for reading a SMgr relation.
+ * See read_stream_begin_impl() for the detailed explanation.
+ */
+ReadStream *
+read_stream_begin_smgr_relation(int flags,
+ BufferAccessStrategy strategy,
+ SMgrRelation smgr,
+ char smgr_persistence,
+ ForkNumber forknum,
+ ReadStreamBlockNumberCB callback,
+ void *callback_private_data,
+ size_t per_buffer_data_size)
+{
+ char persistence;
+
+ if (smgr_persistence == 0)
+ persistence = RELPERSISTENCE_PERMANENT;
+ else
+ persistence = smgr_persistence;
+
+ return read_stream_begin_impl(flags,
+ strategy,
+ NULL,
+ smgr,
+ persistence,
+ forknum,
+ callback,
+ callback_private_data,
+ per_buffer_data_size);
+}
+
/*
* Pull one pinned buffer out of a stream. Each call returns successive
* blocks in the order specified by the callback. If per_buffer_data_size was
--
2.43.0
