From af11d456981908caee447b8144d4fea6ffba2c15 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 1 May 2023 14:16:41 +0900 Subject: [PATCH v3] Fix assertion failure on updates of stats_fetch_consistency Blah, blah.. Reported-by: Alexander Lakhin Author: Kyotaro Horiguchi Discussion: https://postgr.es/m/17804-2a118cd046f2d0e5@postgresql.org backpatch-through: 15 --- src/include/utils/guc_hooks.h | 1 + src/backend/utils/activity/pgstat.c | 36 +++++++++++++++++- src/backend/utils/misc/guc_tables.c | 2 +- src/test/regress/expected/stats.out | 59 +++++++++++++++++++++++++++++ src/test/regress/sql/stats.sql | 20 ++++++++++ doc/src/sgml/config.sgml | 5 ++- 6 files changed, 118 insertions(+), 5 deletions(-) diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h index a82a85c940..2ecb9fc086 100644 --- 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/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index b125802b21..d0b3ae929c 100644 --- 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 2f42cebaf6..5f90aecd47 100644 --- 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/test/regress/expected/stats.out b/src/test/regress/expected/stats.out index 813d6d39ea..4d965aadb1 100644 --- 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 99a28bb79c..4e2c3067f9 100644 --- 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 ---- diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index b56f073a91..909a3f28c7 100644 --- 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 snapshot, the first statistics access caches all statistics accessible in the current database, until the end of the transaction unless - pg_stat_clear_snapshot() is called. The default - is cache. + pg_stat_clear_snapshot() is called. Changing this + parameter in a transaction discards the statistics snapshot. + The default is cache. -- 2.40.1