public inbox for [email protected]
help / color / mirror / Atom feedFrom: Kirill Reshke <[email protected]>
To: Nazir Bilal Yavuz <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Use streaming read API in pgstattuple.
Date: Thu, 28 Nov 2024 11:13:08 +0500
Message-ID: <CALdSSPgvSe2gEpBzs9q1h1BHLG0oewyS2+i5M_2cjaQOtfV4hw@mail.gmail.com> (raw)
In-Reply-To: <CAN55FZ0+xO1Y_Ru2Xe3D0taeJmTkUB2qbg-iwJXC6iX_mpx1Fw@mail.gmail.com>
References: <CALdSSPjPGRXHsVhiuDdSNWKrCfJio1mKrjnoOVDmgSN-PM8pCg@mail.gmail.com>
<CAN55FZ0+xO1Y_Ru2Xe3D0taeJmTkUB2qbg-iwJXC6iX_mpx1Fw@mail.gmail.com>
On Tue, 26 Nov 2024 at 15:39, Nazir Bilal Yavuz <[email protected]> wrote:
>
> Hi,
>
> Thank you for working on this!
>
> On Mon, 25 Nov 2024 at 21:17, Kirill Reshke <[email protected]> wrote:
> > While reviewing other threads implementing stream API for various core
> > subsystems, I spotted that pgstattuple could also benefit from that.
> > So, PFA.
> >
> > Notice refactoring around pgstat_hash_page and changes in pgstat_page
> > signature. This is needed because pgstat_hash_tuple uses
> > _hash_getbuf_with_strategy rather than ReadBufferExtended, which is
> > OK, but makes adapting the streaming API impossible. So, I changed
> > this place. Old codes should behave exactly the same way as new ones.
> > I eliminated the additional check that _hash_getbuf_with_strategy
> > performed, which was blkno == P_NEW. However, since the streaming API
> > already delivers the right buffer, I think it's okay.
>
> I agree that this looks okay.
>
> > This change behaves sanely on my by-hand testing.
>
> I encountered some problems while playing with your patch. This query
> [1] fails when the patch is applied although CI passes, it wasn't
> failing before. I looked at the code and found that:
>
> === 1
>
> blkno = start;
> for (;;)
> {
> /* Get the current relation length */
> LockRelationForExtension(rel, ExclusiveLock);
> nblocks = RelationGetNumberOfBlocks(rel);
> UnlockRelationForExtension(rel, ExclusiveLock);
>
> I think you need to set p.last_exclusive to nblocks here.
>
> === 2
>
> - for (; blkno < nblocks; blkno++)
> + while(BufferIsValid(buf = read_stream_next_buffer(stream, NULL)))
> {
> CHECK_FOR_INTERRUPTS();
>
> - pagefn(&stat, rel, blkno, bstrategy);
> + pagefn(&stat, rel, buf);
> }
>
> blkno doesn't get increased now and this causes an infinite loop.
>
> ===
>
> Also, since this problem couldn't be found by the CI, you may want to
> add a test for the pgstat_index function.
>
> [1] CREATE EXTENSION pgstattuple; create table test (a int primary
> key, b int[]); create index test_hashidx on test using hash (b);
> select pgstattuple(oid) from pg_class where relname = 'test_hashidx';
>
> --
> Regards,
> Nazir Bilal Yavuz
> Microsoft
Hello! Thank you for taking a peek. Your review comments have been
corrected. Since my changes were wrong, I honestly don't know why this
worked in version 1. By a miracle.
As for CI, i rechecked v1:
```
db2=#
select * from pgstathashindex('test_hashidx');
version | bucket_pages | overflow_pages | bitmap_pages | unused_pages
| live_items | dead_items | free_percent
---------+--------------+----------------+--------------+--------------+------------+------------+--------------
4 | 4 | 0 | 1 | 0
| 0 | 0 | 100
(1 row)
db2=#
select * from pgstattuple('test_hashidx');
ERROR: could not read blocks 6..16 in file "base/16454/16473": read
only 0 of 90112 bytes
```
In v2 this behaves correctly.
Should we add `pgstattuple(...)` after `pgstat*type*index(..)`
everywhere in pgstattuple regression tests?
P.S.
I want to mention something that I forgot in my initial email: I
reused codes from here https://commitfest.postgresql.org/50/5327/ with
some adaptation changes. So, Andrey is the author of this patch too.
--
Best regards,
Kirill Reshke
Attachments:
[application/octet-stream] v2-0001-Use-stream-read-interface-for-pgstattuple-routine.patch (5.3K, ../CALdSSPgvSe2gEpBzs9q1h1BHLG0oewyS2+i5M_2cjaQOtfV4hw@mail.gmail.com/2-v2-0001-Use-stream-read-interface-for-pgstattuple-routine.patch)
download | inline diff:
From 72dbd541e8efdbaa7340df871823da71f3c6e0fa Mon Sep 17 00:00:00 2001
From: reshke kirill <[email protected]>
Date: Mon, 25 Nov 2024 18:03:44 +0000
Subject: [PATCH v2] Use stream read interface for pgstattuple routines.
Patch implements new streaming read API for pgstattuple contrib
extension.
This is not perfomance speedup patch.
This patch enables this extension to benefit from stream API in the future
without introducing any performance improvements.
---
contrib/pgstattuple/pgstattuple.c | 64 +++++++++++++++++++------------
1 file changed, 39 insertions(+), 25 deletions(-)
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index 48cb8f59c4f..79b0157b9c0 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -61,22 +61,18 @@ typedef struct pgstattuple_type
uint64 free_space; /* free/reusable space in bytes */
} pgstattuple_type;
-typedef void (*pgstat_page) (pgstattuple_type *, Relation, BlockNumber,
- BufferAccessStrategy);
+typedef void (*pgstat_page) (pgstattuple_type *, Relation, Buffer);
static Datum build_pgstattuple_type(pgstattuple_type *stat,
FunctionCallInfo fcinfo);
static Datum pgstat_relation(Relation rel, FunctionCallInfo fcinfo);
static Datum pgstat_heap(Relation rel, FunctionCallInfo fcinfo);
static void pgstat_btree_page(pgstattuple_type *stat,
- Relation rel, BlockNumber blkno,
- BufferAccessStrategy bstrategy);
+ Relation rel, Buffer buf);
static void pgstat_hash_page(pgstattuple_type *stat,
- Relation rel, BlockNumber blkno,
- BufferAccessStrategy bstrategy);
+ Relation rel, Buffer buf);
static void pgstat_gist_page(pgstattuple_type *stat,
- Relation rel, BlockNumber blkno,
- BufferAccessStrategy bstrategy);
+ Relation rel, Buffer buf);
static Datum pgstat_index(Relation rel, BlockNumber start,
pgstat_page pagefn, FunctionCallInfo fcinfo);
static void pgstat_index_page(pgstattuple_type *stat, Page page,
@@ -405,13 +401,10 @@ pgstat_heap(Relation rel, FunctionCallInfo fcinfo)
* pgstat_btree_page -- check tuples in a btree page
*/
static void
-pgstat_btree_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno,
- BufferAccessStrategy bstrategy)
+pgstat_btree_page(pgstattuple_type *stat, Relation rel, Buffer buf)
{
- Buffer buf;
Page page;
- buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, bstrategy);
LockBuffer(buf, BT_READ);
page = BufferGetPage(buf);
@@ -449,13 +442,15 @@ pgstat_btree_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno,
* pgstat_hash_page -- check tuples in a hash page
*/
static void
-pgstat_hash_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno,
- BufferAccessStrategy bstrategy)
+pgstat_hash_page(pgstattuple_type *stat, Relation rel, Buffer buf)
{
- Buffer buf;
Page page;
- buf = _hash_getbuf_with_strategy(rel, blkno, HASH_READ, 0, bstrategy);
+ LockBuffer(buf, HASH_READ);
+
+ /* ref count and lock type are correct */
+
+ _hash_checkpage(rel, buf, 0);
page = BufferGetPage(buf);
if (PageGetSpecialSize(page) == MAXALIGN(sizeof(HashPageOpaqueData)))
@@ -491,13 +486,10 @@ pgstat_hash_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno,
* pgstat_gist_page -- check tuples in a gist page
*/
static void
-pgstat_gist_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno,
- BufferAccessStrategy bstrategy)
+pgstat_gist_page(pgstattuple_type *stat, Relation rel, Buffer buf)
{
- Buffer buf;
Page page;
- buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, bstrategy);
LockBuffer(buf, GIST_SHARE);
gistcheckpage(rel, buf);
page = BufferGetPage(buf);
@@ -523,14 +515,24 @@ pgstat_index(Relation rel, BlockNumber start, pgstat_page pagefn,
FunctionCallInfo fcinfo)
{
BlockNumber nblocks;
- BlockNumber blkno;
BufferAccessStrategy bstrategy;
pgstattuple_type stat = {0};
+ Buffer buf;
+ BlockRangeReadStreamPrivate p;
+ ReadStream *stream = NULL;
/* prepare access strategy for this index */
bstrategy = GetAccessStrategy(BAS_BULKREAD);
- blkno = start;
+ p.current_blocknum = start;
+ stream = read_stream_begin_relation(READ_STREAM_FULL,
+ bstrategy,
+ rel,
+ MAIN_FORKNUM,
+ block_range_read_stream_cb,
+ &p,
+ 0);
+
for (;;)
{
/* Get the current relation length */
@@ -539,21 +541,33 @@ pgstat_index(Relation rel, BlockNumber start, pgstat_page pagefn,
UnlockRelationForExtension(rel, ExclusiveLock);
/* Quit if we've scanned the whole relation */
- if (blkno >= nblocks)
+ if (p.current_blocknum >= nblocks)
{
stat.table_len = (uint64) nblocks * BLCKSZ;
break;
}
- for (; blkno < nblocks; blkno++)
+ p.last_exclusive = nblocks;
+
+ while (BufferIsValid(buf = read_stream_next_buffer(stream, NULL)))
{
CHECK_FOR_INTERRUPTS();
- pagefn(&stat, rel, blkno, bstrategy);
+ pagefn(&stat, rel, buf);
}
+
+ Assert(read_stream_next_buffer(stream, NULL) == InvalidBuffer);
+
+ /*
+ * After reaching the end we have to reset stream to use it again.
+ * Extra restart in case of just one iteration does not cost us much.
+ */
+ read_stream_reset(stream);
}
+ read_stream_end(stream);
+
relation_close(rel, AccessShareLock);
return build_pgstattuple_type(&stat, fcinfo);
--
2.34.1
view thread (4+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Use streaming read API in pgstattuple.
In-Reply-To: <CALdSSPgvSe2gEpBzs9q1h1BHLG0oewyS2+i5M_2cjaQOtfV4hw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox