From 6595aea491b5f69e9205bbb72231435307e3a6ea Mon Sep 17 00:00:00 2001 From: Antonin Houska Date: Mon, 29 Jun 2026 13:10:49 +0200 Subject: [PATCH] Allow progress tracking of sub-commands. Some commands that support progress reporting run other commands which also need to report their progress. The typical case is that REPACK ("main" command) runs index build (sub-command). The current approach is to disable progress tracking when a command is running as a sub-command of another command. However, the progress reporting can be AM-specific, so it's not really practicable to suppress the progress reporting of the sub-commands entirely. Even if we managed to pass a flag down to all the relevant places, we could end up with progress reporting for the sub-command disabled in cases it's important to the user. Instead of disabling the progress tracking of the sub-commands, this patch allows both the main command and the sub-command to report their progress at the same time. --- contrib/file_fdw/file_fdw.c | 44 +++++++++++ src/backend/access/brin/brin.c | 3 + src/backend/access/transam/xact.c | 33 +++++++- src/backend/bootstrap/bootstrap.c | 11 ++- src/backend/catalog/heap.c | 13 ++- src/backend/catalog/index.c | 50 ++++++------ src/backend/catalog/toasting.c | 9 +++ src/backend/commands/indexcmds.c | 62 ++++++++------- src/backend/commands/repack.c | 35 ++++++-- src/backend/utils/activity/backend_progress.c | 79 ++++++++++++++++--- src/backend/utils/activity/backend_status.c | 1 + src/backend/utils/adt/pgstatfuncs.c | 15 +++- src/include/catalog/index.h | 9 +-- src/include/utils/backend_status.h | 19 +++++ 14 files changed, 294 insertions(+), 89 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 33a37d832ce..a60fb226320 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -36,6 +36,7 @@ #include "optimizer/pathnode.h" #include "optimizer/planmain.h" #include "optimizer/restrictinfo.h" +#include "utils/backend_status.h" #include "utils/acl.h" #include "utils/memutils.h" #include "utils/rel.h" @@ -119,6 +120,16 @@ typedef struct FileFdwExecutionState CopyFromState cstate; /* COPY execution state */ } FileFdwExecutionState; +/* + * Since progress tracking of multiple COPY commands is not supported, the + * first file_fdw node of the plan needs to set pgstat_track_activities to + * false during startup, and the last active node needs to restore the + * original value during shutdown. + */ +static bool save_pgstat_track_activities = false; +static int fdw_nodes = 0; +static int active_fdw_nodes = 0; + /* * SQL functions */ @@ -616,6 +627,12 @@ fileGetForeignPlan(PlannerInfo *root, { Index scan_relid = baserel->relid; + /* + * This seems to be the appropriate place to count file_fdw nodes in the + * plan. + */ + fdw_nodes++; + /* * We have no native ability to evaluate restriction clauses, so we just * put all the scan_clauses into the plan node's qual list for the @@ -695,6 +712,18 @@ fileBeginForeignScan(ForeignScanState *node, int eflags) /* Add any options from the plan (currently only convert_selectively) */ options = list_concat(options, plan->fdw_private); + /* + * Save the value of pgstat_track_activities if this is the first file_fdw + * node of a plan containing multiple file_fdw nodes, and disable the + * progress tracking. The monitoring infrastructure currently does not + * support monitoring of multiple COPY commands. + */ + if (fdw_nodes > 1 && active_fdw_nodes++ == 0) + { + save_pgstat_track_activities = pgstat_track_activities; + pgstat_track_activities = false; + } + /* * Create CopyState from FDW options. We always acquire all columns, so * as to match the expected ScanTupleSlot signature. @@ -861,6 +890,21 @@ fileEndForeignScan(ForeignScanState *node) festate->cstate->num_errors)); EndCopyFrom(festate->cstate); + + + /* + * Restore the value of pgstat_track_activities if this is the last + * file_fdw node of a plan containing multiple file_fdw nodes, and enable + * progress tracking if we disabled it earlier. + */ + if (active_fdw_nodes > 0) + { + if (--active_fdw_nodes == 0) + { + pgstat_track_activities = save_pgstat_track_activities; + fdw_nodes = 0; + } + } } /* diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index bdb30752e09..f84b992478a 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -2847,8 +2847,11 @@ _brin_parallel_scan_and_build(BrinBuildState *state, ParallelTableScanFromBrinShared(brinshared), SO_NONE); + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, + RelationGetRelid(heap)); reltuples = table_index_build_scan(heap, index, indexInfo, true, true, brinbuildCallbackParallel, state, scan); + pgstat_progress_end_command(); /* insert the last item */ form_and_spill_tuple(state); diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 3a89149016f..78a77fe4bb3 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -217,6 +217,8 @@ typedef struct TransactionStateData bool parallelChildXact; /* is any parent transaction parallel? */ bool chain; /* start a new block after this one */ bool topXidLogged; /* for a subxact: is top-level XID logged? */ + int progress_depth; /* progress tracking state before subxact + * started */ struct TransactionStateData *parent; /* back link to parent */ } TransactionStateData; @@ -2253,6 +2255,12 @@ StartTransaction(void) */ s->state = TRANS_INPROGRESS; + /* + * Top transaction always terminates progress tracking on abort, so this + * field is not needed, but be tidy. + */ + s->progress_depth = 0; + /* Schedule transaction timeout */ if (TransactionTimeout > 0) enable_timeout_after(TRANSACTION_TIMEOUT, TransactionTimeout); @@ -5139,6 +5147,12 @@ StartSubTransaction(void) s->state = TRANS_INPROGRESS; + /* + * Subtransaction needs to know if it should cancel progress reporting on + * abort. + */ + s->progress_depth = PGSTAT_PROGRESS_STATE(MyBEEntry); + /* * Call start-of-subxact callbacks */ @@ -5298,7 +5312,24 @@ AbortSubTransaction(void) WaitLSNCleanup(); pgstat_report_wait_end(); - pgstat_progress_end_command(); + + /* + * Abort of a subtransaction does not imply the end of monitoring for the + * whole transaction. Instead, we only cancel monitoring started within + * the subtransaction. + * + * One particular case it matters is an index function that catches an + * exception and lets the index build continue, so the abort should not + * cancel monitoring of that build. + * + * XXX Shouldn't we restore the progress state as it was before the + * subtransaction started? Not sure about use case - the index functions + * mentioned above probably don't start their own progress tracking. On + * the other hand, a monitored command started in a subtransaction usually + * does not have a parent command in the parent transaction. + */ + for (int i = PGSTAT_PROGRESS_STATE(MyBEEntry); i > s->progress_depth; i--) + pgstat_progress_end_command(); pgaio_error_cleanup(); diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index b0dcd9876c5..d15a06aa99c 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -40,6 +40,7 @@ #include "storage/ipc.h" #include "storage/proc.h" #include "storage/shmem_internal.h" +#include "utils/backend_progress.h" #include "utils/builtins.h" #include "utils/fmgroids.h" #include "utils/guc.h" @@ -1188,7 +1189,15 @@ build_indices(void) heap = table_open(ILHead->il_heap, NoLock); ind = index_open(ILHead->il_ind, NoLock); - index_build(heap, ind, ILHead->il_info, false, false, false); + /* + * Progress tracking is not really needed here, but it's simpler to + * start it than to disable it everywhere (including the specific + * AMs). + */ + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, + RelationGetRelid(heap)); + index_build(heap, ind, ILHead->il_info, false, false); + pgstat_progress_end_command(); index_close(ind, NoLock); table_close(heap, NoLock); diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 88087654de9..5339cfa51de 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -3588,10 +3588,15 @@ RelationTruncateIndexes(Relation heapRelation) */ RelationTruncate(currentIndex, 0); - /* Initialize the index and rebuild */ - /* Note: we do not need to re-establish pkey setting */ - index_build(heapRelation, currentIndex, indexInfo, true, false, - true); + /* + * Initialize the index and rebuild + * + * Note: we do not need to re-establish pkey setting + */ + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, + RelationGetRelid(heapRelation)); + index_build(heapRelation, currentIndex, indexInfo, true, false); + pgstat_progress_end_command(); /* We're done with this index */ index_close(currentIndex, NoLock); diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 9407c357f27..e5d12308b6e 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -715,8 +715,6 @@ UpdateIndexRelation(Oid indexoid, * already exists. * INDEX_CREATE_PARTITIONED: * create a partitioned index (table must be partitioned) - * INDEX_CREATE_SUPPRESS_PROGRESS: - * don't report progress during the index build. * * constr_flags: flags passed to index_constraint_create * (only if INDEX_CREATE_ADD_CONSTRAINT is set) @@ -763,7 +761,6 @@ index_create(Relation heapRelation, bool invalid = (flags & INDEX_CREATE_INVALID) != 0; bool concurrent = (flags & INDEX_CREATE_CONCURRENT) != 0; bool partitioned = (flags & INDEX_CREATE_PARTITIONED) != 0; - bool progress = (flags & INDEX_CREATE_SUPPRESS_PROGRESS) == 0; char relkind; TransactionId relfrozenxid; MultiXactId relminmxid; @@ -1280,8 +1277,7 @@ index_create(Relation heapRelation, } else { - index_build(heapRelation, indexRelation, indexInfo, false, true, - progress); + index_build(heapRelation, indexRelation, indexInfo, false, true); } /* @@ -1540,7 +1536,7 @@ index_concurrently_build(Oid heapRelationId, indexInfo->ii_BrokenHotChain = false; /* Now build the index */ - index_build(heapRel, indexRelation, indexInfo, false, true, true); + index_build(heapRel, indexRelation, indexInfo, false, true); /* Roll back any GUC changes executed by index functions */ AtEOXact_GUC(false, save_nestlevel); @@ -2221,6 +2217,8 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode) */ if (concurrent) { + bool progress; + /* * We must commit our transaction in order to make the first pg_index * state update visible to other sessions. If the DROP machinery has @@ -2291,11 +2289,12 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode) * to acquire an exclusive lock on our table. The lock code will * detect deadlock and error out properly. * - * Note: we report progress through WaitForLockers() unconditionally - * here, even though it will only be used when we're called by REINDEX - * CONCURRENTLY and not when called by DROP INDEX CONCURRENTLY. + * Note: we report progress through WaitForLockers() only when the + * command is REINDEX CONCURRENTLY. There's no progress reporting for + * DROP INDEX CONCURRENTLY. */ - WaitForLockers(heaplocktag, AccessExclusiveLock, true); + progress = MyBEEntry->st_progress_command == PROGRESS_COMMAND_CREATE_INDEX; + WaitForLockers(heaplocktag, AccessExclusiveLock, progress); /* * Updating pg_index might involve TOAST table access, so ensure we @@ -2319,7 +2318,7 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode) * Wait till every transaction that saw the old index state has * finished. See above about progress reporting. */ - WaitForLockers(heaplocktag, AccessExclusiveLock, true); + WaitForLockers(heaplocktag, AccessExclusiveLock, progress); /* * Re-open relations to allow us to complete our actions. @@ -3011,7 +3010,6 @@ index_update_stats(Relation rel, * * isreindex indicates we are recreating a previously-existing index. * parallel indicates if parallelism may be useful. - * progress indicates if the backend should update its progress info. * * Note: before Postgres 8.2, the passed-in heap and index Relations * were automatically closed by this routine. This is no longer the case. @@ -3022,8 +3020,7 @@ index_build(Relation heapRelation, Relation indexRelation, IndexInfo *indexInfo, bool isreindex, - bool parallel, - bool progress) + bool parallel) { IndexBuildResult *stats; Oid save_userid; @@ -3074,7 +3071,6 @@ index_build(Relation heapRelation, RestrictSearchPath(); /* Set up initial progress report status */ - if (progress) { const int progress_index[] = { PROGRESS_CREATEIDX_PHASE, @@ -3639,7 +3635,6 @@ reindex_index(const ReindexStmt *stmt, Oid indexId, IndexInfo *indexInfo; volatile bool skipped_constraint = false; PGRUsage ru0; - bool progress = ((params->options & REINDEXOPT_REPORT_PROGRESS) != 0); bool set_tablespace = false; pg_rusage_init(&ru0); @@ -3674,7 +3669,6 @@ reindex_index(const ReindexStmt *stmt, Oid indexId, save_nestlevel = NewGUCNestLevel(); RestrictSearchPath(); - if (progress) { const int progress_cols[] = { PROGRESS_CREATEIDX_COMMAND, @@ -3713,9 +3707,8 @@ reindex_index(const ReindexStmt *stmt, Oid indexId, return; } - if (progress) - pgstat_progress_update_param(PROGRESS_CREATEIDX_ACCESS_METHOD_OID, - iRel->rd_rel->relam); + pgstat_progress_update_param(PROGRESS_CREATEIDX_ACCESS_METHOD_OID, + iRel->rd_rel->relam); /* * If a statement is available, telling that this comes from a REINDEX @@ -3832,7 +3825,7 @@ reindex_index(const ReindexStmt *stmt, Oid indexId, /* Initialize the index and rebuild */ /* Note: we do not need to re-establish pkey setting */ - index_build(heapRelation, iRel, indexInfo, true, true, progress); + index_build(heapRelation, iRel, indexInfo, true, true); /* Re-allow use of target index */ ResetReindexProcessing(); @@ -3926,8 +3919,7 @@ reindex_index(const ReindexStmt *stmt, Oid indexId, index_close(iRel, NoLock); table_close(heapRelation, NoLock); - if (progress) - pgstat_progress_end_command(); + pgstat_progress_end_command(); } /* @@ -4099,9 +4091,15 @@ reindex_relation(const ReindexStmt *stmt, Oid relid, int flags, /* Index should no longer be in the pending list */ Assert(!ReindexIsProcessingIndex(indexOid)); - /* Set index rebuild count */ - pgstat_progress_update_param(PROGRESS_REPACK_INDEX_REBUILD_COUNT, - i); + /* + * Set index rebuild count - only interesting for REPACK. + * + * XXX Shouldn't we add an argument indicating that the function is + * being called from REPACK (or its subroutine)? + */ + if (MyBEEntry->st_progress_command == PROGRESS_COMMAND_REPACK) + pgstat_progress_update_param(PROGRESS_REPACK_INDEX_REBUILD_COUNT, + i); i++; } diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c index 4aa52a4bd25..672dcba84f4 100644 --- a/src/backend/catalog/toasting.c +++ b/src/backend/catalog/toasting.c @@ -30,6 +30,7 @@ #include "catalog/toasting.h" #include "miscadmin.h" #include "nodes/makefuncs.h" +#include "utils/backend_progress.h" #include "utils/fmgroids.h" #include "utils/rel.h" #include "utils/syscache.h" @@ -325,6 +326,13 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid, coloptions[0] = 0; coloptions[1] = 0; + /* + * XXX Progress reporting makes little sense for empty table, but + * index_create() does report the progress and it's not worth disabling it + * in this specific case. + */ + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, + RelationGetRelid(toast_rel)); index_create(toast_rel, toast_idxname, toastIndexOid, InvalidOid, InvalidOid, InvalidOid, indexInfo, @@ -333,6 +341,7 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid, rel->rd_rel->reltablespace, collationIds, opclassIds, NULL, coloptions, NULL, (Datum) 0, INDEX_CREATE_IS_PRIMARY, 0, true, true, NULL); + pgstat_progress_end_command(); table_close(toast_rel, NoLock); diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 9ab74c8df0a..c2d4b9ff087 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -594,6 +594,7 @@ DefineIndex(ParseState *pstate, Oid root_save_userid; int root_save_sec_context; int root_save_nestlevel; + bool progress_started = false; root_save_nestlevel = NewGUCNestLevel(); @@ -622,16 +623,21 @@ DefineIndex(ParseState *pstate, concurrent = false; /* - * Start progress report. If we're building a partition, this was already - * done. + * Start progress report if not done yet. + * + * We could check parentIndexId and only start the reporting when + * processing the parent index (i.e. parentIndexId is invalid), however + * that wouldn't work when adding a partition to an existing table: + * parentIndexId is always valid in that cases. */ - if (!OidIsValid(parentIndexId)) + if (MyBEEntry->st_progress_command == PROGRESS_COMMAND_INVALID) { pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, tableId); pgstat_progress_update_param(PROGRESS_CREATEIDX_COMMAND, concurrent ? PROGRESS_CREATEIDX_COMMAND_CREATE_CONCURRENTLY : PROGRESS_CREATEIDX_COMMAND_CREATE); + progress_started = true; } /* @@ -1277,8 +1283,7 @@ DefineIndex(ParseState *pstate, table_close(rel, NoLock); - /* If this is the top-level index, we're done */ - if (!OidIsValid(parentIndexId)) + if (progress_started) pgstat_progress_end_command(); return address; @@ -1585,21 +1590,18 @@ DefineIndex(ParseState *pstate, } } - /* - * Indexes on partitioned tables are not themselves built, so we're - * done here. - */ AtEOXact_GUC(false, root_save_nestlevel); SetUserIdAndSecContext(root_save_userid, root_save_sec_context); table_close(rel, NoLock); - if (!OidIsValid(parentIndexId)) - pgstat_progress_end_command(); - else + if (OidIsValid(parentIndexId)) { /* Update progress for an intermediate partitioned index itself */ pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1); } + if (progress_started) + pgstat_progress_end_command(); + return address; } @@ -1612,14 +1614,14 @@ DefineIndex(ParseState *pstate, table_close(rel, NoLock); /* - * If this is the top-level index, the command is done overall; - * otherwise, increment progress to report one child index is done. + * Increment progress to report one child index is done. */ - if (!OidIsValid(parentIndexId)) - pgstat_progress_end_command(); - else + if (OidIsValid(parentIndexId)) pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1); + if (progress_started) + pgstat_progress_end_command(); + return address; } @@ -1833,7 +1835,8 @@ DefineIndex(ParseState *pstate, */ UnlockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock); - pgstat_progress_end_command(); + if (progress_started) + pgstat_progress_end_command(); return address; } @@ -2987,7 +2990,6 @@ ReindexIndex(const ReindexStmt *stmt, const ReindexParams *params, bool isTopLev { ReindexParams newparams = *params; - newparams.options |= REINDEXOPT_REPORT_PROGRESS; reindex_index(stmt, indOid, false, persistence, &newparams); } } @@ -3110,7 +3112,6 @@ ReindexTable(const ReindexStmt *stmt, const ReindexParams *params, bool isTopLev { ReindexParams newparams = *params; - newparams.options |= REINDEXOPT_REPORT_PROGRESS; result = reindex_relation(stmt, heapOid, REINDEX_REL_PROCESS_TOAST | REINDEX_REL_CHECK_CONSTRAINTS, @@ -3535,8 +3536,7 @@ ReindexMultipleInternal(const ReindexStmt *stmt, const List *relids, const Reind { ReindexParams newparams = *params; - newparams.options |= - REINDEXOPT_REPORT_PROGRESS | REINDEXOPT_MISSING_OK; + newparams.options |= REINDEXOPT_MISSING_OK; reindex_index(stmt, relid, false, relpersistence, &newparams); PopActiveSnapshot(); /* reindex_index() does the verbose output */ @@ -3546,8 +3546,7 @@ ReindexMultipleInternal(const ReindexStmt *stmt, const List *relids, const Reind bool result; ReindexParams newparams = *params; - newparams.options |= - REINDEXOPT_REPORT_PROGRESS | REINDEXOPT_MISSING_OK; + newparams.options |= REINDEXOPT_MISSING_OK; result = reindex_relation(stmt, relid, REINDEX_REL_PROCESS_TOAST | REINDEX_REL_CHECK_CONSTRAINTS, @@ -3967,7 +3966,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein elog(ERROR, "cannot reindex a temporary table concurrently"); pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, idx->tableId); - progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY; progress_vals[1] = 0; /* initializing */ progress_vals[2] = idx->indexId; @@ -3991,11 +3989,11 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein /* Create new index definition based on given index */ newIndexId = index_create_copy(heapRel, INDEX_CREATE_CONCURRENT | - INDEX_CREATE_SKIP_BUILD | - INDEX_CREATE_SUPPRESS_PROGRESS, + INDEX_CREATE_SKIP_BUILD, idx->indexId, tablespaceid, concurrentName); + pgstat_progress_end_command(); /* * Now open the relation of the new index, a session-level lock is @@ -4114,9 +4112,11 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein * DefineIndex() for more details. */ + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, relationOid); pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_1); WaitForLockersMultiple(lockTags, ShareLock, true); + pgstat_progress_end_command(); CommitTransactionCommand(); foreach(lc, newIndexIds) @@ -4153,6 +4153,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein /* Perform concurrent build of new index */ index_concurrently_build(newidx->tableId, newidx->indexId); + pgstat_progress_end_command(); PopActiveSnapshot(); CommitTransactionCommand(); @@ -4173,9 +4174,11 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein * for more details. */ + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, relationOid); pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_2); WaitForLockersMultiple(lockTags, ShareLock, true); + pgstat_progress_end_command(); CommitTransactionCommand(); foreach(lc, newIndexIds) @@ -4246,6 +4249,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_3); WaitForOlderSnapshots(limitXmin, true); + pgstat_progress_end_command(); CommitTransactionCommand(); } @@ -4341,6 +4345,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein */ INJECTION_POINT("reindex-relation-concurrently-before-set-dead", NULL); + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, relationOid); pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_4); WaitForLockersMultiple(lockTags, AccessExclusiveLock, true); @@ -4386,6 +4391,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_5); WaitForLockersMultiple(lockTags, AccessExclusiveLock, true); + pgstat_progress_end_command(); PushActiveSnapshot(GetTransactionSnapshot()); @@ -4461,8 +4467,6 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein MemoryContextDelete(private_context); - pgstat_progress_end_command(); - return true; } diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index 4d177c868bb..51078d10202 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -1920,11 +1920,19 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap, { ObjectAddress object; Oid mapped_tables[4]; + bool is_repack; int i; + /* + * XXX Shouldn't we add an argument indicating that the function is being + * called from REPACK? + */ + is_repack = MyBEEntry->st_progress_command == PROGRESS_COMMAND_REPACK; + /* Report that we are now swapping relation files */ - pgstat_progress_update_param(PROGRESS_REPACK_PHASE, - PROGRESS_REPACK_PHASE_SWAP_REL_FILES); + if (is_repack) + pgstat_progress_update_param(PROGRESS_REPACK_PHASE, + PROGRESS_REPACK_PHASE_SWAP_REL_FILES); /* Zero out possible results from swapped_relation_files */ memset(mapped_tables, 0, sizeof(mapped_tables)); @@ -1981,15 +1989,17 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap, reindex_flags |= REINDEX_REL_FORCE_INDEXES_PERMANENT; /* Report that we are now reindexing relations */ - pgstat_progress_update_param(PROGRESS_REPACK_PHASE, - PROGRESS_REPACK_PHASE_REBUILD_INDEX); + if (is_repack) + pgstat_progress_update_param(PROGRESS_REPACK_PHASE, + PROGRESS_REPACK_PHASE_REBUILD_INDEX); reindex_relation(NULL, OIDOldHeap, reindex_flags, &reindex_params); } /* Report that we are now doing clean up */ - pgstat_progress_update_param(PROGRESS_REPACK_PHASE, - PROGRESS_REPACK_PHASE_FINAL_CLEANUP); + if (is_repack) + pgstat_progress_update_param(PROGRESS_REPACK_PHASE, + PROGRESS_REPACK_PHASE_FINAL_CLEANUP); /* * If the relation being rebuilt is pg_class, swap_relation_files() @@ -3348,9 +3358,20 @@ build_new_indexes(Relation NewHeap, Relation OldHeap, List *OldIndexes) "repacknew", get_rel_namespace(ind->rd_index->indrelid), false); - newindex = index_create_copy(NewHeap, INDEX_CREATE_SUPPRESS_PROGRESS, + + /* + * We build the index on the new heap, but after the swap phase it'll + * become an index on the old heap. It makes more sense to report the + * progress this way. (The reporting API expects that both command and + * subcommand deal with the same target.) + */ + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, + RelationGetRelid(OldHeap)); + newindex = index_create_copy(NewHeap, 0, oldindex, ind->rd_rel->reltablespace, newName); + pgstat_progress_end_command(); + copy_index_constraints(ind, newindex, RelationGetRelid(NewHeap)); result = lappend_oid(result, newindex); diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c index 6d2049105ab..ce7ca533627 100644 --- a/src/backend/utils/activity/backend_progress.c +++ b/src/backend/utils/activity/backend_progress.c @@ -22,6 +22,10 @@ * * Set st_progress_command (and st_progress_command_target) in own backend * entry. Also, zero-initialize st_progress_param array. + * + * If command has already been started, start a sub-command. Only parameters + * of the sub-command are updated until pgstat_progress_end_command() is + * called. (Target relation must be the same for both commands.) *----------- */ void @@ -33,9 +37,30 @@ pgstat_progress_start_command(ProgressCommandType cmdtype, Oid relid) return; PGSTAT_BEGIN_WRITE_ACTIVITY(beentry); - beentry->st_progress_command = cmdtype; - beentry->st_progress_command_target = relid; - MemSet(&beentry->st_progress_param, 0, sizeof(beentry->st_progress_param)); + /* Sub-command should not be started w/o parent command. */ + if (beentry->st_progress_command == PROGRESS_COMMAND_INVALID) + { + Assert(beentry->st_progress_command2 == PROGRESS_COMMAND_INVALID); + + beentry->st_progress_command = cmdtype; + beentry->st_progress_command_target = relid; + MemSet(&beentry->st_progress_param, 0, + sizeof(beentry->st_progress_param)); + } + else if (beentry->st_progress_command2 == PROGRESS_COMMAND_INVALID) + { + Assert(beentry->st_progress_command != PROGRESS_COMMAND_INVALID); + + beentry->st_progress_command2 = cmdtype; + beentry->st_progress_command_target2 = relid; + MemSet(&beentry->st_progress_param2, 0, + sizeof(beentry->st_progress_param2)); + } + else + { + /* Only one level of nesting is supported. */ + Assert(false); + } PGSTAT_END_WRITE_ACTIVITY(beentry); } @@ -49,14 +74,21 @@ void pgstat_progress_update_param(int index, int64 val) { volatile PgBackendStatus *beentry = MyBEEntry; + volatile int64 *params; Assert(index >= 0 && index < PGSTAT_NUM_PROGRESS_PARAM); if (!beentry || !pgstat_track_activities) return; + Assert(beentry->st_progress_command != PROGRESS_COMMAND_INVALID || + beentry->st_progress_command2 != PROGRESS_COMMAND_INVALID); + + params = (beentry->st_progress_command2 == PROGRESS_COMMAND_INVALID) ? + beentry->st_progress_param : beentry->st_progress_param2; + PGSTAT_BEGIN_WRITE_ACTIVITY(beentry); - beentry->st_progress_param[index] = val; + params[index] = val; PGSTAT_END_WRITE_ACTIVITY(beentry); } @@ -70,14 +102,21 @@ void pgstat_progress_incr_param(int index, int64 incr) { volatile PgBackendStatus *beentry = MyBEEntry; + volatile int64 *params; Assert(index >= 0 && index < PGSTAT_NUM_PROGRESS_PARAM); if (!beentry || !pgstat_track_activities) return; + Assert(beentry->st_progress_command != PROGRESS_COMMAND_INVALID || + beentry->st_progress_command2 != PROGRESS_COMMAND_INVALID); + + params = (beentry->st_progress_command2 == PROGRESS_COMMAND_INVALID) ? + beentry->st_progress_param : beentry->st_progress_param2; + PGSTAT_BEGIN_WRITE_ACTIVITY(beentry); - beentry->st_progress_param[index] += incr; + params[index] += incr; PGSTAT_END_WRITE_ACTIVITY(beentry); } @@ -122,17 +161,24 @@ pgstat_progress_update_multi_param(int nparam, const int *index, { volatile PgBackendStatus *beentry = MyBEEntry; int i; + volatile int64 *params; if (!beentry || !pgstat_track_activities || nparam == 0) return; + Assert(beentry->st_progress_command != PROGRESS_COMMAND_INVALID || + beentry->st_progress_command2 != PROGRESS_COMMAND_INVALID); + + params = (beentry->st_progress_command2 == PROGRESS_COMMAND_INVALID) ? + beentry->st_progress_param : beentry->st_progress_param2; + PGSTAT_BEGIN_WRITE_ACTIVITY(beentry); for (i = 0; i < nparam; ++i) { Assert(index[i] >= 0 && index[i] < PGSTAT_NUM_PROGRESS_PARAM); - beentry->st_progress_param[index[i]] = val[i]; + params[index[i]] = val[i]; } PGSTAT_END_WRITE_ACTIVITY(beentry); @@ -142,7 +188,7 @@ pgstat_progress_update_multi_param(int nparam, const int *index, * pgstat_progress_end_command() - * * Reset st_progress_command (and st_progress_command_target) in own backend - * entry. This signals the end of the command. + * entry. This signals the end of the command (or a sub-command). *----------- */ void @@ -153,11 +199,20 @@ pgstat_progress_end_command(void) if (!beentry || !pgstat_track_activities) return; - if (beentry->st_progress_command == PROGRESS_COMMAND_INVALID) - return; - PGSTAT_BEGIN_WRITE_ACTIVITY(beentry); - beentry->st_progress_command = PROGRESS_COMMAND_INVALID; - beentry->st_progress_command_target = InvalidOid; + + if (beentry->st_progress_command2 != PROGRESS_COMMAND_INVALID) + { + Assert(beentry->st_progress_command != PROGRESS_COMMAND_INVALID); + + beentry->st_progress_command2 = PROGRESS_COMMAND_INVALID; + beentry->st_progress_command_target2 = InvalidOid; + } + else + { + beentry->st_progress_command = PROGRESS_COMMAND_INVALID; + beentry->st_progress_command_target = InvalidOid; + } + PGSTAT_END_WRITE_ACTIVITY(beentry); } diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c index d685fc5cd87..15675992415 100644 --- a/src/backend/utils/activity/backend_status.c +++ b/src/backend/utils/activity/backend_status.c @@ -284,6 +284,7 @@ pgstat_bestart_initial(void) lbeentry.st_state = STATE_STARTING; lbeentry.st_progress_command = PROGRESS_COMMAND_INVALID; + lbeentry.st_progress_command2 = PROGRESS_COMMAND_INVALID; lbeentry.st_progress_command_target = InvalidOid; lbeentry.st_query_id = INT64CONST(0); lbeentry.st_plan_id = INT64CONST(0); diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index 6f9c9c72de5..d0513d6931b 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -314,6 +314,7 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS) Datum values[PG_STAT_GET_PROGRESS_COLS] = {0}; bool nulls[PG_STAT_GET_PROGRESS_COLS] = {0}; int i; + volatile int64 *params; local_beentry = pgstat_get_local_beentry_by_index(curr_backend); beentry = &local_beentry->backendStatus; @@ -322,7 +323,11 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS) * Report values for only those backends which are running the given * command. */ - if (beentry->st_progress_command != cmdtype) + if (beentry->st_progress_command == cmdtype) + params = beentry->st_progress_param; + else if (beentry->st_progress_command2 == cmdtype) + params = beentry->st_progress_param2; + else continue; /* Value available to all callers */ @@ -332,9 +337,13 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS) /* show rest of the values including relid only to role members */ if (HAS_PGSTAT_PERMISSIONS(beentry->st_userid)) { - values[2] = ObjectIdGetDatum(beentry->st_progress_command_target); + if (beentry->st_progress_command == cmdtype) + values[2] = ObjectIdGetDatum(beentry->st_progress_command_target); + else if (beentry->st_progress_command2 == cmdtype) + values[2] = ObjectIdGetDatum(beentry->st_progress_command_target2); + for (i = 0; i < PGSTAT_NUM_PROGRESS_PARAM; i++) - values[i + 3] = Int64GetDatum(beentry->st_progress_param[i]); + values[i + 3] = Int64GetDatum(params[i]); } else { diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h index 9aee8226347..d4c77f3df3f 100644 --- a/src/include/catalog/index.h +++ b/src/include/catalog/index.h @@ -45,9 +45,8 @@ typedef struct ReindexParams /* flag bits for ReindexParams->flags */ #define REINDEXOPT_VERBOSE 0x01 /* print progress info */ -#define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */ -#define REINDEXOPT_MISSING_OK 0x04 /* skip missing relations */ -#define REINDEXOPT_CONCURRENTLY 0x08 /* concurrent mode */ +#define REINDEXOPT_MISSING_OK 0x02 /* skip missing relations */ +#define REINDEXOPT_CONCURRENTLY 0x04 /* concurrent mode */ /* state info for validate_index bulkdelete callback */ typedef struct ValidateIndexState @@ -71,7 +70,6 @@ extern void index_check_primary_key(Relation heapRel, #define INDEX_CREATE_IF_NOT_EXISTS (1 << 4) #define INDEX_CREATE_PARTITIONED (1 << 5) #define INDEX_CREATE_INVALID (1 << 6) -#define INDEX_CREATE_SUPPRESS_PROGRESS (1 << 7) extern Oid index_create(Relation heapRelation, const char *indexRelationName, @@ -149,8 +147,7 @@ extern void index_build(Relation heapRelation, Relation indexRelation, IndexInfo *indexInfo, bool isreindex, - bool parallel, - bool progress); + bool parallel); extern void validate_index(Oid heapId, Oid indexId, Snapshot snapshot); diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h index a334e096e4a..8b39c59df7b 100644 --- a/src/include/utils/backend_status.h +++ b/src/include/utils/backend_status.h @@ -169,6 +169,15 @@ typedef struct PgBackendStatus Oid st_progress_command_target; int64 st_progress_param[PGSTAT_NUM_PROGRESS_PARAM]; + /* + * Some commands have a sub-command, e.g. REPACK (re)builds indexes. The + * target can be different, e.g. when the sub-command builds an index on + * TOAST relation. + */ + ProgressCommandType st_progress_command2; + Oid st_progress_command_target2; + int64 st_progress_param2[PGSTAT_NUM_PROGRESS_PARAM]; + /* query identifier, optionally computed using post_parse_analyze_hook */ int64 st_query_id; @@ -176,6 +185,16 @@ typedef struct PgBackendStatus int64 st_plan_id; } PgBackendStatus; +/* + * Check the depth of progress tracking. + * + * 0 - tracking disabled + * 1 - enabled for the "main" command + * 2 - enabled for both main command and its sub-command. + */ +#define PGSTAT_PROGRESS_STATE(state) \ + ((state)->st_progress_command == PROGRESS_COMMAND_INVALID ? 0 :\ + (state)->st_progress_command2 == PROGRESS_COMMAND_INVALID ? 1 : 2) /* * Macros to load and store st_changecount with appropriate memory barriers. -- 2.52.0