Fix assertion failure when updating stats_fetch_consistency in a transaction

An update of the GUC stats_fetch_consistency in a transaction would be
able to trigger an assertion when doing cache->snapshot.  In this case,
when retrieving a pgstat entry after the switch, a new snapshot would be
rebuilt, confusing pgstat_build_snapshot() because a snapshot is already
cached with an unexpected mode ("cache").

In order to fix this problem, this commit adds a flag to force a
snapshot clear each time this GUC is changed.  Some tests are added to
check, while on it.

Some optimizations in avoiding the snapshot clear should be possible
depending on what is cached and the current GUC value, I guess, but this
solution is simple, and ensures that the state of the cache is updated
each time a new pgstat entry is fetched, hence being consistent with the
level wanted by the client that has set the GUC.

Note that cache->none and snapshot->none would not cause issues, as
fetching a pgstat entry would be retrieved from shared memory on the
second attempt, however a snapshot would still be cached.  Similarly,
none->snapshot and none->cache would build a new snapshot on the second
fetch attempt.  Finally, snapshot->cache would cache a new snapshot on
the second attempt.

Reported-by: Alexander Lakhin
Author: Kyotaro Horiguchi
Discussion: https://postgr.es/m/17804-2a118cd046f2d0e5@postgresql.org
backpatch-through: 15

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b56f073a918d4210ac722a533ea8d83f17b7c23c..909a3f28c790161462b407e91a686d1178b24c4a 100644 (file)
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8219,8 +8219,9 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
         called. When set to <literal>snapshot</literal>, the first statistics
         access caches all statistics accessible in the current database, until
         the end of the transaction unless
-        <function>pg_stat_clear_snapshot()</function> is called. The default
-        is <literal>cache</literal>.
+        <function>pg_stat_clear_snapshot()</function> is called. Changing this
+        parameter in a transaction discards the statistics snapshot.
+        The default is <literal>cache</literal>.
        </para>
        <note>
         <para>

diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index b125802b2155f6d90e40234eb665c1801fa9ad21..f6edfc76ac4be9d8246dcc0c83d0ee282ed69d25 100644 (file)
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -103,7 +103,7 @@
 #include "storage/lwlock.h"
 #include "storage/pg_shmem.h"
 #include "storage/shmem.h"
-#include "utils/guc.h"
+#include "utils/guc_hooks.h"
 #include "utils/memutils.h"
 #include "utils/pgstat_internal.h"
 #include "utils/timestamp.h"
@@ -227,6 +227,13 @@ static dlist_head pgStatPending = DLIST_STATIC_INIT(pgStatPending);
  */
 static bool pgStatForceNextFlush = false;
 
+/*
+ * Force-clear existing snapshot before next use when stats_fetch_consistency
+ * is changed.
+ */
+static bool force_stats_snapshot_clear = false;
+
+
 /*
  * For assertions that check pgstat is not used before initialization / after
  * shutdown.
@@ -760,7 +767,8 @@ pgstat_reset_of_kind(PgStat_Kind kind)
  * request will cause new snapshots to be read.
  *
  * This is also invoked during transaction commit or abort to discard
- * the no-longer-wanted snapshot.
+ * the no-longer-wanted snapshot.  Updates of stats_fetch_consistency can
+ * cause this routine to be called.
  */
 void
 pgstat_clear_snapshot(void)
@@ -787,6 +795,9 @@ pgstat_clear_snapshot(void)
         * forward the reset request.
         */
        pgstat_clear_backend_activity_snapshot();
+
+       /* Reset this flag, as it may be possible that a cleanup was forced. */
+       force_stats_snapshot_clear = false;
 }
 
 void *
