agora inbox for [email protected]
help / color / mirror / Atom feedFrom: Justin Pryzby <[email protected]>
Subject: [PATCH 3/3] f! also assert that progress values don't go backwards and the total is constant
Date: Sat, 21 Jan 2023 21:41:04 -0600
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%40ma...
---
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
--4hk8RYBoBRhXB355--
view thread (6+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected]
Subject: Re: [PATCH 3/3] f! also assert that progress values don't go backwards and the total is constant
In-Reply-To: <no-message-id-109254@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox