From 84388375d4808d44877fc9e26eacf9219158be2b Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Sat, 21 Jan 2023 21:41:04 -0600 Subject: [PATCH 3/3] f! also assert that progress values don't go backwards and the total is constant pgstat_progress_update_multi_param() is being abused as a way to update a value which might otherwise violate the assertion. See also: https://www.postgresql.org/message-id/CA%2BTgmoYSvEP3weQaCPGf6%2BDXLy2__JbJUYtoXyWP%3DqHcyGbihA%40mail.gmail.com --- src/backend/access/heap/vacuumlazy.c | 37 +++++++++ src/backend/storage/lmgr/lmgr.c | 24 +++--- src/backend/utils/activity/backend_progress.c | 81 +++++++++++++++++++ 3 files changed, 130 insertions(+), 12 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 8f14cf85f38..61cfbf6a17a 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -1042,6 +1042,13 @@ lazy_scan_heap(LVRelState *vacrel) /* Forget the LP_DEAD items that we just vacuumed */ dead_items->num_items = 0; + { + const int progress_inds[] = {PROGRESS_VACUUM_NUM_DEAD_TUPLES}; + const int64 progress_vals[] = {0}; + + pgstat_progress_update_multi_param(1, progress_inds, progress_vals); + } + /* * Periodically perform FSM vacuuming to make newly-freed @@ -2199,6 +2206,13 @@ lazy_vacuum(LVRelState *vacrel) { Assert(!vacrel->do_index_cleanup); vacrel->dead_items->num_items = 0; + { + const int progress_inds[] = {PROGRESS_VACUUM_NUM_DEAD_TUPLES}; + const int64 progress_vals[] = {0}; + + pgstat_progress_update_multi_param(1, progress_inds, progress_vals); + } + return; } @@ -2301,6 +2315,13 @@ lazy_vacuum(LVRelState *vacrel) * vacuum) */ vacrel->dead_items->num_items = 0; + + { + const int progress_inds[] = {PROGRESS_VACUUM_NUM_DEAD_TUPLES}; + const int64 progress_vals[] = {0}; + + pgstat_progress_update_multi_param(1, progress_inds, progress_vals); + } } /* @@ -2414,12 +2435,23 @@ lazy_vacuum_heap_rel(LVRelState *vacrel) BlockNumber vacuumed_pages = 0; Buffer vmbuffer = InvalidBuffer; LVSavedErrInfo saved_err_info; +#if 0 + const int progress_inds[] = { + PROGRESS_VACUUM_PHASE, + PROGRESS_VACUUM_NUM_DEAD_TUPLES, + }; + const int64 progress_vals[] = { + PROGRESS_VACUUM_PHASE_VACUUM_HEAP, + 0, + }; +#endif Assert(vacrel->do_index_vacuuming); Assert(vacrel->do_index_cleanup); Assert(vacrel->num_index_scans > 0); /* Report that we are now vacuuming the heap */ + //pgstat_progress_update_multi_param(2, progress_inds, progress_vals); pgstat_progress_update_param(PROGRESS_VACUUM_PHASE, PROGRESS_VACUUM_PHASE_VACUUM_HEAP); @@ -3190,7 +3222,12 @@ dead_items_alloc(LVRelState *vacrel, int nworkers) dead_items = (VacDeadItems *) palloc(vac_max_items_to_alloc_size(max_items)); dead_items->max_items = max_items; dead_items->num_items = 0; + { + const int progress_inds[] = {PROGRESS_VACUUM_NUM_DEAD_TUPLES}; + const int64 progress_vals[] = {0}; + pgstat_progress_update_multi_param(1, progress_inds, progress_vals); + } vacrel->dead_items = dead_items; } diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c index ee9b89a6726..a3acdce5ff5 100644 --- a/src/backend/storage/lmgr/lmgr.c +++ b/src/backend/storage/lmgr/lmgr.c @@ -912,6 +912,13 @@ WaitForLockersMultiple(List *locktags, LOCKMODE lockmode, bool progress) int total = 0; int done = 0; + const int progress_index[] = { + PROGRESS_WAITFOR_TOTAL, + PROGRESS_WAITFOR_DONE, + PROGRESS_WAITFOR_CURRENT_PID + }; + const int64 progress_values[] = {0, 0, 0}; + /* Done if no locks to wait for */ if (locktags == NIL) return; @@ -930,7 +937,10 @@ WaitForLockersMultiple(List *locktags, LOCKMODE lockmode, bool progress) } if (progress) + { + pgstat_progress_update_multi_param(3, progress_index, progress_values); pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, total); + } /* * Note: GetLockConflicts() never reports our own xid, hence we need not @@ -960,19 +970,9 @@ WaitForLockersMultiple(List *locktags, LOCKMODE lockmode, bool progress) pgstat_progress_update_param(PROGRESS_WAITFOR_DONE, ++done); } } + if (progress) - { - const int index[] = { - PROGRESS_WAITFOR_TOTAL, - PROGRESS_WAITFOR_DONE, - PROGRESS_WAITFOR_CURRENT_PID - }; - const int64 values[] = { - 0, 0, 0 - }; - - pgstat_progress_update_multi_param(3, index, values); - } + pgstat_progress_update_multi_param(3, progress_index, progress_values); list_free_deep(holders); } diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c index 63f9482b175..41ad6884521 100644 --- a/src/backend/utils/activity/backend_progress.c +++ b/src/backend/utils/activity/backend_progress.c @@ -117,6 +117,78 @@ pgstat_progress_asserts(void) } } +/* + * Check that newval >= oldval, and that when total is not being set twice. + */ +static void +pgstat_progress_assert_forward_progress(int command, int index, + int64 oldval, int64 newval) +{ + switch (command) + { + case PROGRESS_COMMAND_ANALYZE: + /* + * phase goes backwards for inheritance tables, which are sampled + * twice + */ + if (index != PROGRESS_ANALYZE_CURRENT_CHILD_TABLE_RELID && + index != PROGRESS_ANALYZE_PHASE) + Assert(newval >= oldval); + if (index == PROGRESS_ANALYZE_BLOCKS_TOTAL || + index == PROGRESS_ANALYZE_EXT_STATS_TOTAL || + index == PROGRESS_ANALYZE_CHILD_TABLES_TOTAL) + Assert(oldval == 0); + break; + + case PROGRESS_COMMAND_CLUSTER: + if (index != PROGRESS_CLUSTER_INDEX_RELID) + Assert(newval >= oldval); + if (index == PROGRESS_CLUSTER_TOTAL_HEAP_BLKS) + Assert(oldval == 0); + break; + + case PROGRESS_COMMAND_CREATE_INDEX: + if (index != PROGRESS_CREATEIDX_INDEX_OID && + index != PROGRESS_CREATEIDX_SUBPHASE && + index != PROGRESS_WAITFOR_CURRENT_PID && + index != PROGRESS_CREATEIDX_ACCESS_METHOD_OID) + Assert(newval >= oldval); + + if (index == PROGRESS_CREATEIDX_TUPLES_TOTAL || + index == PROGRESS_CREATEIDX_PARTITIONS_TOTAL) + Assert(oldval == 0); + break; + + case PROGRESS_COMMAND_BASEBACKUP: + if (index == PROGRESS_BASEBACKUP_BACKUP_TOTAL && + oldval == 0 && newval == -1) + return; /* Do nothing: this is the initial "null" + * state before the size is estimated */ + Assert(newval >= oldval); + + if (index == PROGRESS_BASEBACKUP_BACKUP_TOTAL || + index == PROGRESS_BASEBACKUP_TBLSPC_TOTAL) + Assert(oldval == 0); + break; + + case PROGRESS_COMMAND_COPY: + Assert(newval >= oldval); + if (index == PROGRESS_COPY_BYTES_TOTAL) + Assert(oldval == 0); + break; + case PROGRESS_COMMAND_VACUUM: + Assert(newval >= oldval); + if (index == PROGRESS_VACUUM_TOTAL_HEAP_BLKS) + Assert(oldval == 0); + if (index == PROGRESS_VACUUM_MAX_DEAD_TUPLES) + Assert(oldval == 0); + break; + + case PROGRESS_COMMAND_INVALID: + break; + } +} + /*----------- * pgstat_progress_update_param() - * @@ -133,6 +205,15 @@ pgstat_progress_update_param(int index, int64 val) if (!beentry || !pgstat_track_activities) return; + if (index != PROGRESS_SCAN_BLOCKS_DONE) + { + /* Check that progress does not go backwards */ + int64 oldval = beentry->st_progress_param[index]; + + pgstat_progress_assert_forward_progress(beentry->st_progress_command, + index, oldval, val); + } + PGSTAT_BEGIN_WRITE_ACTIVITY(beentry); beentry->st_progress_param[index] = val; PGSTAT_END_WRITE_ACTIVITY(beentry); -- 2.34.1