public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 7/8] Ignore correlation for new BRIN opclasses 4+ messages / 3 participants [nested] [flat]
* [PATCH 7/8] Ignore correlation for new BRIN opclasses @ 2020-09-12 13:07 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Tomas Vondra @ 2020-09-12 13:07 UTC (permalink / raw) The new BRIN opclasses (bloom and minmax-multi) are less sensitive to poorly correlated data, so just assume the data is perfectly correlated during costing. Author: Tomas Vondra <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/backend/access/brin/brin_bloom.c | 1 + src/backend/access/brin/brin_minmax_multi.c | 1 + src/backend/utils/adt/selfuncs.c | 19 ++++++++++++++++++- src/include/access/brin_internal.h | 3 +++ 4 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/backend/access/brin/brin_bloom.c b/src/backend/access/brin/brin_bloom.c index c086e83236..22590b2351 100644 --- a/src/backend/access/brin/brin_bloom.c +++ b/src/backend/access/brin/brin_bloom.c @@ -412,6 +412,7 @@ brin_bloom_opcinfo(PG_FUNCTION_ARGS) result = palloc0(MAXALIGN(SizeofBrinOpcInfo(1)) + sizeof(BloomOpaque)); + result->oi_ignore_correlation = true; result->oi_nstored = 1; result->oi_regular_nulls = true; result->oi_opaque = (BloomOpaque *) diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c index d19ebf08c2..6957be0bdb 100644 --- a/src/backend/access/brin/brin_minmax_multi.c +++ b/src/backend/access/brin/brin_minmax_multi.c @@ -1737,6 +1737,7 @@ brin_minmax_multi_opcinfo(PG_FUNCTION_ARGS) result = palloc0(MAXALIGN(SizeofBrinOpcInfo(1)) + sizeof(MinmaxMultiOpaque)); + result->oi_ignore_correlation = true; result->oi_nstored = 1; result->oi_regular_nulls = true; result->oi_opaque = (MinmaxMultiOpaque *) diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index 52314d3aa1..0320d128f6 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -98,6 +98,7 @@ #include <math.h> #include "access/brin.h" +#include "access/brin_internal.h" #include "access/brin_page.h" #include "access/gin.h" #include "access/table.h" @@ -7352,7 +7353,8 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, double minimalRanges; double estimatedRanges; double selec; - Relation indexRel; + Relation indexRel = NULL; + TupleDesc tupdesc = NULL; ListCell *l; VariableStatData vardata; @@ -7374,6 +7376,7 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, */ indexRel = index_open(index->indexoid, NoLock); brinGetStats(indexRel, &statsData); + tupdesc = RelationGetDescr(indexRel); index_close(indexRel, NoLock); /* work out the actual number of ranges in the index */ @@ -7407,6 +7410,17 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, { IndexClause *iclause = lfirst_node(IndexClause, l); AttrNumber attnum = index->indexkeys[iclause->indexcol]; + FmgrInfo *opcInfoFn; + BrinOpcInfo *opcInfo; + Form_pg_attribute attr = TupleDescAttr(tupdesc, iclause->indexcol); + bool ignore_correlation; + + opcInfoFn = index_getprocinfo(indexRel, iclause->indexcol + 1, BRIN_PROCNUM_OPCINFO); + + opcInfo = (BrinOpcInfo *) + DatumGetPointer(FunctionCall1(opcInfoFn, attr->atttypid)); + + ignore_correlation = opcInfo->oi_ignore_correlation; /* attempt to lookup stats in relation for this index column */ if (attnum != 0) @@ -7477,6 +7491,9 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, if (sslot.nnumbers > 0) varCorrelation = Abs(sslot.numbers[0]); + if (ignore_correlation) + varCorrelation = 1.0; + if (varCorrelation > *indexCorrelation) *indexCorrelation = varCorrelation; diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h index 89254b5766..063b703208 100644 --- a/src/include/access/brin_internal.h +++ b/src/include/access/brin_internal.h @@ -30,6 +30,9 @@ typedef struct BrinOpcInfo /* Regular processing of NULLs in BrinValues? */ bool oi_regular_nulls; + /* Ignore correlation during cost estimation */ + bool oi_ignore_correlation; + /* Opaque pointer for the opclass' private use */ void *oi_opaque; -- 2.26.2 --------------D524CD37E77B36A2883A7617 Content-Type: text/x-patch; charset=UTF-8; name="0008-Consider-selectivity-in-choose_bitmap_and-20210311.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0008-Consider-selectivity-in-choose_bitmap_and-20210311.patc"; filename*1="h" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Use read streams in pg_visibility @ 2024-08-13 12:22 Nazir Bilal Yavuz <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Nazir Bilal Yavuz @ 2024-08-13 12:22 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> Hi, I am working on using the read stream in pg_visibility. There are two places to use it: 1- collect_visibility_data() This one is straightforward. I created a read stream object if 'include_pd' is true because read I/O is done when 'include_pd' is true. There is ~4% timing improvement with this change. I started the server with the default settings and created a 6 GB table. Then run 100 times pg_visibility() by clearing the OS cache between each run. ---------- 2- collect_corrupt_items() This one is more complicated. The read stream callback function loops until it finds a suitable block to read. So, if the callback returns an InvalidBlockNumber; it means that the stream processed all possible blocks and the stream should be finished. There is ~3% timing improvement with this change. I started the server with the default settings and created a 6 GB table. Then run 100 times pg_check_visible() by clearing the OS cache between each run. The downside of this approach is there are too many "vmbuffer is valid but BufferGetBlockNumber(*vmbuffer) is not equal to mapBlock, so vmbuffer needs to be read again" cases in the read stream version (700 vs 20 for the 6 GB table). This is caused by the callback function of the read stream reading a new vmbuf while getting next block numbers. So, vmbuf is wrong when we are checking visibility map bits that might have changed while we were acquiring the page lock. ---------- Both patches are attached. Any feedback would be appreciated. -- Regards, Nazir Bilal Yavuz Microsoft Attachments: [text/x-patch] v1-0001-Use-read-stream-in-pg_visibility-in-collect_visib.patch (3.4K, ../../CAN55FZ1_Ru3XpMgTwsU67FTH2fs_FrRROmb7x6zs+F44QBEiww@mail.gmail.com/2-v1-0001-Use-read-stream-in-pg_visibility-in-collect_visib.patch) download | inline diff: From c178e010759f042dc089fd4c5b2283a04b95f017 Mon Sep 17 00:00:00 2001 From: Nazir Bilal Yavuz <[email protected]> Date: Fri, 9 Aug 2024 11:41:52 +0300 Subject: [PATCH v1 1/2] Use read stream in pg_visibility in collect_visibility_data() Instead of reading blocks with ReadBufferExtended() in collect_visibility_data(), use read stream. This change provides about 4% performance improvement. --- contrib/pg_visibility/pg_visibility.c | 53 ++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c index 1a1a4ff7be7..10f961c58f0 100644 --- a/contrib/pg_visibility/pg_visibility.c +++ b/contrib/pg_visibility/pg_visibility.c @@ -21,6 +21,7 @@ #include "storage/bufmgr.h" #include "storage/proc.h" #include "storage/procarray.h" +#include "storage/read_stream.h" #include "storage/smgr.h" #include "utils/rel.h" #include "utils/snapmgr.h" @@ -41,6 +42,16 @@ typedef struct corrupt_items ItemPointer tids; } corrupt_items; +/* + * Helper struct for read stream object used in + * collect_visibility_data() function. + */ +struct collect_visibility_data_read_stream_private +{ + BlockNumber blocknum; + BlockNumber nblocks; +}; + PG_FUNCTION_INFO_V1(pg_visibility_map); PG_FUNCTION_INFO_V1(pg_visibility_map_rel); PG_FUNCTION_INFO_V1(pg_visibility); @@ -463,6 +474,23 @@ pg_visibility_tupdesc(bool include_blkno, bool include_pd) return BlessTupleDesc(tupdesc); } +/* + * Callback function to get next block for read stream object used in + * collect_visibility_data() function. + */ +static BlockNumber +collect_visibility_data_read_stream_next_block(ReadStream *stream, + void *callback_private_data, + void *per_buffer_data) +{ + struct collect_visibility_data_read_stream_private *p = callback_private_data; + + if (p->blocknum < p->nblocks) + return p->blocknum++; + + return InvalidBlockNumber; +} + /* * Collect visibility data about a relation. * @@ -478,6 +506,8 @@ collect_visibility_data(Oid relid, bool include_pd) BlockNumber blkno; Buffer vmbuffer = InvalidBuffer; BufferAccessStrategy bstrategy = GetAccessStrategy(BAS_BULKREAD); + struct collect_visibility_data_read_stream_private p; + ReadStream *stream = NULL; rel = relation_open(relid, AccessShareLock); @@ -489,6 +519,20 @@ collect_visibility_data(Oid relid, bool include_pd) info->next = 0; info->count = nblocks; + /* Crate a read stream if read will be done. */ + if (include_pd) + { + p.blocknum = 0; + p.nblocks = nblocks; + stream = read_stream_begin_relation(READ_STREAM_FULL, + bstrategy, + rel, + MAIN_FORKNUM, + collect_visibility_data_read_stream_next_block, + &p, + 0); + } + for (blkno = 0; blkno < nblocks; ++blkno) { int32 mapbits; @@ -513,8 +557,7 @@ collect_visibility_data(Oid relid, bool include_pd) Buffer buffer; Page page; - buffer = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, - bstrategy); + buffer = read_stream_next_buffer(stream, NULL); LockBuffer(buffer, BUFFER_LOCK_SHARE); page = BufferGetPage(buffer); @@ -525,6 +568,12 @@ collect_visibility_data(Oid relid, bool include_pd) } } + if (include_pd && stream) + { + Assert(read_stream_next_buffer(stream, NULL) == InvalidBuffer); + read_stream_end(stream); + } + /* Clean up. */ if (vmbuffer != InvalidBuffer) ReleaseBuffer(vmbuffer); -- 2.45.2 [text/x-patch] v1-0002-Use-read-stream-in-pg_visibility-in-collect_corru.patch (5.0K, ../../CAN55FZ1_Ru3XpMgTwsU67FTH2fs_FrRROmb7x6zs+F44QBEiww@mail.gmail.com/3-v1-0002-Use-read-stream-in-pg_visibility-in-collect_corru.patch) download | inline diff: From 456d1df641fd6e81658fd9288e1f81b9ce068708 Mon Sep 17 00:00:00 2001 From: Nazir Bilal Yavuz <[email protected]> Date: Tue, 13 Aug 2024 13:03:39 +0300 Subject: [PATCH v1 2/2] Use read stream in pg_visibility in collect_corrupt_items() Instead of reading blocks with ReadBufferExtended() in collect_corrupt_items(), use read stream. This change provides about 3% performance improvement. --- contrib/pg_visibility/pg_visibility.c | 93 +++++++++++++++++++++++---- 1 file changed, 81 insertions(+), 12 deletions(-) diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c index 10f961c58f0..4729ee95991 100644 --- a/contrib/pg_visibility/pg_visibility.c +++ b/contrib/pg_visibility/pg_visibility.c @@ -52,6 +52,20 @@ struct collect_visibility_data_read_stream_private BlockNumber nblocks; }; +/* + * Helper struct for read stream object used in + * collect_corrupt_items() function. + */ +struct collect_corrupt_items_read_stream_private +{ + bool all_frozen; + bool all_visible; + BlockNumber blocknum; + BlockNumber nblocks; + Relation rel; + Buffer *vmbuffer; +}; + PG_FUNCTION_INFO_V1(pg_visibility_map); PG_FUNCTION_INFO_V1(pg_visibility_map_rel); PG_FUNCTION_INFO_V1(pg_visibility); @@ -639,6 +653,35 @@ GetStrictOldestNonRemovableTransactionId(Relation rel) } } +/* + * Callback function to get next block for read stream object used in + * collect_corrupt_items() function. + */ +static BlockNumber +collect_corrupt_items_read_stream_next_block(ReadStream *stream, + void *callback_private_data, + void *per_buffer_data) +{ + struct collect_corrupt_items_read_stream_private *p = callback_private_data; + + for (; p->blocknum < p->nblocks; p->blocknum++) + { + bool check_frozen = false; + bool check_visible = false; + + if (p->all_frozen && VM_ALL_FROZEN(p->rel, p->blocknum, p->vmbuffer)) + check_frozen = true; + if (p->all_visible && VM_ALL_VISIBLE(p->rel, p->blocknum, p->vmbuffer)) + check_visible = true; + if (!check_visible && !check_frozen) + continue; + + return p->blocknum++; + } + + return InvalidBlockNumber; +} + /* * Returns a list of items whose visibility map information does not match * the status of the tuples on the page. @@ -663,6 +706,10 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) Buffer vmbuffer = InvalidBuffer; BufferAccessStrategy bstrategy = GetAccessStrategy(BAS_BULKREAD); TransactionId OldestXmin = InvalidTransactionId; + struct collect_corrupt_items_read_stream_private p; + ReadStream *stream; + + Assert(all_visible || all_frozen); rel = relation_open(relid, AccessShareLock); @@ -687,6 +734,20 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) items->count = 64; items->tids = palloc(items->count * sizeof(ItemPointerData)); + p.blocknum = 0; + p.nblocks = nblocks; + p.rel = rel; + p.vmbuffer = &vmbuffer; + p.all_frozen = all_frozen; + p.all_visible = all_visible; + stream = read_stream_begin_relation(READ_STREAM_FULL, + bstrategy, + rel, + MAIN_FORKNUM, + collect_corrupt_items_read_stream_next_block, + &p, + 0); + /* Loop over every block in the relation. */ for (blkno = 0; blkno < nblocks; ++blkno) { @@ -700,17 +761,20 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) /* Make sure we are interruptible. */ CHECK_FOR_INTERRUPTS(); - /* Use the visibility map to decide whether to check this page. */ - if (all_frozen && VM_ALL_FROZEN(rel, blkno, &vmbuffer)) - check_frozen = true; - if (all_visible && VM_ALL_VISIBLE(rel, blkno, &vmbuffer)) - check_visible = true; - if (!check_visible && !check_frozen) - continue; - /* Read and lock the page. */ - buffer = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, - bstrategy); + buffer = read_stream_next_buffer(stream, NULL); + + /* + * If the read stream returns an InvalidBuffer, this means all the + * blocks are processed. So, end the stream and loop. + */ + if (buffer == InvalidBuffer) + { + read_stream_end(stream); + stream = NULL; + break; + } + LockBuffer(buffer, BUFFER_LOCK_SHARE); page = BufferGetPage(buffer); @@ -720,9 +784,9 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) * The visibility map bits might have changed while we were acquiring * the page lock. Recheck to avoid returning spurious results. */ - if (check_frozen && !VM_ALL_FROZEN(rel, blkno, &vmbuffer)) + if (all_frozen && !VM_ALL_FROZEN(rel, blkno, &vmbuffer)) check_frozen = false; - if (check_visible && !VM_ALL_VISIBLE(rel, blkno, &vmbuffer)) + if (all_visible && !VM_ALL_VISIBLE(rel, blkno, &vmbuffer)) check_visible = false; if (!check_visible && !check_frozen) { @@ -807,6 +871,11 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) UnlockReleaseBuffer(buffer); } + if (stream) + { + Assert(read_stream_next_buffer(stream, NULL) == InvalidBuffer); + read_stream_end(stream); + } /* Clean up. */ if (vmbuffer != InvalidBuffer) -- 2.45.2 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Use read streams in pg_visibility @ 2024-08-20 18:47 Noah Misch <[email protected]> parent: Nazir Bilal Yavuz <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Noah Misch @ 2024-08-20 18:47 UTC (permalink / raw) To: Nazir Bilal Yavuz <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Tue, Aug 13, 2024 at 03:22:27PM +0300, Nazir Bilal Yavuz wrote: > 2- collect_corrupt_items() > > This one is more complicated. The read stream callback function loops > until it finds a suitable block to read. So, if the callback returns > an InvalidBlockNumber; it means that the stream processed all possible > blocks and the stream should be finished. There is ~3% timing > improvement with this change. I started the server with the default > settings and created a 6 GB table. Then run 100 times > pg_check_visible() by clearing the OS cache between each run. > > The downside of this approach is there are too many "vmbuffer is valid > but BufferGetBlockNumber(*vmbuffer) is not equal to mapBlock, so > vmbuffer needs to be read again" cases in the read stream version (700 > vs 20 for the 6 GB table). This is caused by the callback function of > the read stream reading a new vmbuf while getting next block numbers. > So, vmbuf is wrong when we are checking visibility map bits that might > have changed while we were acquiring the page lock. Did the test that found 700 "read again" use a different procedure than the one shared as setup.sh down-thread? Using that script alone, none of the vm bits are set, so I'd not expect any heap page reads. The "vmbuffer needs to be read again" regression fits what I would expect the v1 patch to do with a table having many vm bits set. In general, I think the fix is to keep two heap Buffer vars, so the callback can work on one vmbuffer while collect_corrupt_items() works on another vmbuffer. Much of the time, they'll be the same buffer. It could be as simple as that, but you could consider further optimizations like these: - Use ReadRecentBuffer() or a similar technique, to avoid a buffer mapping lookup when the other Buffer var already has the block you want. - [probably not worth it] Add APIs for pg_visibility.c to tell read_stream.c to stop calling the ReadStreamBlockNumberCB for awhile. This could help if nonzero vm bits are infrequent, causing us to visit few heap blocks per vm block. For example, if one block out of every 33000 is all-visible, every heap block we visit has a different vmbuffer. It's likely not optimal for the callback to pin and unpin 20 vmbuffers, then have collect_corrupt_items() pin and unpin the same 20 vmbuffers. pg_visibility could notice this trend and request a stop of the callbacks until more of the heap block work completes. If pg_visibility is going to be the only place in the code with this use case, it's probably not worth carrying the extra API just for pg_visibility. However, if we get a stronger use case later, pg_visibility could be another beneficiary. > +/* > + * Callback function to get next block for read stream object used in > + * collect_visibility_data() function. > + */ > +static BlockNumber > +collect_visibility_data_read_stream_next_block(ReadStream *stream, > + void *callback_private_data, > + void *per_buffer_data) > +{ > + struct collect_visibility_data_read_stream_private *p = callback_private_data; > + > + if (p->blocknum < p->nblocks) > + return p->blocknum++; > + > + return InvalidBlockNumber; This is the third callback that just iterates over a block range, after pg_prewarm_read_stream_next_block() and copy_storage_using_buffer_read_stream_next_block(). While not a big problem, I think it's time to have a general-use callback for block range scans. The quantity of duplicate code is low, but the existing function names are long and less informative than a behavior-based name. > +static BlockNumber > +collect_corrupt_items_read_stream_next_block(ReadStream *stream, > + void *callback_private_data, > + void *per_buffer_data) > +{ > + struct collect_corrupt_items_read_stream_private *p = callback_private_data; > + > + for (; p->blocknum < p->nblocks; p->blocknum++) > + { > + bool check_frozen = false; > + bool check_visible = false; > + > + if (p->all_frozen && VM_ALL_FROZEN(p->rel, p->blocknum, p->vmbuffer)) > + check_frozen = true; > + if (p->all_visible && VM_ALL_VISIBLE(p->rel, p->blocknum, p->vmbuffer)) > + check_visible = true; > + if (!check_visible && !check_frozen) > + continue; If a vm has zero bits set, this loop will scan the entire vm before returning. Hence, this loop deserves a CHECK_FOR_INTERRUPTS() or a comment about how VM_ALL_FROZEN/VM_ALL_VISIBLE reaches a CHECK_FOR_INTERRUPTS(). > @@ -687,6 +734,20 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) > items->count = 64; > items->tids = palloc(items->count * sizeof(ItemPointerData)); > > + p.blocknum = 0; > + p.nblocks = nblocks; > + p.rel = rel; > + p.vmbuffer = &vmbuffer; > + p.all_frozen = all_frozen; > + p.all_visible = all_visible; > + stream = read_stream_begin_relation(READ_STREAM_FULL, > + bstrategy, > + rel, > + MAIN_FORKNUM, > + collect_corrupt_items_read_stream_next_block, > + &p, > + 0); > + > /* Loop over every block in the relation. */ > for (blkno = 0; blkno < nblocks; ++blkno) The callback doesn't return blocks having zero vm bits, so the blkno variable is not accurate. I didn't test, but I think the loop's "Recheck to avoid returning spurious results." looks at the bit for the wrong block. If that's what v1 does, could you expand the test file to catch that? For example, make a two-block table with only the second block all-visible. Since the callback skips blocks, this function should use the read_stream_reset() style of looping: while ((buffer = read_stream_next_buffer(stream, NULL)) != InvalidBuffer) ... Thanks, nm ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Use read streams in pg_visibility @ 2024-08-23 11:20 Nazir Bilal Yavuz <[email protected]> parent: Noah Misch <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Nazir Bilal Yavuz @ 2024-08-23 11:20 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> Hi, Thanks for the review and feedback! On Tue, 20 Aug 2024 at 21:47, Noah Misch <[email protected]> wrote: > > On Tue, Aug 13, 2024 at 03:22:27PM +0300, Nazir Bilal Yavuz wrote: > > 2- collect_corrupt_items() > > > > This one is more complicated. The read stream callback function loops > > until it finds a suitable block to read. So, if the callback returns > > an InvalidBlockNumber; it means that the stream processed all possible > > blocks and the stream should be finished. There is ~3% timing > > improvement with this change. I started the server with the default > > settings and created a 6 GB table. Then run 100 times > > pg_check_visible() by clearing the OS cache between each run. > > > > The downside of this approach is there are too many "vmbuffer is valid > > but BufferGetBlockNumber(*vmbuffer) is not equal to mapBlock, so > > vmbuffer needs to be read again" cases in the read stream version (700 > > vs 20 for the 6 GB table). This is caused by the callback function of > > the read stream reading a new vmbuf while getting next block numbers. > > So, vmbuf is wrong when we are checking visibility map bits that might > > have changed while we were acquiring the page lock. > > Did the test that found 700 "read again" use a different procedure than the > one shared as setup.sh down-thread? Using that script alone, none of the vm > bits are set, so I'd not expect any heap page reads. Yes, it is basically the same script but the query is 'SELECT pg_check_visible('${TABLE}'::regclass);'. > The "vmbuffer needs to be read again" regression fits what I would expect the > v1 patch to do with a table having many vm bits set. In general, I think the > fix is to keep two heap Buffer vars, so the callback can work on one vmbuffer > while collect_corrupt_items() works on another vmbuffer. Much of the time, > they'll be the same buffer. It could be as simple as that, but you could > consider further optimizations like these: Thanks for the suggestion. Keeping two vmbuffers lowered the count of "read-again" cases to ~40. I ran the test again and the timing improvement is ~5% now. > - Use ReadRecentBuffer() or a similar technique, to avoid a buffer mapping > lookup when the other Buffer var already has the block you want. > > - [probably not worth it] Add APIs for pg_visibility.c to tell read_stream.c > to stop calling the ReadStreamBlockNumberCB for awhile. This could help if > nonzero vm bits are infrequent, causing us to visit few heap blocks per vm > block. For example, if one block out of every 33000 is all-visible, every > heap block we visit has a different vmbuffer. It's likely not optimal for > the callback to pin and unpin 20 vmbuffers, then have > collect_corrupt_items() pin and unpin the same 20 vmbuffers. pg_visibility > could notice this trend and request a stop of the callbacks until more of > the heap block work completes. If pg_visibility is going to be the only > place in the code with this use case, it's probably not worth carrying the > extra API just for pg_visibility. However, if we get a stronger use case > later, pg_visibility could be another beneficiary. I think the number of "read-again" cases are low enough now. I am working on 'using ReadRecentBuffer() or a similar technique' but that may take more time, so I attached patches without these optimizations. > > +/* > > + * Callback function to get next block for read stream object used in > > + * collect_visibility_data() function. > > + */ > > +static BlockNumber > > +collect_visibility_data_read_stream_next_block(ReadStream *stream, > > + void *callback_private_data, > > + void *per_buffer_data) > > +{ > > + struct collect_visibility_data_read_stream_private *p = callback_private_data; > > + > > + if (p->blocknum < p->nblocks) > > + return p->blocknum++; > > + > > + return InvalidBlockNumber; > > This is the third callback that just iterates over a block range, after > pg_prewarm_read_stream_next_block() and > copy_storage_using_buffer_read_stream_next_block(). While not a big problem, > I think it's time to have a general-use callback for block range scans. The > quantity of duplicate code is low, but the existing function names are long > and less informative than a behavior-based name. I agree. Does creating something like pg_general_read_stream_next_block() in read_stream code and exporting it makes sense? > > +static BlockNumber > > +collect_corrupt_items_read_stream_next_block(ReadStream *stream, > > + void *callback_private_data, > > + void *per_buffer_data) > > +{ > > + struct collect_corrupt_items_read_stream_private *p = callback_private_data; > > + > > + for (; p->blocknum < p->nblocks; p->blocknum++) > > + { > > + bool check_frozen = false; > > + bool check_visible = false; > > + > > + if (p->all_frozen && VM_ALL_FROZEN(p->rel, p->blocknum, p->vmbuffer)) > > + check_frozen = true; > > + if (p->all_visible && VM_ALL_VISIBLE(p->rel, p->blocknum, p->vmbuffer)) > > + check_visible = true; > > + if (!check_visible && !check_frozen) > > + continue; > > If a vm has zero bits set, this loop will scan the entire vm before returning. > Hence, this loop deserves a CHECK_FOR_INTERRUPTS() or a comment about how > VM_ALL_FROZEN/VM_ALL_VISIBLE reaches a CHECK_FOR_INTERRUPTS(). Done. I added CHECK_FOR_INTERRUPTS() to the loop in callback. > > @@ -687,6 +734,20 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) > > items->count = 64; > > items->tids = palloc(items->count * sizeof(ItemPointerData)); > > > > + p.blocknum = 0; > > + p.nblocks = nblocks; > > + p.rel = rel; > > + p.vmbuffer = &vmbuffer; > > + p.all_frozen = all_frozen; > > + p.all_visible = all_visible; > > + stream = read_stream_begin_relation(READ_STREAM_FULL, > > + bstrategy, > > + rel, > > + MAIN_FORKNUM, > > + collect_corrupt_items_read_stream_next_block, > > + &p, > > + 0); > > + > > /* Loop over every block in the relation. */ > > for (blkno = 0; blkno < nblocks; ++blkno) > > The callback doesn't return blocks having zero vm bits, so the blkno variable > is not accurate. I didn't test, but I think the loop's "Recheck to avoid > returning spurious results." looks at the bit for the wrong block. If that's > what v1 does, could you expand the test file to catch that? For example, make > a two-block table with only the second block all-visible. Yes, it was not accurate. I am getting blockno from the buffer now. I checked and confirmed it is working as expected by manually logging blocknos returned from the read stream. I am not sure how to add a test case for this. > Since the callback skips blocks, this function should use the > read_stream_reset() style of looping: Done. There is one behavioral change introduced with the patches. It could happen when collect_corrupt_items() is called with both all_visible and all_frozen being true. -> Let's say VM_ALL_FROZEN() returns true but VM_ALL_VISIBLE() returns false in callback. Callback returns this block number because VM_ALL_FROZEN() is true. -> At the /* Recheck to avoid returning spurious results. */ part, we should only check VM_ALL_FROZEN() again as this was returning true in the callback. But we check both VM_ALL_FROZEN() and VM_ALL_VISIBLE(). VM_ALL_FROZEN() returns false and VM_ALL_VISIBLE() returns true now (vice versa of callback). -> We were skipping this block in the master but the patched version does not skip that. Is this a problem? If this is a problem, a solution might be to callback return values of VM_ALL_FROZEN() and VM_ALL_VISIBLE() in the per_buffer_data. v2 of the patches are attached, only 0002 is updated. -- Regards, Nazir Bilal Yavuz Microsoft Attachments: [text/x-patch] v2-0001-Use-read-stream-in-pg_visibility-in-collect_visib.patch (3.4K, ../../CAN55FZ3o1CoykUOVMxdAebPnseZ7vgxnqTjpKe1s4hHVkcyDFw@mail.gmail.com/2-v2-0001-Use-read-stream-in-pg_visibility-in-collect_visib.patch) download | inline diff: From e7ec4f3af86499b8433379b3b102eb58f75cf361 Mon Sep 17 00:00:00 2001 From: Nazir Bilal Yavuz <[email protected]> Date: Fri, 9 Aug 2024 11:41:52 +0300 Subject: [PATCH v2 1/2] Use read stream in pg_visibility in collect_visibility_data() Instead of reading blocks with ReadBufferExtended() in collect_visibility_data(), use read stream. This change provides about 4% performance improvement. --- contrib/pg_visibility/pg_visibility.c | 53 ++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c index 773ba92e454..ece67ccbe5f 100644 --- a/contrib/pg_visibility/pg_visibility.c +++ b/contrib/pg_visibility/pg_visibility.c @@ -21,6 +21,7 @@ #include "storage/bufmgr.h" #include "storage/proc.h" #include "storage/procarray.h" +#include "storage/read_stream.h" #include "storage/smgr.h" #include "utils/rel.h" #include "utils/snapmgr.h" @@ -41,6 +42,16 @@ typedef struct corrupt_items ItemPointer tids; } corrupt_items; +/* + * Helper struct for read stream object used in + * collect_visibility_data() function. + */ +struct collect_visibility_data_read_stream_private +{ + BlockNumber blocknum; + BlockNumber nblocks; +}; + PG_FUNCTION_INFO_V1(pg_visibility_map); PG_FUNCTION_INFO_V1(pg_visibility_map_rel); PG_FUNCTION_INFO_V1(pg_visibility); @@ -463,6 +474,23 @@ pg_visibility_tupdesc(bool include_blkno, bool include_pd) return BlessTupleDesc(tupdesc); } +/* + * Callback function to get next block for read stream object used in + * collect_visibility_data() function. + */ +static BlockNumber +collect_visibility_data_read_stream_next_block(ReadStream *stream, + void *callback_private_data, + void *per_buffer_data) +{ + struct collect_visibility_data_read_stream_private *p = callback_private_data; + + if (p->blocknum < p->nblocks) + return p->blocknum++; + + return InvalidBlockNumber; +} + /* * Collect visibility data about a relation. * @@ -478,6 +506,8 @@ collect_visibility_data(Oid relid, bool include_pd) BlockNumber blkno; Buffer vmbuffer = InvalidBuffer; BufferAccessStrategy bstrategy = GetAccessStrategy(BAS_BULKREAD); + struct collect_visibility_data_read_stream_private p; + ReadStream *stream = NULL; rel = relation_open(relid, AccessShareLock); @@ -489,6 +519,20 @@ collect_visibility_data(Oid relid, bool include_pd) info->next = 0; info->count = nblocks; + /* Crate a read stream if read will be done. */ + if (include_pd) + { + p.blocknum = 0; + p.nblocks = nblocks; + stream = read_stream_begin_relation(READ_STREAM_FULL, + bstrategy, + rel, + MAIN_FORKNUM, + collect_visibility_data_read_stream_next_block, + &p, + 0); + } + for (blkno = 0; blkno < nblocks; ++blkno) { int32 mapbits; @@ -513,8 +557,7 @@ collect_visibility_data(Oid relid, bool include_pd) Buffer buffer; Page page; - buffer = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, - bstrategy); + buffer = read_stream_next_buffer(stream, NULL); LockBuffer(buffer, BUFFER_LOCK_SHARE); page = BufferGetPage(buffer); @@ -525,6 +568,12 @@ collect_visibility_data(Oid relid, bool include_pd) } } + if (include_pd && stream) + { + Assert(read_stream_next_buffer(stream, NULL) == InvalidBuffer); + read_stream_end(stream); + } + /* Clean up. */ if (vmbuffer != InvalidBuffer) ReleaseBuffer(vmbuffer); -- 2.45.2 [text/x-patch] v2-0002-Use-read-stream-in-pg_visibility-in-collect_corru.patch (5.2K, ../../CAN55FZ3o1CoykUOVMxdAebPnseZ7vgxnqTjpKe1s4hHVkcyDFw@mail.gmail.com/3-v2-0002-Use-read-stream-in-pg_visibility-in-collect_corru.patch) download | inline diff: From 1ed469a6301d8ac889228c49198368028a6b6a0e Mon Sep 17 00:00:00 2001 From: Nazir Bilal Yavuz <[email protected]> Date: Fri, 23 Aug 2024 10:15:09 +0300 Subject: [PATCH v2 2/2] Use read stream in pg_visibility in collect_corrupt_items() Instead of reading blocks with ReadBufferExtended() in collect_corrupt_items(), use read stream. This change provides about 5% performance improvement. --- contrib/pg_visibility/pg_visibility.c | 87 ++++++++++++++++++++++----- 1 file changed, 72 insertions(+), 15 deletions(-) diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c index ece67ccbe5f..cfd94bd3be9 100644 --- a/contrib/pg_visibility/pg_visibility.c +++ b/contrib/pg_visibility/pg_visibility.c @@ -52,6 +52,20 @@ struct collect_visibility_data_read_stream_private BlockNumber nblocks; }; +/* + * Helper struct for read stream object used in + * collect_corrupt_items() function. + */ +struct collect_corrupt_items_read_stream_private +{ + bool all_frozen; + bool all_visible; + BlockNumber blocknum; + BlockNumber nblocks; + Relation rel; + Buffer *vmbuffer; +}; + PG_FUNCTION_INFO_V1(pg_visibility_map); PG_FUNCTION_INFO_V1(pg_visibility_map_rel); PG_FUNCTION_INFO_V1(pg_visibility); @@ -659,6 +673,38 @@ GetStrictOldestNonRemovableTransactionId(Relation rel) } } +/* + * Callback function to get next block for read stream object used in + * collect_corrupt_items() function. + */ +static BlockNumber +collect_corrupt_items_read_stream_next_block(ReadStream *stream, + void *callback_private_data, + void *per_buffer_data) +{ + struct collect_corrupt_items_read_stream_private *p = callback_private_data; + + for (; p->blocknum < p->nblocks; p->blocknum++) + { + bool check_frozen = false; + bool check_visible = false; + + /* Make sure we are interruptible. */ + CHECK_FOR_INTERRUPTS(); + + if (p->all_frozen && VM_ALL_FROZEN(p->rel, p->blocknum, p->vmbuffer)) + check_frozen = true; + if (p->all_visible && VM_ALL_VISIBLE(p->rel, p->blocknum, p->vmbuffer)) + check_visible = true; + if (!check_visible && !check_frozen) + continue; + + return p->blocknum++; + } + + return InvalidBlockNumber; +} + /* * Returns a list of items whose visibility map information does not match * the status of the tuples on the page. @@ -681,8 +727,12 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) corrupt_items *items; BlockNumber blkno; Buffer vmbuffer = InvalidBuffer; + Buffer stream_vmbuffer = InvalidBuffer; BufferAccessStrategy bstrategy = GetAccessStrategy(BAS_BULKREAD); TransactionId OldestXmin = InvalidTransactionId; + struct collect_corrupt_items_read_stream_private p; + ReadStream *stream; + Buffer buffer; rel = relation_open(relid, AccessShareLock); @@ -707,12 +757,25 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) items->count = 64; items->tids = palloc(items->count * sizeof(ItemPointerData)); + p.blocknum = 0; + p.nblocks = nblocks; + p.rel = rel; + p.vmbuffer = &stream_vmbuffer; + p.all_frozen = all_frozen; + p.all_visible = all_visible; + stream = read_stream_begin_relation(READ_STREAM_FULL, + bstrategy, + rel, + MAIN_FORKNUM, + collect_corrupt_items_read_stream_next_block, + &p, + 0); + /* Loop over every block in the relation. */ - for (blkno = 0; blkno < nblocks; ++blkno) + while ((buffer = read_stream_next_buffer(stream, NULL)) != InvalidBuffer) { bool check_frozen = false; bool check_visible = false; - Buffer buffer; Page page; OffsetNumber offnum, maxoff; @@ -720,29 +783,19 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) /* Make sure we are interruptible. */ CHECK_FOR_INTERRUPTS(); - /* Use the visibility map to decide whether to check this page. */ - if (all_frozen && VM_ALL_FROZEN(rel, blkno, &vmbuffer)) - check_frozen = true; - if (all_visible && VM_ALL_VISIBLE(rel, blkno, &vmbuffer)) - check_visible = true; - if (!check_visible && !check_frozen) - continue; - - /* Read and lock the page. */ - buffer = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, - bstrategy); LockBuffer(buffer, BUFFER_LOCK_SHARE); page = BufferGetPage(buffer); maxoff = PageGetMaxOffsetNumber(page); + blkno = BufferGetBlockNumber(buffer); /* * The visibility map bits might have changed while we were acquiring * the page lock. Recheck to avoid returning spurious results. */ - if (check_frozen && !VM_ALL_FROZEN(rel, blkno, &vmbuffer)) + if (all_frozen && !VM_ALL_FROZEN(rel, blkno, &vmbuffer)) check_frozen = false; - if (check_visible && !VM_ALL_VISIBLE(rel, blkno, &vmbuffer)) + if (all_visible && !VM_ALL_VISIBLE(rel, blkno, &vmbuffer)) check_visible = false; if (!check_visible && !check_frozen) { @@ -827,10 +880,14 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen) UnlockReleaseBuffer(buffer); } + Assert(read_stream_next_buffer(stream, NULL) == InvalidBuffer); + read_stream_end(stream); /* Clean up. */ if (vmbuffer != InvalidBuffer) ReleaseBuffer(vmbuffer); + if (stream_vmbuffer != InvalidBuffer) + ReleaseBuffer(stream_vmbuffer); relation_close(rel, AccessShareLock); /* -- 2.45.2 ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-08-23 11:20 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-09-12 13:07 [PATCH 7/8] Ignore correlation for new BRIN opclasses Tomas Vondra <[email protected]> 2024-08-13 12:22 Use read streams in pg_visibility Nazir Bilal Yavuz <[email protected]> 2024-08-20 18:47 ` Re: Use read streams in pg_visibility Noah Misch <[email protected]> 2024-08-23 11:20 ` Re: Use read streams in pg_visibility Nazir Bilal Yavuz <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox