From: Michael Paquier <michael@paquier.xyz>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: exclusion@gmail.com
Cc: 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: Mon, 1 May 2023 14:18:00 +0900
Message-ID: <ZE9LiFc7JdNHokz/@paquier.xyz> (raw)
In-Reply-To: <20230428.160752.1318937619423627249.horikyota.ntt@gmail.com>
References: <ZEtNg1EpciAQ5fjX@paquier.xyz>
<20230428.150404.2168290194163999912.horikyota.ntt@gmail.com>
<ZEtsMK8FgEE4K0w8@paquier.xyz>
<20230428.160752.1318937619423627249.horikyota.ntt@gmail.com>
On Fri, Apr 28, 2023 at 04:07:52PM +0900, Kyotaro Horiguchi wrote:
> For the the record, I'm not saying that it is dangerous to clear
> snapshots directly in the callback. In fact, as I mentioned earlier, I
> believe there is no issue with that. But, I believe it is simpler
> that the actual work is separate from the callback path since we don't
> need to worry about when the guc-callback will be called.
pgstat_clear_backend_activity_snapshot() and pgstat_assert_is_up() are
the two points of contention here. The former could be called in a
non-backend context, which would be an incorrect use of this API. I
am not completely sure that pgstat_assert_is_up() would be always
correct, as well, though I have not been able to see a problem.
Anyway, sorry for the late replay, it took me some time to study this
subsystem and understand what's going on here (like why we have fixed
snapshots but these don't list a timestamp, for example). At the end,
I think that I'm OK with your suggestion to just force a snapshot
cleanup each time the GUC is changed, mainly because that's simple to
understand, and do the clear based on the timing where the next
snapshot build would happen. I would be curious to hear arguments if
there would be a need for a more complex mechanism, but that seems
overkill to me as this is mainly here when switching out of the
"snapshot" mode. A commit/abort would force a cleanup of the
snapshot, clearing the flag, as well.
I would add a small note in the docs about this behavior. Another
thing is to add a few pg_stat_get_snapshot_timestamp() after building
a snapshot for each mode tested.
While on it, I have noticed that postgresql.conf.sample does not list
the values available for stats_fetch_consistency. These had better be
added to the sample file, no?
At the end, I am finishing with the attached, without the sample file
part. Thoughts?
--
Michael
Attachments:v3-0001-Fix-assertion-failure-on-updates-of-stats_fetch_c.patch (text/x-diff, 8.1K) download
From af11d456981908caee447b8144d4fea6ffba2c15 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
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.hindex 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.cindex 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.cindex 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.outindex 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.sqlindex 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.sgmlindex 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 <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>
--
2.40.1
signature.asc (application/pgp-signature, 832B) download
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: michael@paquier.xyz, horikyota.ntt@gmail.com, exclusion@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: <ZE9LiFc7JdNHokz/@paquier.xyz>
* 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