pg.ddx.io pgsql-bugs@postgresql.org mailing list archive help / color / mirror / Atom feed From: Alexander Lakhin <exclusion@gmail.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
To: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot
Date: Thu, 27 Apr 2023 13:00:00 +0300
Message-ID: <f3f81a47-e417-f23e-ac4b-8ef3b32c7b93@gmail.com> (raw )
In-Reply-To: <1836967a-9f78-aed8-1ff1-3351c504e8f0@gmail.com >
References: <17804-2a118cd046f2d0e5@postgresql.org >
<20230224.160812.1001813717917457753.horikyota.ntt@gmail.com >
<1836967a-9f78-aed8-1ff1-3351c504e8f0@gmail.com >
Hello,
24.02.2023 19:00, Alexander Lakhin wrote:
> Hello Kyotaro-san,
> 24.02.2023 10:08, Kyotaro Horiguchi wrote:
>> Thanks for the report! We didn't consider the case that variable is
>> changed in-transaction.
>>
>>
>> What are your thoughts on this?
> What if we implement assign_pgstat_fetch_consistency() and execute
> pgstat_clear_snapshot() in it when the mode switch happens (to avoid
> possible inconsistency between the GUC and the current pgstat state)?
Please look at the attached patch, which implements that idea and also
includes a simple test. The behavior change doesn't affect any of existing
tests and seems natural to me, so I haven't added a note about it in the docs.
Best regards,
Alexander
Attachments:
v1-01-fix-pgstat-sfc.patch (text/x-patch, 4.1K) download
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index b125802b21..4eddecaa77 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"
@@ -1671,3 +1671,17 @@ 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)
+{
+ /*
+ * Must clear the snapshot when changing the consistency mode, to avoid
+ * discrepancies between pgstat_fetch_consistency and the snapshot state.
+ */
+ if (pgstat_fetch_consistency != newval)
+ pgstat_clear_snapshot();
+}
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/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/test/regress/expected/stats.out b/src/test/regress/expected/stats.out
index 813d6d39ea..bb743ae15e 100644
--- a/src/test/regress/expected/stats.out
+++ b/src/test/regress/expected/stats.out
@@ -985,6 +985,35 @@ SELECT pg_stat_get_snapshot_timestamp();
COMMIT;
----
+-- Changing fetch consistency mode inside transaction
+----
+BEGIN;
+SET LOCAL stats_fetch_consistency = cache;
+-- filling stats for the cache mode
+SELECT pg_stat_get_function_calls(0);
+ pg_stat_get_function_calls
+----------------------------
+
+(1 row)
+
+SET LOCAL stats_fetch_consistency = snapshot;
+-- verify that there is no failure on accessing pre-existing snapshot
+SELECT pg_stat_get_function_calls(0);
+ pg_stat_get_function_calls
+----------------------------
+
+(1 row)
+
+SET LOCAL stats_fetch_consistency = none;
+-- verify that snapshot is cleared on changing stats_fetch_consistency
+SELECT pg_stat_get_snapshot_timestamp();
+ pg_stat_get_snapshot_timestamp
+--------------------------------
+
+(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..502d35e6eb 100644
--- a/src/test/regress/sql/stats.sql
+++ b/src/test/regress/sql/stats.sql
@@ -476,6 +476,21 @@ SELECT pg_stat_clear_snapshot();
SELECT pg_stat_get_snapshot_timestamp();
COMMIT;
+----
+-- Changing fetch consistency mode inside transaction
+----
+BEGIN;
+SET LOCAL stats_fetch_consistency = cache;
+-- filling stats for the cache mode
+SELECT pg_stat_get_function_calls(0);
+SET LOCAL stats_fetch_consistency = snapshot;
+-- verify that there is no failure on accessing pre-existing snapshot
+SELECT pg_stat_get_function_calls(0);
+SET LOCAL stats_fetch_consistency = none;
+-- verify that snapshot is cleared on changing stats_fetch_consistency
+SELECT pg_stat_get_snapshot_timestamp();
+ROLLBACK;
+
----
-- pg_stat_have_stats behavior
----
view thread (14+ messages) latest in thread
Message-ID: <f3f81a47-e417-f23e-ac4b-8ef3b32c7b93@gmail.com>
Permalink: ../f3f81a47-e417-f23e-ac4b-8ef3b32c7b93@gmail.com/
Also on: postgresql.org/message-id/f3f81a47-e417-f23e-ac4b-8ef3b32c7b93@gmail.com
copy link · copy postgr.es · copy git am
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: pgsql-bugs@postgresql.org
Cc: exclusion@gmail.com, horikyota.ntt@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot
In-Reply-To: <f3f81a47-e417-f23e-ac4b-8ef3b32c7b93@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox