public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 4/4] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>) 4+ messages / 3 participants [nested] [flat]
* [PATCH 4/4] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>) @ 2020-04-01 01:35 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Justin Pryzby @ 2020-04-01 01:35 UTC (permalink / raw) --- doc/src/sgml/ref/cluster.sgml | 12 ++++- doc/src/sgml/ref/vacuum.sgml | 12 ++++- src/backend/commands/cluster.c | 56 ++++++++++++++--------- src/backend/commands/matview.c | 3 +- src/backend/commands/tablecmds.c | 2 +- src/backend/commands/vacuum.c | 40 ++++++++-------- src/backend/postmaster/autovacuum.c | 1 + src/include/commands/cluster.h | 4 +- src/include/commands/vacuum.h | 5 +- src/test/regress/input/tablespace.source | 13 ++++++ src/test/regress/output/tablespace.source | 20 ++++++++ 11 files changed, 117 insertions(+), 51 deletions(-) diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index 6758ef97a6..9c0f5add59 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -29,6 +29,7 @@ CLUSTER [VERBOSE] VERBOSE [ <replaceable class="parameter">boolean</replaceable> ] TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> + INDEX_TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> </synopsis> </refsynopsisdiv> @@ -106,6 +107,15 @@ CLUSTER [VERBOSE] </listitem> </varlistentry> + <varlistentry> + <term><literal>INDEX_TABLESPACE</literal></term> + <listitem> + <para> + Specifies that the table's indexes will be rebuilt on a new tablespace. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><literal>TABLESPACE</literal></term> <listitem> @@ -128,7 +138,7 @@ CLUSTER [VERBOSE] <term><replaceable class="parameter">new_tablespace</replaceable></term> <listitem> <para> - The tablespace where the table will be rebuilt. + The tablespace where the table or its indexes will be rebuilt. </para> </listitem> </varlistentry> diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 5261a7c727..28cab119b6 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -36,6 +36,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet TRUNCATE [ <replaceable class="parameter">boolean</replaceable> ] PARALLEL <replaceable class="parameter">integer</replaceable> TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> + INDEX_TABLESPACE <replaceable class="parameter">new_tablespace</replaceable> <phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase> @@ -265,6 +266,15 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet </listitem> </varlistentry> + <varlistentry> + <term><literal>INDEX_TABLESPACE</literal></term> + <listitem> + <para> + Specifies that the relation's indexes will be rebuilt on a new tablespace. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><replaceable class="parameter">boolean</replaceable></term> <listitem> @@ -314,7 +324,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet <term><replaceable class="parameter">new_tablespace</replaceable></term> <listitem> <para> - The tablespace where the relation will be rebuilt. + The tablespace where the relation or its indexes will be rebuilt. </para> </listitem> </varlistentry> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 626321e3ac..7fc6b69e95 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -71,7 +71,7 @@ typedef struct static void rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, - Oid NewTableSpaceOid); + Oid NewTableSpaceOid, Oid NewIdxTableSpaceOid); static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex, bool verbose, bool *pSwapToastByContent, TransactionId *pFreezeXid, MultiXactId *pCutoffMulti); @@ -108,8 +108,9 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) ListCell *lc; ClusterParams params = {0}; bool verbose = false; - /* Name of tablespace to use for clustered relation. */ - char *tablespaceName = NULL; + /* Names of tablespaces to use for clustered relations. */ + char *tablespaceName = NULL, + *idxtablespaceName = NULL; /* Parse option list */ foreach(lc, stmt->params) @@ -120,6 +121,8 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) verbose = defGetBoolean(opt); else if (strcmp(opt->defname, "tablespace") == 0) tablespaceName = defGetString(opt); + else if (strcmp(opt->defname, "index_tablespace") == 0) + idxtablespaceName = defGetString(opt); else ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -130,18 +133,11 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) params.options = (verbose ? CLUOPT_VERBOSE : 0); - /* Select tablespace Oid to use. */ - if (tablespaceName) - { - params.tablespaceOid = get_tablespace_oid(tablespaceName, false); - - /* Can't move a non-shared relation into pg_global */ - if (params.tablespaceOid == GLOBALTABLESPACE_OID) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot move non-shared relation to tablespace \"%s\"", - tablespaceName))); - } + /* Get tablespaces to use. */ + params.tablespaceOid = tablespaceName ? + get_tablespace_oid(tablespaceName, false) : InvalidOid; + params.idxtablespaceOid = idxtablespaceName ? + get_tablespace_oid(idxtablespaceName, false) : InvalidOid; if (stmt->relation != NULL) { @@ -293,8 +289,9 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) * instead of index order. This is the new implementation of VACUUM FULL, * and error messages should refer to the operation as VACUUM not CLUSTER. * - * "tablespaceOid" is the tablespace where the relation will be rebuilt, or - * InvalidOid to use its current tablespace. + * "tablespaceOid" and "idxtablespaceOid" are the tablespaces where the relation + * and its indexes will be rebuilt, or InvalidOid to use their current + * tablespaces. */ void cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params) @@ -464,7 +461,8 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params) TransferPredicateLocksToHeapRelation(OldHeap); /* rebuild_relation does all the dirty work */ - rebuild_relation(OldHeap, indexOid, verbose, params->tablespaceOid); + rebuild_relation(OldHeap, indexOid, verbose, params->tablespaceOid, + params->idxtablespaceOid); /* NB: rebuild_relation does table_close() on OldHeap */ @@ -613,10 +611,11 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal) * NB: this routine closes OldHeap at the right time; caller should not. */ static void -rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespaceOid) +rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespaceOid, Oid NewIdxTablespaceOid) { Oid tableOid = RelationGetRelid(OldHeap); Oid tableSpace = OldHeap->rd_rel->reltablespace; + Oid idxtableSpace; Oid OIDNewHeap; char relpersistence; bool is_system_catalog; @@ -626,7 +625,20 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespace /* Use new tablespace if passed. */ if (OidIsValid(NewTablespaceOid)) + { tableSpace = NewTablespaceOid; + /* It's not a shared catalog, so refuse to move it to shared tablespace */ + if (tableSpace == GLOBALTABLESPACE_OID) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot move non-shared relation to tablespace \"%s\"", + get_tablespace_name(tableSpace)))); + } + + if (OidIsValid(NewIdxTablespaceOid)) + idxtableSpace = NewIdxTablespaceOid; + else + idxtableSpace = get_rel_tablespace(indexOid); /* Mark the correct index as clustered */ if (OidIsValid(indexOid)) @@ -655,7 +667,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespace finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog, swap_toast_by_content, false, true, frozenXid, cutoffMulti, - relpersistence); + relpersistence, idxtableSpace); } @@ -1403,12 +1415,12 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap, bool is_internal, TransactionId frozenXid, MultiXactId cutoffMulti, - char newrelpersistence) + char newrelpersistence, Oid idxtableSpace) { ObjectAddress object; Oid mapped_tables[4]; int reindex_flags; - ReindexParams reindex_params = {0}; + ReindexParams reindex_params = { .tablespaceOid = idxtableSpace }; int i; /* Report that we are now swapping relation files */ diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c index c5c25ce11d..ad1bfdc0d2 100644 --- a/src/backend/commands/matview.c +++ b/src/backend/commands/matview.c @@ -855,7 +855,8 @@ static void refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap, char relpersistence) { finish_heap_swap(matviewOid, OIDNewHeap, false, false, true, true, - RecentXmin, ReadNextMultiXactId(), relpersistence); + RecentXmin, ReadNextMultiXactId(), relpersistence, + InvalidOid); } /* diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 75b04eb161..eba8478f91 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -5077,7 +5077,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode, !OidIsValid(tab->newTableSpace), RecentXmin, ReadNextMultiXactId(), - persistence); + persistence, InvalidOid); } else { diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 61565c3f14..0eb9786bea 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -109,9 +109,9 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) bool disable_page_skipping = false; ListCell *lc; - /* Name and Oid of tablespace to use for relations after VACUUM FULL. */ - char *tablespacename = NULL; - Oid tablespaceOid = InvalidOid; + /* Tablespace to use for relations after VACUUM FULL. */ + char *tablespacename = NULL, + *idxtablespacename = NULL; /* Set default value */ params.index_cleanup = VACOPT_TERNARY_DEFAULT; @@ -151,6 +151,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) params.truncate = get_vacopt_ternary_value(opt); else if (strcmp(opt->defname, "tablespace") == 0) tablespacename = defGetString(opt); + else if (strcmp(opt->defname, "index_tablespace") == 0) + idxtablespacename = defGetString(opt); else if (strcmp(opt->defname, "parallel") == 0) { if (opt->arg == NULL) @@ -211,26 +213,18 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("VACUUM FULL cannot be performed in parallel"))); - /* Get tablespace Oid to use. */ - if (tablespacename) - { - if ((params.options & VACOPT_FULL) == 0) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("incompatible TABLESPACE option"), - errdetail("TABLESPACE can only be used with VACUUM FULL."))); - - tablespaceOid = get_tablespace_oid(tablespacename, false); - - /* Can't move a non-shared relation into pg_global */ - if (tablespaceOid == GLOBALTABLESPACE_OID) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot move non-shared relation to tablespace \"%s\"", - tablespacename))); + if ((params.options & VACOPT_FULL) == 0 && + (tablespacename || idxtablespacename)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("incompatible TABLESPACE option"), + errdetail("TABLESPACE can only be used with VACUUM FULL."))); - } - params.tablespace_oid = tablespaceOid; + /* Get tablespace Oids to use. */ + params.tablespace_oid = tablespacename ? + get_tablespace_oid(tablespacename, false) : InvalidOid; + params.idxtablespace_oid = idxtablespacename ? + get_tablespace_oid(idxtablespacename, false) : InvalidOid; /* * Make sure VACOPT_ANALYZE is specified if any column lists are present. @@ -1964,6 +1958,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params) { ClusterParams cluster_params = { .tablespaceOid = params->tablespace_oid, + .idxtablespaceOid = params->idxtablespace_oid, + /* Other params initialized to 0/false/NULL */ }; /* close relation before vacuuming, but hold lock until commit */ diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index a5d98c8c95..fd9eb75e11 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -2933,6 +2933,7 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, tab->at_params.is_wraparound = wraparound; tab->at_params.log_min_duration = log_min_duration; tab->at_params.tablespace_oid = InvalidOid; + tab->at_params.idxtablespace_oid = InvalidOid; tab->at_vacuum_cost_limit = vac_cost_limit; tab->at_vacuum_cost_delay = vac_cost_delay; tab->at_relname = NULL; diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h index 868dea5ecc..c04a2ce20b 100644 --- a/src/include/commands/cluster.h +++ b/src/include/commands/cluster.h @@ -28,6 +28,7 @@ typedef struct ClusterParams { bits32 options; /* bitmask of CLUOPT_* */ Oid tablespaceOid; /* tablespace to rebuild relation, or InvalidOid */ + Oid idxtablespaceOid; /* tablespace to rebuild relation's indexes */ } ClusterParams; extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel); @@ -45,6 +46,7 @@ extern void finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap, bool is_internal, TransactionId frozenXid, MultiXactId minMulti, - char newrelpersistence); + char newrelpersistence, + Oid idxtablespaceOid); #endif /* CLUSTER_H */ diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 0934487d4b..6c4e085a45 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -227,8 +227,9 @@ typedef struct VacuumParams * disabled. */ int nworkers; - Oid tablespace_oid; /* tablespace to use for relations - * rebuilt by VACUUM FULL */ + /* tablespaces to use for relations rebuilt by VACUUM FULL */ + Oid tablespace_oid; + Oid idxtablespace_oid; } VacuumParams; /* GUC parameters */ diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source index 2244c1d51d..9a13112605 100644 --- a/src/test/regress/input/tablespace.source +++ b/src/test/regress/input/tablespace.source @@ -80,6 +80,19 @@ REINDEX (TABLESPACE pg_default) TABLE CONCURRENTLY regress_tblspace_test_tbl; -- SELECT relname FROM pg_class WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace'); +-- check CLUSTER with INDEX_TABLESPACE change to non-default location +CLUSTER (INDEX_TABLESPACE regress_tblspace) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- ok +-- check relations moved to new tablespace +SELECT relname FROM pg_class +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace') +ORDER BY relname; +-- check VACUUM with INDEX_TABLESPACE change +VACUUM (FULL, ANALYSE, FREEZE, INDEX_TABLESPACE pg_default) regress_tblspace_test_tbl; -- ok +-- check relations moved back to pg_default +SELECT relname FROM pg_class +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace'); + + -- create a schema we can use CREATE SCHEMA testschema; diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source index 889933a9c8..2f8ecf514d 100644 --- a/src/test/regress/output/tablespace.source +++ b/src/test/regress/output/tablespace.source @@ -114,6 +114,26 @@ WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspa --------- (0 rows) +-- check CLUSTER with INDEX_TABLESPACE change to non-default location +CLUSTER (INDEX_TABLESPACE regress_tblspace) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- ok +-- check relations moved to new tablespace +SELECT relname FROM pg_class +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace') +ORDER BY relname; + relname +------------------------------- + regress_tblspace_test_tbl_idx +(1 row) + +-- check VACUUM with INDEX_TABLESPACE change +VACUUM (FULL, ANALYSE, FREEZE, INDEX_TABLESPACE pg_default) regress_tblspace_test_tbl; -- ok +-- check relations moved back to pg_default +SELECT relname FROM pg_class +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace'); + relname +--------- +(0 rows) + -- create a schema we can use CREATE SCHEMA testschema; -- try a table -- 2.17.0 --tsOsTdHNUZQcU9Ye-- ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Use read streams in pg_visibility @ 2024-08-23 19:01 Noah Misch <[email protected]> 2024-08-27 07:49 ` Re: Use read streams in pg_visibility Nazir Bilal Yavuz <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Noah Misch @ 2024-08-23 19:01 UTC (permalink / raw) To: Nazir Bilal Yavuz <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Fri, Aug 23, 2024 at 02:20:06PM +0300, Nazir Bilal Yavuz wrote: > 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: > > > 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);'. I gather the scripts deal exclusively in tables with no vm bits set, so pg_visibility did no heap page reads under those scripts. But the '700 "read again"' result suggests either I'm guessing wrong, or that result came from a different test procedure. Thoughts? > > 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. > I think the number of "read-again" cases are low enough now. Agreed. The increase from 20 to 40 is probably an increase in buffer mapping lookups, not an increase in I/O. > Does creating something like > pg_general_read_stream_next_block() in read_stream code and exporting > it makes sense? To me, that name isn't conveying the function's behavior, and the words "pg_" and "general_" aren't adding information there. How about one of these names or similar: seq_read_stream_cb sequential_read_stream_cb block_range_read_stream_cb > > 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. VACUUM FREEZE makes an all-visible, all-frozen table. DELETE of a particular TID, even if rolled back, clears both vm bits for the TID's page. Past tests like that had instability problems. One cause is a concurrent session's XID or snapshot, which can prevent VACUUM setting vm bits. Using a TEMP table may have been one of the countermeasures, but I don't remember clearly. Hence, please search the archives or the existing pg_visibility tests for how we dealt with that. It may not be problem for this particular test. > 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. No, I don't consider that a problem. That's not making us do additional I/O, just testing more bits within the pages we're already loading. The difference in behavior is user-visible, but it's the same behavior change the user would get if the timing had been a bit different. ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Use read streams in pg_visibility 2024-08-23 19:01 Re: Use read streams in pg_visibility Noah Misch <[email protected]> @ 2024-08-27 07:49 ` Nazir Bilal Yavuz <[email protected]> 2024-08-30 23:51 ` Re: Use read streams in pg_visibility Noah Misch <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Nazir Bilal Yavuz @ 2024-08-27 07:49 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> Hi, On Fri, 23 Aug 2024 at 22:01, Noah Misch <[email protected]> wrote: > > On Fri, Aug 23, 2024 at 02:20:06PM +0300, Nazir Bilal Yavuz wrote: > > 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: > > > > 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);'. > > I gather the scripts deal exclusively in tables with no vm bits set, so > pg_visibility did no heap page reads under those scripts. But the '700 "read > again"' result suggests either I'm guessing wrong, or that result came from a > different test procedure. Thoughts? Sorry, yes. You need to run VACUUM on the test table before running the query. The script is attached. You can run the script with [corrupt | visibility] arguments to test the related patches. > > > 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. > > > I think the number of "read-again" cases are low enough now. > > Agreed. The increase from 20 to 40 is probably an increase in buffer mapping > lookups, not an increase in I/O. > > > Does creating something like > > pg_general_read_stream_next_block() in read_stream code and exporting > > it makes sense? > > To me, that name isn't conveying the function's behavior, and the words "pg_" > and "general_" aren't adding information there. How about one of these names > or similar: > > seq_read_stream_cb > sequential_read_stream_cb > block_range_read_stream_cb I liked the block_range_read_stream_cb. Attached patches for that (first 3 patches). I chose an nblocks way instead of last_blocks in the struct. > > > 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. > > VACUUM FREEZE makes an all-visible, all-frozen table. DELETE of a particular > TID, even if rolled back, clears both vm bits for the TID's page. Past tests > like that had instability problems. One cause is a concurrent session's XID > or snapshot, which can prevent VACUUM setting vm bits. Using a TEMP table may > have been one of the countermeasures, but I don't remember clearly. Hence, > please search the archives or the existing pg_visibility tests for how we > dealt with that. It may not be problem for this particular test. Thanks for the information, I will check these. What I still do not understand is how to make sure that only the second block is processed and the first one is skipped. pg_check_visible() and pg_check_frozen() returns TIDs that cause corruption in the visibility map, there is no information about block numbers. > > 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. > > No, I don't consider that a problem. That's not making us do additional I/O, > just testing more bits within the pages we're already loading. The difference > in behavior is user-visible, but it's the same behavior change the user would > get if the timing had been a bit different. Thanks for the clarification. -- Regards, Nazir Bilal Yavuz Microsoft Attachments: [application/x-patch] v3-0001-Add-general-use-struct-and-callback-to-read-strea.patch (2.8K, ../../CAN55FZ2NuoNdikpo5+OXeYaS+=+4upo4hug4ZV+Pj_nn7comgg@mail.gmail.com/2-v3-0001-Add-general-use-struct-and-callback-to-read-strea.patch) download | inline diff: From 55e8e78d05b135babf8ff8c84fb9747ad19c58c1 Mon Sep 17 00:00:00 2001 From: Nazir Bilal Yavuz <[email protected]> Date: Mon, 26 Aug 2024 12:12:52 +0300 Subject: [PATCH v3 1/5] Add general-use struct and callback to read stream Number of callbacks that just iterates over block range in read stream are increasing. So, add general-use struct and callback to read stream. --- src/include/storage/read_stream.h | 12 ++++++++++++ src/backend/storage/aio/read_stream.c | 17 +++++++++++++++++ src/tools/pgindent/typedefs.list | 1 + 3 files changed, 30 insertions(+) diff --git a/src/include/storage/read_stream.h b/src/include/storage/read_stream.h index 4e599904f26..7613c1e361c 100644 --- a/src/include/storage/read_stream.h +++ b/src/include/storage/read_stream.h @@ -45,11 +45,23 @@ struct ReadStream; typedef struct ReadStream ReadStream; +/* + * General-use struct to use in callback functions for block range scans. + */ +typedef struct BlockRangeReadStreamPrivate +{ + BlockNumber blocknum; + BlockNumber nblocks; +} BlockRangeReadStreamPrivate; + /* Callback that returns the next block number to read. */ typedef BlockNumber (*ReadStreamBlockNumberCB) (ReadStream *stream, void *callback_private_data, void *per_buffer_data); +extern BlockNumber block_range_read_stream_cb(ReadStream *stream, + void *callback_private_data, + void *per_buffer_data); extern ReadStream *read_stream_begin_relation(int flags, BufferAccessStrategy strategy, Relation rel, diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c index a83c18c2a4b..937d001ef08 100644 --- a/src/backend/storage/aio/read_stream.c +++ b/src/backend/storage/aio/read_stream.c @@ -166,6 +166,23 @@ get_per_buffer_data(ReadStream *stream, int16 buffer_index) stream->per_buffer_data_size * buffer_index; } +/* + * General-use callback function for block range scans. Callback loops between + * blocknum (inclusive) and nblocks (exclusive). + */ +BlockNumber +block_range_read_stream_cb(ReadStream *stream, + void *callback_private_data, + void *per_buffer_data) +{ + BlockRangeReadStreamPrivate *p = callback_private_data; + + if (p->blocknum < p->nblocks) + return p->blocknum++; + + return InvalidBlockNumber; +} + /* * Ask the callback which block it would like us to read next, with a small * buffer in front to allow read_stream_unget_block() to work and to allow the diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 9e951a9e6f3..df3f336bec0 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -275,6 +275,7 @@ BlockId BlockIdData BlockInfoRecord BlockNumber +BlockRangeReadStreamPrivate BlockRefTable BlockRefTableBuffer BlockRefTableChunk -- 2.45.2 [application/x-patch] v3-0002-pg_prewarm-Use-generic-use-read-stream-struct-and.patch (1.9K, ../../CAN55FZ2NuoNdikpo5+OXeYaS+=+4upo4hug4ZV+Pj_nn7comgg@mail.gmail.com/3-v3-0002-pg_prewarm-Use-generic-use-read-stream-struct-and.patch) download | inline diff: From 01de5ff954f7db6a749ebfc0b1281ed010b5e8f9 Mon Sep 17 00:00:00 2001 From: Nazir Bilal Yavuz <[email protected]> Date: Mon, 26 Aug 2024 12:16:06 +0300 Subject: [PATCH v3 2/5] pg_prewarm: Use generic-use read stream struct and callback Instead of creating another read stream struct and callback, use generic one that is exported from read stream. --- contrib/pg_prewarm/pg_prewarm.c | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c index 5c859e983c5..889639a09a3 100644 --- a/contrib/pg_prewarm/pg_prewarm.c +++ b/contrib/pg_prewarm/pg_prewarm.c @@ -39,25 +39,6 @@ typedef enum static PGIOAlignedBlock blockbuffer; -struct pg_prewarm_read_stream_private -{ - BlockNumber blocknum; - int64 last_block; -}; - -static BlockNumber -pg_prewarm_read_stream_next_block(ReadStream *stream, - void *callback_private_data, - void *per_buffer_data) -{ - struct pg_prewarm_read_stream_private *p = callback_private_data; - - if (p->blocknum <= p->last_block) - return p->blocknum++; - - return InvalidBlockNumber; -} - /* * pg_prewarm(regclass, mode text, fork text, * first_block int8, last_block int8) @@ -203,7 +184,7 @@ pg_prewarm(PG_FUNCTION_ARGS) } else if (ptype == PREWARM_BUFFER) { - struct pg_prewarm_read_stream_private p; + BlockRangeReadStreamPrivate p; ReadStream *stream; /* @@ -212,13 +193,13 @@ pg_prewarm(PG_FUNCTION_ARGS) /* Set up the private state for our streaming buffer read callback. */ p.blocknum = first_block; - p.last_block = last_block; + p.nblocks = last_block + 1; stream = read_stream_begin_relation(READ_STREAM_FULL, NULL, rel, forkNumber, - pg_prewarm_read_stream_next_block, + block_range_read_stream_cb, &p, 0); -- 2.45.2 [application/x-patch] v3-0003-RelationCopyStorageUsingBuffer-Use-generic-use-re.patch (2.2K, ../../CAN55FZ2NuoNdikpo5+OXeYaS+=+4upo4hug4ZV+Pj_nn7comgg@mail.gmail.com/4-v3-0003-RelationCopyStorageUsingBuffer-Use-generic-use-re.patch) download | inline diff: From afc8c50c39212e016f71ee287062c287fffbacc2 Mon Sep 17 00:00:00 2001 From: Nazir Bilal Yavuz <[email protected]> Date: Mon, 26 Aug 2024 12:24:26 +0300 Subject: [PATCH v3 3/5] RelationCopyStorageUsingBuffer: Use generic-use read stream struct and callback Instead of creating another read stream struct and callback, use generic one that is exported from read stream. --- src/backend/storage/buffer/bufmgr.c | 31 ++--------------------------- 1 file changed, 2 insertions(+), 29 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index fb38c7bdd45..c04e4733921 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -136,33 +136,6 @@ typedef struct SMgrSortArray SMgrRelation srel; } SMgrSortArray; -/* - * Helper struct for read stream object used in - * RelationCopyStorageUsingBuffer() function. - */ -struct copy_storage_using_buffer_read_stream_private -{ - BlockNumber blocknum; - BlockNumber nblocks; -}; - -/* - * Callback function to get next block for read stream object used in - * RelationCopyStorageUsingBuffer() function. - */ -static BlockNumber -copy_storage_using_buffer_read_stream_next_block(ReadStream *stream, - void *callback_private_data, - void *per_buffer_data) -{ - struct copy_storage_using_buffer_read_stream_private *p = callback_private_data; - - if (p->blocknum < p->nblocks) - return p->blocknum++; - - return InvalidBlockNumber; -} - /* GUC variables */ bool zero_damaged_pages = false; int bgwriter_lru_maxpages = 100; @@ -4710,7 +4683,7 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator, PGIOAlignedBlock buf; BufferAccessStrategy bstrategy_src; BufferAccessStrategy bstrategy_dst; - struct copy_storage_using_buffer_read_stream_private p; + BlockRangeReadStreamPrivate p; ReadStream *src_stream; SMgrRelation src_smgr; @@ -4750,7 +4723,7 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator, src_smgr, permanent ? RELPERSISTENCE_PERMANENT : RELPERSISTENCE_UNLOGGED, forkNum, - copy_storage_using_buffer_read_stream_next_block, + block_range_read_stream_cb, &p, 0); -- 2.45.2 [application/x-patch] v3-0004-Use-read-stream-in-pg_visibility-in-collect_visib.patch (2.4K, ../../CAN55FZ2NuoNdikpo5+OXeYaS+=+4upo4hug4ZV+Pj_nn7comgg@mail.gmail.com/5-v3-0004-Use-read-stream-in-pg_visibility-in-collect_visib.patch) download | inline diff: From fbbca6e1c27e45afae9137228811d9fa11a7a197 Mon Sep 17 00:00:00 2001 From: Nazir Bilal Yavuz <[email protected]> Date: Mon, 26 Aug 2024 15:45:20 +0300 Subject: [PATCH v3 4/5] 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 | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c index 773ba92e454..6c64f5ead7e 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" @@ -478,6 +479,8 @@ collect_visibility_data(Oid relid, bool include_pd) BlockNumber blkno; Buffer vmbuffer = InvalidBuffer; BufferAccessStrategy bstrategy = GetAccessStrategy(BAS_BULKREAD); + BlockRangeReadStreamPrivate p; + ReadStream *stream = NULL; rel = relation_open(relid, AccessShareLock); @@ -489,6 +492,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, + block_range_read_stream_cb, + &p, + 0); + } + for (blkno = 0; blkno < nblocks; ++blkno) { int32 mapbits; @@ -513,8 +530,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 +541,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 [application/x-patch] v3-0005-Use-read-stream-in-pg_visibility-in-collect_corru.patch (5.2K, ../../CAN55FZ2NuoNdikpo5+OXeYaS+=+4upo4hug4ZV+Pj_nn7comgg@mail.gmail.com/6-v3-0005-Use-read-stream-in-pg_visibility-in-collect_corru.patch) download | inline diff: From aae5ded7c3b3f7600f01674a6200645d10d07116 Mon Sep 17 00:00:00 2001 From: Nazir Bilal Yavuz <[email protected]> Date: Fri, 23 Aug 2024 10:15:09 +0300 Subject: [PATCH v3 5/5] 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 6c64f5ead7e..75a5b362fe2 100644 --- a/contrib/pg_visibility/pg_visibility.c +++ b/contrib/pg_visibility/pg_visibility.c @@ -42,6 +42,20 @@ typedef struct corrupt_items ItemPointer tids; } corrupt_items; +/* + * 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); @@ -632,6 +646,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. @@ -654,8 +700,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); @@ -680,12 +730,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; @@ -693,29 +756,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) { @@ -800,10 +853,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 [application/x-sh] setup.sh (977B, ../../CAN55FZ2NuoNdikpo5+OXeYaS+=+4upo4hug4ZV+Pj_nn7comgg@mail.gmail.com/7-setup.sh) download ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Use read streams in pg_visibility 2024-08-23 19:01 Re: Use read streams in pg_visibility Noah Misch <[email protected]> 2024-08-27 07:49 ` Re: Use read streams in pg_visibility Nazir Bilal Yavuz <[email protected]> @ 2024-08-30 23:51 ` Noah Misch <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Noah Misch @ 2024-08-30 23:51 UTC (permalink / raw) To: Nazir Bilal Yavuz <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Tue, Aug 27, 2024 at 10:49:19AM +0300, Nazir Bilal Yavuz wrote: > On Fri, 23 Aug 2024 at 22:01, Noah Misch <[email protected]> wrote: > > On Fri, Aug 23, 2024 at 02:20:06PM +0300, Nazir Bilal Yavuz wrote: > > > 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: > I liked the block_range_read_stream_cb. Attached patches for that > (first 3 patches). I chose an nblocks way instead of last_blocks in > the struct. To read blocks 10 and 11, I would expect to initialize the struct with one of: { .first=10, .nblocks=2 } { .first=10, .last_inclusive=11 } { .first=10, .last_exclusive=12 } With the patch's API, I would need {.first=10,.nblocks=12}. The struct field named "nblocks" behaves like a last_block_exclusive. Please either make the behavior an "nblocks" behavior or change the field name to replace the term "nblocks" with something matching the behavior. (I used longer field names in my examples here, to disambiguate those examples. It's okay if the final field names aren't those, as long as the field names and the behavior align.) > > > > 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. > > > > VACUUM FREEZE makes an all-visible, all-frozen table. DELETE of a particular > > TID, even if rolled back, clears both vm bits for the TID's page. Past tests > > like that had instability problems. One cause is a concurrent session's XID > > or snapshot, which can prevent VACUUM setting vm bits. Using a TEMP table may > > have been one of the countermeasures, but I don't remember clearly. Hence, > > please search the archives or the existing pg_visibility tests for how we > > dealt with that. It may not be problem for this particular test. > > Thanks for the information, I will check these. What I still do not > understand is how to make sure that only the second block is processed > and the first one is skipped. pg_check_visible() and pg_check_frozen() > returns TIDs that cause corruption in the visibility map, there is no > information about block numbers. I see what you're saying. collect_corrupt_items() needs a corrupt table to report anything; all corruption-free tables get the same output. Testing this would need extra C code or techniques like corrupt_page_checksum() to create the corrupt state. That wouldn't be a bad thing to have, but it's big enough that I'll consider it out of scope for $SUBJECT. With the callback change above, I'll be ready to push all this. ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-08-30 23:51 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-04-01 01:35 [PATCH 4/4] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>) Justin Pryzby <[email protected]> 2024-08-23 19:01 Re: Use read streams in pg_visibility Noah Misch <[email protected]> 2024-08-27 07:49 ` Re: Use read streams in pg_visibility Nazir Bilal Yavuz <[email protected]> 2024-08-30 23:51 ` Re: Use read streams in pg_visibility Noah Misch <[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