Fix data checksum progress counter initialization

pg_stat_progress_data_checksums uses -1 as a sentinel value that is
displayed as NULL for progress counters. However, after
pgstat_progress_start_command() initialized all progress counters to
zero, data checksum progress did not reset those counters to -1.
As a result, some counters could incorrectly appear as zero instead
of NULL. For example, workers could report zero database counters,
and the disabling launcher could report zero relation and block counters.

Also, blocks_done was not reset when a worker started processing
a new relation fork. As a result, it could temporarily exceed blocks_total
or report a stale value for an empty relation fork.

Fix this by initializing the data checksum progress counters to -1
when progress reporting starts for both launcher and worker processes.
Also reset blocks_done together with blocks_total when starting each
relation fork.

Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CAHGQGwEOQyEzW2cqrHEzvwbcsAsuH8MEe7MMidFOFxECy0E1_Q@mail.gmail.com
Backpatch-through: 19

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 858788b227c4ee8a67eb3d731f0e4d94629f3d1e..d1a20d001e9c8a1c99a6de45868fe5386bd4c47c 100644 (file)
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -8095,8 +8095,8 @@ FROM pg_stat_get_backend_idset() AS backendid;
        </para>
        <para>
         The total number of databases which will be processed. Only the
-        launcher process has this value set, the worker processes have this
-        set to <literal>NULL</literal>.
+        launcher process has this value set when enabling data checksums;
+        otherwise this is set to <literal>NULL</literal>.
        </para>
       </entry>
      </row>
@@ -8108,8 +8108,8 @@ FROM pg_stat_get_backend_idset() AS backendid;
        </para>
        <para>
         The number of databases which have been processed. Only the launcher
-        process has this value set, the worker processes have this set to
-        <literal>NULL</literal>.
+        process has this value set when enabling data checksums; otherwise
+        this is set to <literal>NULL</literal>.
        </para>
       </entry>
      </row>

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 6c1c5545cb56a0b81d2b522cf72f94c26116f965..090281a03ddff98056c3e9d0a78165ea6eeb0e1c 100644 (file)
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1493,7 +1493,7 @@ CREATE VIEW pg_stat_progress_data_checksums AS
                       WHEN 4 THEN 'done'
                       END AS phase,
         CASE S.param2 WHEN -1 THEN NULL ELSE S.param2 END AS databases_total,
-        S.param3 AS databases_done,
+        CASE S.param3 WHEN -1 THEN NULL ELSE S.param3 END AS databases_done,
         CASE S.param4 WHEN -1 THEN NULL ELSE S.param4 END AS relations_total,
         CASE S.param5 WHEN -1 THEN NULL ELSE S.param5 END AS relations_done,
         CASE S.param6 WHEN -1 THEN NULL ELSE S.param6 END AS blocks_total,

diff --git a/src/backend/postmaster/datachecksum_state.c b/src/backend/postmaster/datachecksum_state.c
index 68557c16cb94f3e05fcf258277f0cd0598811920..bec110105dfc7da98e232b6aaeb11885df207f44 100644 (file)
--- a/src/backend/postmaster/datachecksum_state.c
+++ b/src/backend/postmaster/datachecksum_state.c
@@ -391,6 +391,7 @@ static void FreeDatabaseList(List *dblist);
 static DataChecksumsWorkerResult ProcessDatabase(DataChecksumsWorkerDatabase *db);
 static bool ProcessAllDatabases(void);
 static bool ProcessSingleRelationFork(Relation reln, ForkNumber forkNum, BufferAccessStrategy strategy);
+static void ResetDataChecksumsProgressCounters(void);
 static void launcher_cancel_handler(SIGNAL_ARGS);
 static void WaitForAllTransactionsToFinish(void);
 
