pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and swithing cache->snapshot
14+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2023-05-10 605994651b ccd21e1cfa Fix assertion failure when updating stats_fetch_consistency in a transaction
2023-05-10 4d47eff99c Document values of stats_fetch_consistency in postgresql.conf.sample
* BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and swithing cache->snapshot
@ 2023-02-22 06:00 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 14+ messages in thread
From: PG Bug reporting form @ 2023-02-22 06:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
The following bug has been logged on the website:
Bug reference: 17804
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 15.2
Operating system: Ubuntu 22.04
Description:
The following transaction:
BEGIN;
SET LOCAL stats_fetch_consistency = cache;
SELECT * FROM pg_stat_database;
SET LOCAL stats_fetch_consistency = snapshot;
SELECT pg_stat_get_function_calls(0);
raises an assertion failure with the following stack:
Core was generated by `postgres: law regression [local] SELECT
'.
Program terminated with signal SIGABRT, Aborted.
warning: Section `.reg-xstate/1355782' in core file too small.
#0 __pthread_kill_implementation (no_tid=0, signo=6,
threadid=140663482824512) at ./nptl/pthread_kill.c:44
44 ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0 __pthread_kill_implementation (no_tid=0, signo=6,
threadid=140663482824512) at ./nptl/pthread_kill.c:44
#1 __pthread_kill_internal (signo=6, threadid=140663482824512) at
./nptl/pthread_kill.c:78
#2 __GI___pthread_kill (threadid=140663482824512, signo=signo@entry=6) at
./nptl/pthread_kill.c:89
#3 0x00007feec4a1a476 in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#4 0x00007feec4a007f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x0000555cf0603315 in ExceptionalCondition (
conditionName=0x555cf07ecf38 "pgStatLocal.snapshot.stats->members == 0",
errorType=0x555cf07ec974 "FailedAssertion", fileName=0x555cf07ecc8f
"pgstat.c", lineNumber=948)
at assert.c:69
#6 0x0000555cf045e815 in pgstat_build_snapshot () at pgstat.c:948
#7 0x0000555cf045e39b in pgstat_fetch_entry (kind=PGSTAT_KIND_FUNCTION,
dboid=16384, objoid=0)
at pgstat.c:798
#8 0x0000555cf0461fae in pgstat_fetch_stat_funcentry (func_id=0) at
pgstat_function.c:242
#9 0x0000555cf0544c66 in pg_stat_get_function_calls (fcinfo=0x555cf164cb68)
at pgstatfuncs.c:384
#10 0x0000555cf01b7396 in ExecInterpExpr (state=0x555cf164ca80,
econtext=0x555cf164c780,
isnull=0x7ffe50dc040f) at execExprInterp.c:751
#11 0x0000555cf01b964e in ExecInterpExprStillValid (state=0x555cf164ca80,
econtext=0x555cf164c780,
isNull=0x7ffe50dc040f) at execExprInterp.c:1826
#12 0x0000555cf0217315 in ExecEvalExprSwitchContext (state=0x555cf164ca80,
econtext=0x555cf164c780,
isNull=0x7ffe50dc040f) at ../../../src/include/executor/executor.h:341
#13 0x0000555cf021738d in ExecProject (projInfo=0x555cf164ca78)
at ../../../src/include/executor/executor.h:375
#14 0x0000555cf02175d7 in ExecResult (pstate=0x555cf164c668) at
nodeResult.c:136
#15 0x0000555cf01d022d in ExecProcNodeFirst (node=0x555cf164c668) at
execProcnode.c:464
#16 0x0000555cf01c34ee in ExecProcNode (node=0x555cf164c668) at
../../../src/include/executor/executor.h:259
#17 0x0000555cf01c63f5 in ExecutePlan (estate=0x555cf164c430,
planstate=0x555cf164c668,
use_parallel_mode=false, operation=CMD_SELECT, sendTuples=true,
numberTuples=0,
direction=ForwardScanDirection, dest=0x555cf16519a0, execute_once=true)
at execMain.c:1636
#18 0x0000555cf01c3c25 in standard_ExecutorRun (queryDesc=0x555cf165bba0,
direction=ForwardScanDirection,
count=0, execute_once=true) at execMain.c:363
#19 0x0000555cf01c3a10 in ExecutorRun (queryDesc=0x555cf165bba0,
direction=ForwardScanDirection, count=0,
execute_once=true) at execMain.c:307
#20 0x0000555cf043cae6 in PortalRunSelect (portal=0x555cf15eeb90,
forward=true, count=0, dest=0x555cf16519a0)
at pquery.c:924
#21 0x0000555cf043c704 in PortalRun (portal=0x555cf15eeb90,
count=9223372036854775807, isTopLevel=true,
run_once=true, dest=0x555cf16519a0, altdest=0x555cf16519a0,
qc=0x7ffe50dc0810) at pquery.c:768
#22 0x0000555cf0435590 in exec_simple_query (
query_string=0x555cf1582570 "SELECT pg_stat_get_function_calls(0);") at
postgres.c:1250
#23 0x0000555cf043a4af in PostgresMain (dbname=0x555cf15aea18 "regression",
username=0x555cf15ae9f8 "law")
at postgres.c:4593
#24 0x0000555cf035fd2b in BackendRun (port=0x555cf15a6130) at
postmaster.c:4511
#25 0x0000555cf035f5b2 in BackendStartup (port=0x555cf15a6130) at
postmaster.c:4239
#26 0x0000555cf035b53f in ServerLoop () at postmaster.c:1806
#27 0x0000555cf035ac9c in PostmasterMain (argc=3, argv=0x555cf157da30) at
postmaster.c:1478
#28 0x0000555cf024ebdd in main (argc=3, argv=0x555cf157da30) at main.c:202
It appeared with the stats_fetch_consistency introduction (5891c7a8e).
Looks like pgstat_fetch_entry() can fill up the snapshot.stats structure
and it's contents preserved when switching the consistency mode,
what pgstat_build_snapshot() doesn't expect.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and swithing cache->snapshot
@ 2023-02-24 07:08 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 14+ messages in thread
From: Kyotaro Horiguchi @ 2023-02-24 07:08 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
At Wed, 22 Feb 2023 06:00:01 +0000, PG Bug reporting form <noreply@postgresql.org> wrote in
> The following transaction:
> BEGIN;
> SET LOCAL stats_fetch_consistency = cache;
> SELECT * FROM pg_stat_database;
> SET LOCAL stats_fetch_consistency = snapshot;
> SELECT pg_stat_get_function_calls(0);
>
> raises an assertion failure with the following stack:
> Core was generated by `postgres: law regression [local] SELECT
> '.
> Program terminated with signal SIGABRT, Aborted.
Thanks for the report! We didn't consider the case that variable is
changed in-transaction.
Perhaps we should reject changes to fetch consistency once we the
snapshot hash is created. I can think of two ways to do this. One
approach is to perform that check at GUC-level. However, this would
result in an ERROR meassage that isn't very informative. Addition to
that, it seems like aborting the transaction would be too severe a
punishment for this operation.
ERROR: invalid value for parameter "stats_fetch_consistency": "cache"
DETAIL: pgstat_fetch_consistency cannot be changed in this context.
ERROR: current transaction is aborted, commands ignored until end of transactio
The attached is an alternative solution. If pgstat_fetch_consistency
is different from the existing snapshot, pgstat_fetch_entry() will
issue a warning then continue with the old snapshot mode instead. To
make this work, I modified pgstat_prep_snapshot to determine the
actual consistency mode to use, and then the subsequent code uses the
returned mode.
WARNING: ignored in-transaction change of statistics consistency
The patch allows the transitions only none->cahce and none->snapshot.
To dislable all types of transitions, it seems that we need either an
additional boolean in the struct PgStat_LocalState or an additional
enumerator in the enyum PgStat_FetchConsistency to keep track of
whether pgstat_prep_snapshot() is called during the current
transaction.
What are your thoughts on this?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
pgstat_reject_inconsistent_consistency_mode_change.diff (text/x-patch, 7.0K) download
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 60fc4e761f..5568b9d6dc 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -172,8 +172,8 @@ static void pgstat_reset_after_failure(void);
static bool pgstat_flush_pending_entries(bool nowait);
-static void pgstat_prep_snapshot(void);
-static void pgstat_build_snapshot(void);
+static PgStat_FetchConsistency pgstat_prep_snapshot(void);
+static void pgstat_build_snapshot(PgStat_FetchConsistency fetch_consistency);
static void pgstat_build_snapshot_fixed(PgStat_Kind kind);
static inline bool pgstat_is_kind_valid(int ikind);
@@ -796,23 +796,25 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
PgStat_EntryRef *entry_ref;
void *stats_data;
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+ PgStat_FetchConsistency fetch_consistency;
/* should be called from backends */
Assert(IsUnderPostmaster || !IsPostmasterEnvironment);
Assert(!kind_info->fixed_amount);
- pgstat_prep_snapshot();
-
key.kind = kind;
key.dboid = dboid;
key.objoid = objoid;
/* if we need to build a full snapshot, do so */
- if (pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_SNAPSHOT)
- pgstat_build_snapshot();
+ fetch_consistency = pgstat_prep_snapshot();
+ /* if we need to build a full snapshot, do so */
+ if (fetch_consistency == PGSTAT_FETCH_CONSISTENCY_SNAPSHOT)
+ pgstat_build_snapshot(fetch_consistency);
+
/* if caching is desired, look up in cache */
- if (pgstat_fetch_consistency > PGSTAT_FETCH_CONSISTENCY_NONE)
+ if (fetch_consistency > PGSTAT_FETCH_CONSISTENCY_NONE)
{
PgStat_SnapshotEntry *entry = NULL;
@@ -825,18 +827,16 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
* If we built a full snapshot and the key is not in
* pgStatLocal.snapshot.stats, there are no matching stats.
*/
- if (pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_SNAPSHOT)
+ if (fetch_consistency == PGSTAT_FETCH_CONSISTENCY_SNAPSHOT)
return NULL;
}
- pgStatLocal.snapshot.mode = pgstat_fetch_consistency;
-
entry_ref = pgstat_get_entry_ref(kind, dboid, objoid, false, NULL);
if (entry_ref == NULL || entry_ref->shared_entry->dropped)
{
/* create empty entry when using PGSTAT_FETCH_CONSISTENCY_CACHE */
- if (pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_CACHE)
+ if (fetch_consistency == PGSTAT_FETCH_CONSISTENCY_CACHE)
{
PgStat_SnapshotEntry *entry = NULL;
bool found;
@@ -853,7 +853,7 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
* otherwise we could quickly end up with a fair bit of memory used due to
* repeated accesses.
*/
- if (pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_NONE)
+ if (fetch_consistency == PGSTAT_FETCH_CONSISTENCY_NONE)
stats_data = palloc(kind_info->shared_data_len);
else
stats_data = MemoryContextAlloc(pgStatLocal.snapshot.context,
@@ -865,7 +865,7 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
kind_info->shared_data_len);
pgstat_unlock_entry(entry_ref);
- if (pgstat_fetch_consistency > PGSTAT_FETCH_CONSISTENCY_NONE)
+ if (fetch_consistency > PGSTAT_FETCH_CONSISTENCY_NONE)
{
PgStat_SnapshotEntry *entry = NULL;
bool found;
@@ -915,23 +915,45 @@ pgstat_have_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
void
pgstat_snapshot_fixed(PgStat_Kind kind)
{
+ PgStat_FetchConsistency fetch_consistency;
+
Assert(pgstat_is_kind_valid(kind));
Assert(pgstat_get_kind_info(kind)->fixed_amount);
- if (pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_SNAPSHOT)
- pgstat_build_snapshot();
+ fetch_consistency = pgstat_prep_snapshot();
+ if (fetch_consistency == PGSTAT_FETCH_CONSISTENCY_SNAPSHOT)
+ pgstat_build_snapshot(fetch_consistency);
else
pgstat_build_snapshot_fixed(kind);
Assert(pgStatLocal.snapshot.fixed_valid[kind]);
}
-static void
+/*
+ * Prepare snapshot hash if needed.
+ *
+ * Returns the actual snapshot consistency mode to use.
+ */
+static PgStat_FetchConsistency
pgstat_prep_snapshot(void)
{
+ /*
+ * To maintain consistency, reject consistency changes if we have created a
+ * snapshot. Non-null stats and PGSTAT_FETCH_CONSISTENCY_NONE mode means
+ * the snapshot has been prepared and is waiting for being filled.
+ */
+ if (pgStatLocal.snapshot.stats != NULL &&
+ pgStatLocal.snapshot.mode != PGSTAT_FETCH_CONSISTENCY_NONE &&
+ pgstat_fetch_consistency != pgStatLocal.snapshot.mode)
+ {
+ ereport(WARNING,
+ errmsg("ignored in-transaction change of statistics consistency"));
+ return pgStatLocal.snapshot.mode;
+ }
+
if (pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_NONE ||
pgStatLocal.snapshot.stats != NULL)
- return;
+ return pgstat_fetch_consistency;
if (!pgStatLocal.snapshot.context)
pgStatLocal.snapshot.context = AllocSetContextCreate(TopMemoryContext,
@@ -942,22 +964,34 @@ pgstat_prep_snapshot(void)
pgstat_snapshot_create(pgStatLocal.snapshot.context,
PGSTAT_SNAPSHOT_HASH_SIZE,
NULL);
+
+ /*
+ * Enter PGSTAT_FETCH_CONSISTENCY_CACHE mode now. However, we don't enter
+ * PGSTAT_FETCH_CONSISTENCY_SNAPSHOT mode until pgstat_build_snapshot() has
+ * completed the remaining task later.
+ */
+ if (pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_CACHE)
+ pgStatLocal.snapshot.mode = pgstat_fetch_consistency;
+
+ return pgstat_fetch_consistency;
}
static void
-pgstat_build_snapshot(void)
+pgstat_build_snapshot(PgStat_FetchConsistency fetch_consistency)
{
dshash_seq_status hstat;
PgStatShared_HashEntry *p;
/* should only be called when we need a snapshot */
- Assert(pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_SNAPSHOT);
+ Assert(fetch_consistency == PGSTAT_FETCH_CONSISTENCY_SNAPSHOT);
/* snapshot already built */
if (pgStatLocal.snapshot.mode == PGSTAT_FETCH_CONSISTENCY_SNAPSHOT)
return;
- pgstat_prep_snapshot();
+ /* return immediately if we don't enter snapshot mode */
+ if (pgstat_prep_snapshot() != PGSTAT_FETCH_CONSISTENCY_SNAPSHOT)
+ return;
Assert(pgStatLocal.snapshot.stats->members == 0);
@@ -1014,7 +1048,8 @@ pgstat_build_snapshot(void)
dshash_seq_term(&hstat);
/*
- * Build snapshot of all fixed-numbered stats.
+ * Build snapshot of all fixed-numbered stats. snapshot mode is required
+ * to be PGSTAT_FETCH_CONSISTENCY_NONE tentatively while initialization.
*/
for (int kind = PGSTAT_KIND_FIRST_VALID; kind <= PGSTAT_KIND_LAST; kind++)
{
@@ -1040,7 +1075,7 @@ pgstat_build_snapshot_fixed(PgStat_Kind kind)
Assert(kind_info->fixed_amount);
Assert(kind_info->snapshot_cb != NULL);
- if (pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_NONE)
+ if (pgStatLocal.snapshot.mode == PGSTAT_FETCH_CONSISTENCY_NONE)
{
/* rebuild every time */
pgStatLocal.snapshot.fixed_valid[kind] = false;
@@ -1048,7 +1083,7 @@ pgstat_build_snapshot_fixed(PgStat_Kind kind)
else if (pgStatLocal.snapshot.fixed_valid[kind])
{
/* in snapshot mode we shouldn't get called again */
- Assert(pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_CACHE);
+ Assert(pgStatLocal.snapshot.mode == PGSTAT_FETCH_CONSISTENCY_CACHE);
return;
}
* Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot
@ 2023-02-24 16:00 Alexander Lakhin <exclusion@gmail.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Alexander Lakhin @ 2023-02-24 16:00 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; pgsql-bugs@lists.postgresql.org
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)?
If snapshots in different modes contain different data sets, will such
behaviour be unexpected for a user?
For example, if a user performs:
SET LOCAL stats_fetch_consistency = snapshot;
{some-actions-1}
SET LOCAL stats_fetch_consistency = none;
{some-actions-2}
SET LOCAL stats_fetch_consistency = snapshot;
{some-actions-3}
Should she expect that some-actions-1, some-actions-2 will be
reflected in the snapshot at the end?
Best regards,
Alexander
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot
@ 2023-04-27 10:00 Alexander Lakhin <exclusion@gmail.com>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Alexander Lakhin @ 2023-04-27 10:00 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; pgsql-bugs@lists.postgresql.org
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
----
* Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot
@ 2023-04-28 01:41 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Kyotaro Horiguchi @ 2023-04-28 01:41 UTC (permalink / raw)
To: exclusion@gmail.com; +Cc: pgsql-bugs@lists.postgresql.org
At Thu, 27 Apr 2023 13:00:00 +0300, Alexander Lakhin <exclusion@gmail.com> wrote in
> 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)?
Thank you for the patch. (Sorry, I forgot about this..)
> 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.
I tried to keep existing snapshots as long as possible, but
considering how rarely users change the variable and the lack of
necessity to to preserve the snapshots, it's simply easier to clear
them with each variable change.
Nonetheless, I'm a bit concerned about making a direct call to
pgstat_clear_snapshot() from the assign callback, it might be fine for
now, but I worry that it could an issue later on.
So, how about just settin a trigger that causes a snapshot clearing
prior to the next use, like the attached?
As for the test, I can't come up with a better one, but I think the
comment should explain its intention more clearly.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
v2-01-fix-pgstat-sfc.patch (text/x-patch, 5.6K) download
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index b125802b21..cfc6511093 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"
@@ -187,7 +187,6 @@ static inline bool pgstat_is_kind_valid(int ikind);
bool pgstat_track_counts = false;
int pgstat_fetch_consistency = PGSTAT_FETCH_CONSISTENCY_CACHE;
-
/* ----------
* state shared with pgstat_*.c
* ----------
@@ -227,6 +226,13 @@ static dlist_head pgStatPending = DLIST_STATIC_INIT(pgStatPending);
*/
static bool pgStatForceNextFlush = false;
+/*
+ * Force-clear existing snapshot before next use to prevent in-transaction
+ * change of stats_fetch_consistency from causing cache mechanism issues.
+ */
+static bool clear_existing_snapshot = false;
+
+
/*
* For assertions that check pgstat is not used before initialization / after
* shutdown.
@@ -787,6 +793,8 @@ pgstat_clear_snapshot(void)
* forward the reset request.
*/
pgstat_clear_backend_activity_snapshot();
+
+ clear_existing_snapshot = false;
}
void *
@@ -885,6 +893,9 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
TimestampTz
pgstat_get_stat_snapshot_timestamp(bool *have_snapshot)
{
+ if (clear_existing_snapshot)
+ pgstat_clear_snapshot();
+
if (pgStatLocal.snapshot.mode == PGSTAT_FETCH_CONSISTENCY_SNAPSHOT)
{
*have_snapshot = true;
@@ -929,6 +940,9 @@ pgstat_snapshot_fixed(PgStat_Kind kind)
static void
pgstat_prep_snapshot(void)
{
+ if (clear_existing_snapshot)
+ pgstat_clear_snapshot();
+
if (pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_NONE ||
pgStatLocal.snapshot.stats != NULL)
return;
@@ -1671,3 +1685,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. Clear the current snapshot to avoid issues before the
+ * next snapshot use.
+ */
+ if (pgstat_fetch_consistency != newval)
+ clear_existing_snapshot = 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/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..d62a923cac 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 doesn't cause crash
+----
+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..4ead6b16ca 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 doesn't cause crash
+----
+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
----
* Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot
@ 2023-04-28 04:00 Alexander Lakhin <exclusion@gmail.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Alexander Lakhin @ 2023-04-28 04:00 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
Hello Kyotaro-san,
28.04.2023 04:41, Kyotaro Horiguchi wrote:
> Nonetheless, I'm a bit concerned about making a direct call to
> pgstat_clear_snapshot() from the assign callback, it might be fine for
> now, but I worry that it could an issue later on.
>
> So, how about just settin a trigger that causes a snapshot clearing
> prior to the next use, like the attached?
I'm agree with the change — it's still rather simple and fixes the issue.
Maybe you would also find appropriate to update the comment for
pgstat_get_stat_snapshot_timestamp().
Perhaps add something like?:
Note that the snapshot may be cleared here, if requested.
> As for the test, I can't come up with a better one, but I think the
> comment should explain its intention more clearly.
I'd hesitated to mention a crash in the comment because I've seen
assertion failures only (and no crash with a non-assert build), so I
thought that the test should illustrate the new behavior in the first place.
But I'm not opposed to your comment change, maybe it's more prominent indeed.
Thank you for the fix!
Best regards,
Alexander
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot
@ 2023-04-28 04:37 Michael Paquier <michael@paquier.xyz>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2023-04-28 04:37 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; pgsql-bugs@lists.postgresql.org
On Fri, Apr 28, 2023 at 07:00:01AM +0300, Alexander Lakhin wrote:
> 28.04.2023 04:41, Kyotaro Horiguchi wrote:
>> Nonetheless, I'm a bit concerned about making a direct call to
>> pgstat_clear_snapshot() from the assign callback, it might be fine for
>> now, but I worry that it could an issue later on.
>>
>> So, how about just settin a trigger that causes a snapshot clearing
>> prior to the next use, like the attached?
>
> I'm agree with the change — it's still rather simple and fixes the issue.
> Maybe you would also find appropriate to update the comment for
> pgstat_get_stat_snapshot_timestamp().
> Perhaps add something like?:
> Note that the snapshot may be cleared here, if requested.
FWIW, I was looking at this patch and this proposal of relying on a
static flag to conditionally clear a snapshot looks rather brittle by
design to me because this relies on quite a few assumptions that the
snapshot will always be cleared when necessary, as proved by the two
code paths of pgstat.c patched where each gain a check on
clear_existing_snapshot. The AtEOXacts for pgstat when doing an
abort/commit/prepare would guarantee that the flag is cleared, still
that's not really exciting. What is the problem in forcing a snapshot
cleanup each time stats_fetch_consistency is switched to a new value?
Somebody using SET LOCAL on that means, at least it sounds to me as
such, that they don't really care about the current snapshot contents
so they'd better be wiped out. And the cleanup timing does not really
seem to matter much.
>> As for the test, I can't come up with a better one, but I think the
>> comment should explain its intention more clearly.
>
> I'd hesitated to mention a crash in the comment because I've seen
> assertion failures only (and no crash with a non-assert build), so I
> thought that the test should illustrate the new behavior in the first place.
> But I'm not opposed to your comment change, maybe it's more prominent indeed.
If the issue is fixed, there would be no crash.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot
@ 2023-04-28 06:04 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 14+ messages in thread
From: Kyotaro Horiguchi @ 2023-04-28 06:04 UTC (permalink / raw)
To: michael@paquier.xyz; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
At Fri, 28 Apr 2023 13:37:23 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> On Fri, Apr 28, 2023 at 07:00:01AM +0300, Alexander Lakhin wrote:
> > 28.04.2023 04:41, Kyotaro Horiguchi wrote:
> >> Nonetheless, I'm a bit concerned about making a direct call to
> >> pgstat_clear_snapshot() from the assign callback, it might be fine for
> >> now, but I worry that it could an issue later on.
> >>
> >> So, how about just settin a trigger that causes a snapshot clearing
> >> prior to the next use, like the attached?
> >
> > I'm agree with the change $B!=(B it's still rather simple and fixes the issue.
> > Maybe you would also find appropriate to update the comment for
> > pgstat_get_stat_snapshot_timestamp().
> > Perhaps add something like?:
> > Note that the snapshot may be cleared here, if requested.
>
> FWIW, I was looking at this patch and this proposal of relying on a
> static flag to conditionally clear a snapshot looks rather brittle by
> design to me because this relies on quite a few assumptions that the
> snapshot will always be cleared when necessary, as proved by the two
> code paths of pgstat.c patched where each gain a check on
pgstat_get_stat_snapshot_timestmp() necessarily requires that. Just
for seeming consistency. The only significant part is
gstat_prep_snapshot, which is always called when the caller wants a
snapshot.
> clear_existing_snapshot. The AtEOXacts for pgstat when doing an
> abort/commit/prepare would guarantee that the flag is cleared, still
> that's not really exciting. What is the problem in forcing a snapshot
> cleanup each time stats_fetch_consistency is switched to a new value?
Just I wanted not to do that much in the guc callback including memory
context operations. If it is compeltly safe, I don't object just
clearing snapshots in the callback.
> Somebody using SET LOCAL on that means, at least it sounds to me as
> such, that they don't really care about the current snapshot contents
> so they'd better be wiped out. And the cleanup timing does not really
> seem to matter much.
>
> >> As for the test, I can't come up with a better one, but I think the
> >> comment should explain its intention more clearly.
> >
> > I'd hesitated to mention a crash in the comment because I've seen
> > assertion failures only (and no crash with a non-assert build), so I
> > thought that the test should illustrate the new behavior in the first place.
> > But I'm not opposed to your comment change, maybe it's more prominent indeed.
>
> If the issue is fixed, there would be no crash.
If crash is not appropriate there, I am fine with "changing the
variable clears snapshot".
Regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot
@ 2023-04-28 06:06 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
1 sibling, 0 replies; 14+ messages in thread
From: Kyotaro Horiguchi @ 2023-04-28 06:06 UTC (permalink / raw)
To: michael@paquier.xyz; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
At Fri, 28 Apr 2023 15:04:04 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
> At Fri, 28 Apr 2023 13:37:23 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> > FWIW, I was looking at this patch and this proposal of relying on a
> > static flag to conditionally clear a snapshot looks rather brittle by
> > design to me because this relies on quite a few assumptions that the
> > snapshot will always be cleared when necessary, as proved by the two
> > code paths of pgstat.c patched where each gain a check on
>
> pgstat_get_stat_snapshot_timestmp() necessarily requires that. Just
Sorry, this is actually the opposite.
pgstat_get_stat_snapshot_timestmp() doesn't necessarily requires that. Just
> for seeming consistency. The only significant part is
> gstat_prep_snapshot, which is always called when the caller wants a
> snapshot.
Regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot
@ 2023-04-28 06:48 Michael Paquier <michael@paquier.xyz>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
1 sibling, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2023-04-28 06:48 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Apr 28, 2023 at 03:04:04PM +0900, Kyotaro Horiguchi wrote:
> Just I wanted not to do that much in the guc callback including memory
> context operations. If it is compeltly safe, I don't object just
> clearing snapshots in the callback.
I vaguely recalled some memory context deletions done in one of the
assign callbacks, like the one for the plan resets, but it doesn't
seem to be the case, looking closely.. Hm.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot
@ 2023-04-28 07:07 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 14+ messages in thread
From: Kyotaro Horiguchi @ 2023-04-28 07:07 UTC (permalink / raw)
To: michael@paquier.xyz; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
At Fri, 28 Apr 2023 15:48:16 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> On Fri, Apr 28, 2023 at 03:04:04PM +0900, Kyotaro Horiguchi wrote:
> > Just I wanted not to do that much in the guc callback including memory
> > context operations. If it is compeltly safe, I don't object just
> > clearing snapshots in the callback.
>
> I vaguely recalled some memory context deletions done in one of the
> assign callbacks, like the one for the plan resets, but it doesn't
> seem to be the case, looking closely.. Hm.
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.
Regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot
@ 2023-05-01 05:18 Michael Paquier <michael@paquier.xyz>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2023-05-01 05:18 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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.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 <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
* Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot
@ 2023-05-06 03:28 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2023-05-06 03:28 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, May 01, 2023 at 02:18:00PM +0900, Michael Paquier wrote:
> At the end, I am finishing with the attached, without the sample file
> part. Thoughts?
FYI, I am planning to get this stuff done next week after the next set
of minor releases gets tagged (time was a bit scarce here this week to
get that properly done). If there are no objections, of course.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot
@ 2023-05-10 02:26 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 14+ messages in thread
From: Michael Paquier @ 2023-05-10 02:26 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Sat, May 06, 2023 at 12:28:51PM +0900, Michael Paquier wrote:
> FYI, I am planning to get this stuff done next week after the next set
> of minor releases gets tagged (time was a bit scarce here this week to
> get that properly done). If there are no objections, of course.
Okay, so applied and backpatched down to 15 after a second review of
the whole.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2023-05-10 02:26 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-02-22 06:00 BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and swithing cache->snapshot PG Bug reporting form <noreply@postgresql.org>
2023-02-24 07:08 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2023-02-24 16:00 ` Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot Alexander Lakhin <exclusion@gmail.com>
2023-04-27 10:00 ` Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot Alexander Lakhin <exclusion@gmail.com>
2023-04-28 01:41 ` Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2023-04-28 04:00 ` Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot Alexander Lakhin <exclusion@gmail.com>
2023-04-28 04:37 ` Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot Michael Paquier <michael@paquier.xyz>
2023-04-28 06:04 ` Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2023-04-28 06:06 ` Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2023-04-28 06:48 ` Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot Michael Paquier <michael@paquier.xyz>
2023-04-28 07:07 ` Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2023-05-01 05:18 ` Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot Michael Paquier <michael@paquier.xyz>
2023-05-06 03:28 ` Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot Michael Paquier <michael@paquier.xyz>
2023-05-10 02:26 ` Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot Michael Paquier <michael@paquier.xyz>
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