@@ -885,6 +896,9 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
 TimestampTz
 pgstat_get_stat_snapshot_timestamp(bool *have_snapshot)
 {
+       if (force_stats_snapshot_clear)
+               pgstat_clear_snapshot();
+
        if (pgStatLocal.snapshot.mode == PGSTAT_FETCH_CONSISTENCY_SNAPSHOT)
        {
                *have_snapshot = true;
@@ -929,6 +943,9 @@ pgstat_snapshot_fixed(PgStat_Kind kind)
 static void
 pgstat_prep_snapshot(void)
 {
+       if (force_stats_snapshot_clear)
+               pgstat_clear_snapshot();
+
        if (pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_NONE ||
                pgStatLocal.snapshot.stats != NULL)
                return;
@@ -1671,3 +1688,18 @@ pgstat_reset_after_failure(void)
        /* and drop variable-numbered ones */
        pgstat_drop_all_entries();
 }
+
+/*
+ * GUC assign_hook for stats_fetch_consistency.
+ */
+void
+assign_stats_fetch_consistency(int newval, void *extra)
+{
+       /*
+        * Changing this value in a transaction may cause snapshot state
+        * inconsistencies, so force a clear of the current snapshot on the next
+        * snapshot build attempt.
+        */
+       if (pgstat_fetch_consistency != newval)
+               force_stats_snapshot_clear = true;
+}

diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 2f42cebaf62df19b8e637fbc5fc6cc510cfb9c16..5f90aecd47842dbf7873d8956a61607c04e9af5e 100644 (file)
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4821,7 +4821,7 @@ struct config_enum ConfigureNamesEnum[] =
                },
                &pgstat_fetch_consistency,
                PGSTAT_FETCH_CONSISTENCY_CACHE, stats_fetch_consistency,
-               NULL, NULL, NULL
+               NULL, assign_stats_fetch_consistency, NULL
        },
 
        {

diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index a82a85c9401169cbea7151756c24dcca1081aa37..2ecb9fc0866b3ece4229f49d122960f482820758 100644 (file)
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -121,6 +121,7 @@ extern void assign_search_path(const char *newval, void *extra);
 extern bool check_session_authorization(char **newval, void **extra, GucSource source);
 extern void assign_session_authorization(const char *newval, void *extra);
 extern void assign_session_replication_role(int newval, void *extra);
+extern void assign_stats_fetch_consistency(int newval, void *extra);
 extern bool check_ssl(bool *newval, void **extra, GucSource source);
 extern bool check_stage_log_stats(bool *newval, void **extra, GucSource source);
 extern bool check_synchronous_standby_names(char **newval, void **extra,

diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out
index 813d6d39eabd281e84e30b865fe1c76ecfa3894d..4d965aadb161cc1ede8767c9e8fc645c523f33f8 100644 (file)
--- a/src/test/regress/expected/stats.out
+++ b/src/test/regress/expected/stats.out
@@ -985,6 +985,65 @@ SELECT pg_stat_get_snapshot_timestamp();
 
 COMMIT;
 ----
+-- Changing stats_fetch_consistency in a transaction.
+----
+BEGIN;
+-- Stats filled under the cache mode
+SET LOCAL stats_fetch_consistency = cache;
+SELECT pg_stat_get_function_calls(0);
+ pg_stat_get_function_calls 
+----------------------------
+                           
+(1 row)
+
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+ snapshot_ok 
+-------------
+ f
+(1 row)
+
+-- Success in accessing pre-existing snapshot data.
+SET LOCAL stats_fetch_consistency = snapshot;
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+ snapshot_ok 
+-------------
+ f
+(1 row)
+
+SELECT pg_stat_get_function_calls(0);
+ pg_stat_get_function_calls 
+----------------------------
+                           
+(1 row)
+
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+ snapshot_ok 
+-------------
+ t
+(1 row)
+
+-- Snapshot cleared.
+SET LOCAL stats_fetch_consistency = none;
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+ snapshot_ok 
+-------------
+ f
+(1 row)
+
+SELECT pg_stat_get_function_calls(0);
+ pg_stat_get_function_calls 
+----------------------------
+                           
+(1 row)
+
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+ snapshot_ok 
+-------------
+ f
+(1 row)
+
+ROLLBACK;
+----
 -- pg_stat_have_stats behavior
 ----
 -- fixed-numbered stats exist

diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql
index 99a28bb79cd2cd79979fe211186b7255f04a1091..4e2c3067f93bc5acd19893d99a2dd273eafda168 100644 (file)
--- a/src/test/regress/sql/stats.sql
+++ b/src/test/regress/sql/stats.sql
@@ -476,6 +476,26 @@ SELECT pg_stat_clear_snapshot();
 SELECT pg_stat_get_snapshot_timestamp();
 COMMIT;
 
+----
+-- Changing stats_fetch_consistency in a transaction.
+----
+BEGIN;
+-- Stats filled under the cache mode
+SET LOCAL stats_fetch_consistency = cache;
+SELECT pg_stat_get_function_calls(0);
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+-- Success in accessing pre-existing snapshot data.
+SET LOCAL stats_fetch_consistency = snapshot;
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+SELECT pg_stat_get_function_calls(0);
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+-- Snapshot cleared.
+SET LOCAL stats_fetch_consistency = none;
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+SELECT pg_stat_get_function_calls(0);
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+ROLLBACK;
+
 ----
 -- pg_stat_have_stats behavior
 ----