From 4f9abf4e69db12addf6b3023de473d6dc86f3f6d Mon Sep 17 00:00:00 2001 From: Alexander Korotkov Date: Mon, 8 Apr 2024 12:30:45 +0300 Subject: [PATCH v2] Generalize acquire_sample_rows() Reported-by: Bug: Discussion: Author: Reviewed-by: Tested-by: Backpatch-through: --- contrib/file_fdw/file_fdw.c | 13 ++++-- contrib/postgres_fdw/postgres_fdw.c | 13 ++++-- src/backend/commands/analyze.c | 66 +++++++++++++++++++---------- src/include/access/tableam.h | 21 +++++++-- src/include/commands/vacuum.h | 58 ++++++++++++++++++++++++- src/include/foreign/fdwapi.h | 3 +- src/tools/pgindent/typedefs.list | 1 + 7 files changed, 138 insertions(+), 37 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 249d82d3a05..9c162c099f2 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -139,7 +139,8 @@ static void fileReScanForeignScan(ForeignScanState *node); static void fileEndForeignScan(ForeignScanState *node); static bool fileAnalyzeForeignTable(Relation relation, AcquireSampleRowsFunc *func, - BlockNumber *totalpages); + BlockNumber *totalpages, + void **arg); static bool fileIsForeignScanParallelSafe(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte); @@ -162,7 +163,8 @@ static void estimate_costs(PlannerInfo *root, RelOptInfo *baserel, Cost *startup_cost, Cost *total_cost); static int file_acquire_sample_rows(Relation onerel, int elevel, HeapTuple *rows, int targrows, - double *totalrows, double *totaldeadrows); + double *totalrows, double *totaldeadrows, + void *arg); /* @@ -806,7 +808,8 @@ fileEndForeignScan(ForeignScanState *node) static bool fileAnalyzeForeignTable(Relation relation, AcquireSampleRowsFunc *func, - BlockNumber *totalpages) + BlockNumber *totalpages, + void **arg) { char *filename; bool is_program; @@ -845,6 +848,7 @@ fileAnalyzeForeignTable(Relation relation, *totalpages = 1; *func = file_acquire_sample_rows; + *arg = NULL; return true; } @@ -1122,7 +1126,8 @@ estimate_costs(PlannerInfo *root, RelOptInfo *baserel, static int file_acquire_sample_rows(Relation onerel, int elevel, HeapTuple *rows, int targrows, - double *totalrows, double *totaldeadrows) + double *totalrows, double *totaldeadrows, + void *arg) { int numrows = 0; double rowstoskip = -1; /* -1 means not set yet */ diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 142dcfc9957..08e53aa99d6 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -400,7 +400,8 @@ static void postgresExecForeignTruncate(List *rels, bool restart_seqs); static bool postgresAnalyzeForeignTable(Relation relation, AcquireSampleRowsFunc *func, - BlockNumber *totalpages); + BlockNumber *totalpages, + void **arg); static List *postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid); static void postgresGetForeignJoinPaths(PlannerInfo *root, @@ -501,7 +502,8 @@ static void process_query_params(ExprContext *econtext, static int postgresAcquireSampleRowsFunc(Relation relation, int elevel, HeapTuple *rows, int targrows, double *totalrows, - double *totaldeadrows); + double *totaldeadrows, + void *arg); static void analyze_row_processor(PGresult *res, int row, PgFdwAnalyzeState *astate); static void produce_tuple_asynchronously(AsyncRequest *areq, bool fetch); @@ -4921,7 +4923,8 @@ process_query_params(ExprContext *econtext, static bool postgresAnalyzeForeignTable(Relation relation, AcquireSampleRowsFunc *func, - BlockNumber *totalpages) + BlockNumber *totalpages, + void **arg) { ForeignTable *table; UserMapping *user; @@ -4931,6 +4934,7 @@ postgresAnalyzeForeignTable(Relation relation, /* Return the row-analysis function pointer */ *func = postgresAcquireSampleRowsFunc; + *arg = NULL; /* * Now we have to get the number of pages. It's annoying that the ANALYZE @@ -5057,7 +5061,8 @@ static int postgresAcquireSampleRowsFunc(Relation relation, int elevel, HeapTuple *rows, int targrows, double *totalrows, - double *totaldeadrows) + double *totaldeadrows, + void *arg) { PgFdwAnalyzeState astate; ForeignTable *table; diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index da27a13a3f0..d5b2e537361 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -80,7 +80,8 @@ static BufferAccessStrategy vac_strategy; static void do_analyze_rel(Relation onerel, VacuumParams *params, List *va_cols, - AcquireSampleRowsFunc acquirefunc, BlockNumber relpages, + AcquireSampleRowsFunc acquirefunc, + void *acquirefuncArg, BlockNumber relpages, bool inh, bool in_outer_xact, int elevel); static void compute_index_stats(Relation onerel, double totalrows, AnlIndexData *indexdata, int nindexes, @@ -88,9 +89,6 @@ static void compute_index_stats(Relation onerel, double totalrows, MemoryContext col_context); static VacAttrStats *examine_attribute(Relation onerel, int attnum, Node *index_expr); -static int acquire_sample_rows(Relation onerel, int elevel, - HeapTuple *rows, int targrows, - double *totalrows, double *totaldeadrows); static int compare_rows(const void *a, const void *b, void *arg); static int acquire_inherited_sample_rows(Relation onerel, int elevel, HeapTuple *rows, int targrows, @@ -116,6 +114,7 @@ analyze_rel(Oid relid, RangeVar *relation, Relation onerel; int elevel; AcquireSampleRowsFunc acquirefunc = NULL; + void *acquirefuncArg = NULL; BlockNumber relpages = 0; /* Select logging level */ @@ -193,7 +192,7 @@ analyze_rel(Oid relid, RangeVar *relation, { /* Use row acquisition function provided by table AM */ table_relation_analyze(onerel, &acquirefunc, - &relpages, vac_strategy); + &relpages, vac_strategy, &acquirefuncArg); } else if (onerel->rd_rel->relkind == RELKIND_FOREIGN_TABLE) { @@ -209,7 +208,8 @@ analyze_rel(Oid relid, RangeVar *relation, if (fdwroutine->AnalyzeForeignTable != NULL) ok = fdwroutine->AnalyzeForeignTable(onerel, &acquirefunc, - &relpages); + &relpages, + &acquirefuncArg); if (!ok) { @@ -248,15 +248,15 @@ analyze_rel(Oid relid, RangeVar *relation, * tables, which don't contain any rows. */ if (onerel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) - do_analyze_rel(onerel, params, va_cols, acquirefunc, + do_analyze_rel(onerel, params, va_cols, acquirefunc, acquirefuncArg, relpages, false, in_outer_xact, elevel); /* * If there are child tables, do recursive ANALYZE. */ if (onerel->rd_rel->relhassubclass) - do_analyze_rel(onerel, params, va_cols, acquirefunc, relpages, - true, in_outer_xact, elevel); + do_analyze_rel(onerel, params, va_cols, acquirefunc, acquirefuncArg, + relpages, true, in_outer_xact, elevel); /* * Close source relation now, but keep lock so that no one deletes it @@ -279,8 +279,8 @@ analyze_rel(Oid relid, RangeVar *relation, static void do_analyze_rel(Relation onerel, VacuumParams *params, List *va_cols, AcquireSampleRowsFunc acquirefunc, - BlockNumber relpages, bool inh, bool in_outer_xact, - int elevel) + void *acquirefuncArg, BlockNumber relpages, bool inh, + bool in_outer_xact, int elevel) { int attr_cnt, tcnt, @@ -526,7 +526,8 @@ do_analyze_rel(Relation onerel, VacuumParams *params, else numrows = (*acquirefunc) (onerel, elevel, rows, targrows, - &totalrows, &totaldeadrows); + &totalrows, &totaldeadrows, + acquirefuncArg); /* * Compute the statistics. Temporary results during the calculations for @@ -1149,11 +1150,13 @@ block_sampling_read_stream_next(ReadStream *stream, * block. The previous sampling method put too much credence in the row * density near the start of the heap. */ -static int +int acquire_sample_rows(Relation onerel, int elevel, HeapTuple *rows, int targrows, - double *totalrows, double *totaldeadrows) + double *totalrows, double *totaldeadrows, + void *arg) { + AcquireSampleRowsArg *cb = (AcquireSampleRowsArg *) arg; int numrows = 0; /* # rows now in reservoir */ double samplerows = 0; /* total # rows collected */ double liverows = 0; /* # live rows seen */ @@ -1188,7 +1191,7 @@ acquire_sample_rows(Relation onerel, int elevel, /* Prepare for sampling rows */ reservoir_init_selection_state(&rstate, targrows); - scan = heap_beginscan(onerel, NULL, 0, NULL, NULL, SO_TYPE_ANALYZE); + scan = table_beginscan_analyze(onerel); slot = table_slot_create(onerel, NULL); stream = read_stream_begin_relation(READ_STREAM_MAINTENANCE, @@ -1200,11 +1203,11 @@ acquire_sample_rows(Relation onerel, int elevel, 0); /* Outer loop over blocks to sample */ - while (heapam_scan_analyze_next_block(scan, stream)) + while (cb->scan_analyze_next_block(scan, stream)) { vacuum_delay_point(); - while (heapam_scan_analyze_next_tuple(scan, OldestXmin, &liverows, &deadrows, slot)) + while (cb->scan_analyze_next_tuple(scan, OldestXmin, &liverows, &deadrows, slot)) { /* * The first targrows sample rows are simply copied into the @@ -1256,7 +1259,7 @@ acquire_sample_rows(Relation onerel, int elevel, read_stream_end(stream); ExecDropSingleTupleTableSlot(slot); - heap_endscan(scan); + table_endscan(scan); /* * If we didn't find as many tuples as we wanted then we're done. No sort @@ -1333,10 +1336,18 @@ compare_rows(const void *a, const void *b, void *arg) */ void heapam_analyze(Relation relation, AcquireSampleRowsFunc *func, - BlockNumber *totalpages, BufferAccessStrategy bstrategy) + BlockNumber *totalpages, BufferAccessStrategy bstrategy, + void **arg) { + static AcquireSampleRowsArg cb = + { + .scan_analyze_next_block = heapam_scan_analyze_next_block, + .scan_analyze_next_tuple = heapam_scan_analyze_next_tuple + }; + *func = acquire_sample_rows; *totalpages = RelationGetNumberOfBlocks(relation); + *arg = &cb; vac_strategy = bstrategy; } @@ -1349,7 +1360,7 @@ heapam_analyze(Relation relation, AcquireSampleRowsFunc *func, * We fail and return zero if there are no inheritance children, or if all * children are foreign tables that don't support ANALYZE. */ -static int +int acquire_inherited_sample_rows(Relation onerel, int elevel, HeapTuple *rows, int targrows, double *totalrows, double *totaldeadrows) @@ -1357,6 +1368,7 @@ acquire_inherited_sample_rows(Relation onerel, int elevel, List *tableOIDs; Relation *rels; AcquireSampleRowsFunc *acquirefuncs; + void **acquirefuncArgs; double *relblocks; double totalblocks; int numrows, @@ -1402,6 +1414,8 @@ acquire_inherited_sample_rows(Relation onerel, int elevel, rels = (Relation *) palloc(list_length(tableOIDs) * sizeof(Relation)); acquirefuncs = (AcquireSampleRowsFunc *) palloc(list_length(tableOIDs) * sizeof(AcquireSampleRowsFunc)); + acquirefuncArgs = (void **) + palloc(list_length(tableOIDs) * sizeof(void *)); relblocks = (double *) palloc(list_length(tableOIDs) * sizeof(double)); totalblocks = 0; nrels = 0; @@ -1411,6 +1425,7 @@ acquire_inherited_sample_rows(Relation onerel, int elevel, Oid childOID = lfirst_oid(lc); Relation childrel; AcquireSampleRowsFunc acquirefunc = NULL; + void *acquirefuncArg = NULL; BlockNumber relpages = 0; /* We already got the needed lock */ @@ -1431,7 +1446,8 @@ acquire_inherited_sample_rows(Relation onerel, int elevel, { /* Use row acquisition function provided by table AM */ table_relation_analyze(childrel, &acquirefunc, - &relpages, vac_strategy); + &relpages, vac_strategy, + &acquirefuncArg); } else if (childrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE) { @@ -1447,7 +1463,8 @@ acquire_inherited_sample_rows(Relation onerel, int elevel, if (fdwroutine->AnalyzeForeignTable != NULL) ok = fdwroutine->AnalyzeForeignTable(childrel, &acquirefunc, - &relpages); + &relpages, + &acquirefuncArg); if (!ok) { @@ -1475,6 +1492,7 @@ acquire_inherited_sample_rows(Relation onerel, int elevel, has_child = true; rels[nrels] = childrel; acquirefuncs[nrels] = acquirefunc; + acquirefuncArgs[nrels] = acquirefuncArg; relblocks[nrels] = (double) relpages; totalblocks += (double) relpages; nrels++; @@ -1506,6 +1524,7 @@ acquire_inherited_sample_rows(Relation onerel, int elevel, { Relation childrel = rels[i]; AcquireSampleRowsFunc acquirefunc = acquirefuncs[i]; + void *acquirefuncArg = acquirefuncArgs[i]; double childblocks = relblocks[i]; /* @@ -1544,7 +1563,8 @@ acquire_inherited_sample_rows(Relation onerel, int elevel, /* Fetch a random sample of the child's rows */ childrows = (*acquirefunc) (childrel, elevel, rows + numrows, childtargrows, - &trows, &tdrows); + &trows, &tdrows, + acquirefuncArg); /* We may need to convert from child's rowtype to parent's */ if (childrows > 0 && diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index ec827ac12bf..f28044d8342 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -692,7 +692,8 @@ typedef struct TableAmRoutine void (*relation_analyze) (Relation relation, AcquireSampleRowsFunc *func, BlockNumber *totalpages, - BufferAccessStrategy bstrategy); + BufferAccessStrategy bstrategy, + void **arg); /* ------------------------------------------------------------------------ @@ -1020,6 +1021,19 @@ table_beginscan_tid(Relation rel, Snapshot snapshot) return rel->rd_tableam->scan_begin(rel, snapshot, 0, NULL, NULL, flags); } +/* + * table_beginscan_analyze is an alternative entry point for setting up a + * TableScanDesc for an ANALYZE scan. As with bitmap scans, it's worth using + * the same data structure although the behavior is rather different. + */ +static inline TableScanDesc +table_beginscan_analyze(Relation rel) +{ + uint32 flags = SO_TYPE_ANALYZE; + + return rel->rd_tableam->scan_begin(rel, NULL, 0, NULL, NULL, flags); +} + /* * End relation scan. */ @@ -1868,10 +1882,11 @@ table_index_validate_scan(Relation table_rel, */ static inline void table_relation_analyze(Relation relation, AcquireSampleRowsFunc *func, - BlockNumber *totalpages, BufferAccessStrategy bstrategy) + BlockNumber *totalpages, BufferAccessStrategy bstrategy, + void **arg) { relation->rd_tableam->relation_analyze(relation, func, - totalpages, bstrategy); + totalpages, bstrategy, arg); } /* ---------------------------------------------------------------------------- diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 9514f8b2fd8..8cf66a00dc2 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -21,9 +21,11 @@ #include "catalog/pg_class.h" #include "catalog/pg_statistic.h" #include "catalog/pg_type.h" +#include "executor/tuptable.h" #include "parser/parse_node.h" #include "storage/buf.h" #include "storage/lock.h" +#include "storage/read_stream.h" #include "utils/relcache.h" /* @@ -189,7 +191,8 @@ typedef struct VacAttrStats typedef int (*AcquireSampleRowsFunc) (Relation relation, int elevel, HeapTuple *rows, int targrows, double *totalrows, - double *totaldeadrows); + double *totaldeadrows, + void *arg); /* flag bits for VacuumParams->options */ #define VACOPT_VACUUM 0x01 /* do VACUUM */ @@ -390,12 +393,63 @@ extern void parallel_vacuum_cleanup_all_indexes(ParallelVacuumState *pvs, extern void parallel_vacuum_main(dsm_segment *seg, shm_toc *toc); /* in commands/analyze.c */ + +struct TableScanDescData; + +/* The struct to be passed as '*arg' to acquire_sample_rows */ +typedef struct +{ + /* + * Prepare to analyze block from `stream` of `scan`. The scan has been + * started with table_beginscan_analyze(). + * + * The callback may acquire resources like locks that are held until + * (*scan_analyze_next_tuple)() returns false. It e.g. can make sense to + * hold a lock until all tuples on a block have been analyzed by + * (*scan_analyze_next_tuple)(). + * + * The callback can return false if the block is not suitable for + * sampling, e.g. because it's a metapage that could never contain tuples. + * + * XXX: This obviously is primarily suited for block-based AMs. It's not + * clear what a good interface for non block based AMs would be, so there + * isn't one yet. + */ + bool (*scan_analyze_next_block) (struct TableScanDescData *scan, + ReadStream *stream); + + /* + * Iterate over tuples in the block selected with + * (*scan_analyze_next_block)() (which needs to have returned true, and + * this routine may not have returned false for the same block before). If + * a tuple that's suitable for sampling is found, true is returned and a + * tuple is stored in `slot`. + * + * *liverows and *deadrows are incremented according to the encountered + * tuples. + * + * Not every AM might have a meaningful concept of dead rows, in which + * case it's OK to not increment *deadrows - but note that that may + * influence autovacuum scheduling (see comment for relation_vacuum + * callback). + */ + bool (*scan_analyze_next_tuple) (struct TableScanDescData *scan, + TransactionId OldestXmin, + double *liverows, + double *deadrows, + TupleTableSlot *slot); +} AcquireSampleRowsArg; + extern void analyze_rel(Oid relid, RangeVar *relation, VacuumParams *params, List *va_cols, bool in_outer_xact, BufferAccessStrategy bstrategy); +extern int acquire_sample_rows(Relation onerel, int elevel, + HeapTuple *rows, int targrows, + double *totalrows, double *totaldeadrows, + void *arg); extern void heapam_analyze(Relation relation, AcquireSampleRowsFunc *func, BlockNumber *totalpages, - BufferAccessStrategy bstrategy); + BufferAccessStrategy bstrategy, void **arg); extern bool std_typanalyze(VacAttrStats *stats); diff --git a/src/include/foreign/fdwapi.h b/src/include/foreign/fdwapi.h index 0968e0a01ec..ebea559e531 100644 --- a/src/include/foreign/fdwapi.h +++ b/src/include/foreign/fdwapi.h @@ -151,7 +151,8 @@ typedef void (*ExplainDirectModify_function) (ForeignScanState *node, typedef bool (*AnalyzeForeignTable_function) (Relation relation, AcquireSampleRowsFunc *func, - BlockNumber *totalpages); + BlockNumber *totalpages, + void **arg); typedef List *(*ImportForeignSchema_function) (ImportForeignSchemaStmt *stmt, Oid serverOid); diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index cb78f11119c..0156ef27aba 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -22,6 +22,7 @@ AclItem AclMaskHow AclMode AclResult +AcquireSampleRowsArg AcquireSampleRowsFunc ActionList ActiveSnapshotElt -- 2.39.3 (Apple Git-145)