@@ -698,7 +699,19 @@ ProcessSingleRelationFork(Relation reln, ForkNumber forkNum, BufferAccessStrateg
        snprintf(activity, sizeof(activity) - 1, "processing: %s.%s (%s, %u blocks)",
                         (relns ? relns : ""), RelationGetRelationName(reln), forkNames[forkNum], numblocks);
        pgstat_report_activity(STATE_RUNNING, activity);
-       pgstat_progress_update_param(PROGRESS_DATACHECKSUMS_BLOCKS_TOTAL, numblocks);
+       {
+               const int       index[] = {
+                       PROGRESS_DATACHECKSUMS_BLOCKS_TOTAL,
+                       PROGRESS_DATACHECKSUMS_BLOCKS_DONE
+               };
+
+               int64           vals[2];
+
+               vals[0] = numblocks;
+               vals[1] = 0;
+
+               pgstat_progress_update_multi_param(2, index, vals);
+       }
        if (relns)
                pfree(relns);
 
@@ -764,6 +777,29 @@ ProcessSingleRelationFork(Relation reln, ForkNumber forkNum, BufferAccessStrateg
        return true;
 }
 
+/*
+ * Initialize all data checksum progress counters to be displayed as NULL.
+ */
+static void
+ResetDataChecksumsProgressCounters(void)
+{
+       const int       index[] = {
+               PROGRESS_DATACHECKSUMS_DBS_TOTAL,
+               PROGRESS_DATACHECKSUMS_DBS_DONE,
+               PROGRESS_DATACHECKSUMS_RELS_TOTAL,
+               PROGRESS_DATACHECKSUMS_RELS_DONE,
+               PROGRESS_DATACHECKSUMS_BLOCKS_TOTAL,
+               PROGRESS_DATACHECKSUMS_BLOCKS_DONE,
+       };
+
+       int64           vals[lengthof(index)];
+
+       for (int i = 0; i < lengthof(index); i++)
+               vals[i] = -1;
+
+       pgstat_progress_update_multi_param(lengthof(index), index, vals);
+}
+
 /*
  * ProcessSingleRelationByOid
  *             Process a single relation based on oid.
@@ -1142,6 +1178,7 @@ again:
 
        pgstat_progress_start_command(PROGRESS_COMMAND_DATACHECKSUMS,
                                                                  InvalidOid);
+       ResetDataChecksumsProgressCounters();
 
        if (operation == ENABLE_DATACHECKSUMS)
        {
@@ -1269,23 +1306,14 @@ ProcessAllDatabases(void)
                const int       index[] = {
                        PROGRESS_DATACHECKSUMS_DBS_TOTAL,
                        PROGRESS_DATACHECKSUMS_DBS_DONE,
-                       PROGRESS_DATACHECKSUMS_RELS_TOTAL,
-                       PROGRESS_DATACHECKSUMS_RELS_DONE,
-                       PROGRESS_DATACHECKSUMS_BLOCKS_TOTAL,
-                       PROGRESS_DATACHECKSUMS_BLOCKS_DONE,
                };
 
-               int64           vals[6];
+               int64           vals[2];
 
                vals[0] = list_length(DatabaseList);
                vals[1] = 0;
-               /* translated to NULL */
-               vals[2] = -1;
-               vals[3] = -1;
-               vals[4] = -1;
-               vals[5] = -1;
 
-               pgstat_progress_update_multi_param(6, index, vals);
+               pgstat_progress_update_multi_param(2, index, vals);
        }
 
        foreach_ptr(DataChecksumsWorkerDatabase, db, DatabaseList)
@@ -1581,6 +1609,7 @@ DataChecksumsWorkerMain(Datum arg)
        /* worker will have a separate entry in pg_stat_progress_data_checksums */
        pgstat_progress_start_command(PROGRESS_COMMAND_DATACHECKSUMS,
                                                                  InvalidOid);
+       ResetDataChecksumsProgressCounters();
 
        /*
         * Get a list of all temp tables present as we start in this database. We

diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 39905c2de142c6aa18605120634768700d53a799..6a3341356da1f2d39e9f66590a432c9285330e29 100644 (file)
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -2123,7 +2123,10 @@ pg_stat_progress_data_checksums| SELECT s.pid,
             WHEN '-1'::integer THEN NULL::bigint
             ELSE s.param2
         END AS databases_total,
-    s.param3 AS databases_done,
+        CASE s.param3
+            WHEN '-1'::integer THEN NULL::bigint
+            ELSE s.param3
+        END AS databases_done,
         CASE s.param4
             WHEN '-1'::integer THEN NULL::bigint
             ELSE s.param4