From: Michael Paquier Date: Wed, 10 May 2023 02:24:40 +0000 (+0900) Subject: Fix assertion failure when updating stats_fetch_consistency in a transaction X-Git-Tag: REL_15_4~99 X-Git-Url: http://git.postgresql.org/gitweb/?a=commitdiff_plain;h=ccd21e1cfa11fa4f39d01d95cf119beae9cf4d20;p=postgresql.git 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 86c9b5ee19a..058519d31cb 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -8064,8 +8064,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. diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index d58a299d3e6..84d65a76af1 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -226,6 +226,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. @@ -746,7 +753,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) @@ -773,6 +781,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 * @@ -871,6 +882,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; @@ -915,6 +929,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; @@ -1644,3 +1661,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.c b/src/backend/utils/misc/guc.c index e9084f4506c..915f557c688 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -4948,7 +4948,7 @@ static 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/pgstat.h b/src/include/pgstat.h index dbb2074ed4a..44279ea3d70 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -432,6 +432,8 @@ extern TimestampTz pgstat_get_stat_snapshot_timestamp(bool *have_snapshot); extern PgStat_Kind pgstat_get_kind_from_str(char *kind_str); extern bool pgstat_have_entry(PgStat_Kind kind, Oid dboid, Oid objoid); +/* GUC hook for stats_fetch_consistency */ +extern void assign_stats_fetch_consistency(int newval, void *extra); /* * Functions in pgstat_archiver.c diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out index 6a10dc462bd..e5e8f070ecb 100644 --- a/src/test/regress/expected/stats.out +++ b/src/test/regress/expected/stats.out @@ -766,6 +766,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 a6b0e9e0428..252d7161bde 100644 --- a/src/test/regress/sql/stats.sql +++ b/src/test/regress/sql/stats.sql @@ -383,6 +383,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 ----