Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 1 May 2023 14:18:00 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: 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
Message-ID: <ZE9LiFc7JdNHokz/@paquier.xyz>
References: <ZEtNg1EpciAQ5fjX@paquier.xyz>
 <20230428.150404.2168290194163999912.horikyota.ntt@gmail.com>
 <ZEtsMK8FgEE4K0w8@paquier.xyz>
 <20230428.160752.1318937619423627249.horikyota.ntt@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="cx1P3/2YRQBVo1sz"
Content-Disposition: inline
In-Reply-To: <20230428.160752.1318937619423627249.horikyota.ntt@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZE9LiFc7JdNHokz/%40paquier.xyz>
Precedence: bulk


--cx1P3/2YRQBVo1sz
Content-Type: multipart/mixed; boundary="4PEc7oYrW9N10Y5a"
Content-Disposition: inline


--4PEc7oYrW9N10Y5a
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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=20
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

--4PEc7oYrW9N10Y5a
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0001-Fix-assertion-failure-on-updates-of-stats_fetch_c.patch"
Content-Transfer-Encoding: quoted-printable

=46rom af11d456981908caee447b8144d4fea6ffba2c15 Mon Sep 17 00:00:00 2001
=46rom: 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, GucSo=
urce 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 so=
urce);
 extern bool check_synchronous_standby_names(char **newval, void **extra,
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activi=
ty/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 =3D DLIST_STATIC_INIT(=
pgStatPending);
  */
 static bool pgStatForceNextFlush =3D false;
=20
+/*
+ * Force-clear existing snapshot before next use when stats_fetch_consiste=
ncy
+ * is changed.
+ */
+static bool	force_stats_snapshot_clear =3D false;
+
+
 /*
  * For assertions that check pgstat is not used before initialization / af=
ter
  * 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 =3D false;
 }
=20
 void *
@@ -885,6 +896,9 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, Oid obj=
oid)
 TimestampTz
 pgstat_get_stat_snapshot_timestamp(bool *have_snapshot)
 {
+	if (force_stats_snapshot_clear)
+		pgstat_clear_snapshot();
+
 	if (pgStatLocal.snapshot.mode =3D=3D PGSTAT_FETCH_CONSISTENCY_SNAPSHOT)
 	{
 		*have_snapshot =3D 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 =3D=3D PGSTAT_FETCH_CONSISTENCY_NONE ||
 		pgStatLocal.snapshot.stats !=3D 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 !=3D newval)
+		force_stats_snapshot_clear =3D true;
+}
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/g=
uc_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[] =3D
 		},
 		&pgstat_fetch_consistency,
 		PGSTAT_FETCH_CONSISTENCY_CACHE, stats_fetch_consistency,
-		NULL, NULL, NULL
+		NULL, assign_stats_fetch_consistency, NULL
 	},
=20
 	{
diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expecte=
d/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();
=20
 COMMIT;
 ----
+-- Changing stats_fetch_consistency in a transaction.
+----
+BEGIN;
+-- Stats filled under the cache mode
+SET LOCAL stats_fetch_consistency =3D cache;
+SELECT pg_stat_get_function_calls(0);
+ pg_stat_get_function_calls=20
+----------------------------
+                          =20
+(1 row)
+
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+ snapshot_ok=20
+-------------
+ f
+(1 row)
+
+-- Success in accessing pre-existing snapshot data.
+SET LOCAL stats_fetch_consistency =3D snapshot;
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+ snapshot_ok=20
+-------------
+ f
+(1 row)
+
+SELECT pg_stat_get_function_calls(0);
+ pg_stat_get_function_calls=20
+----------------------------
+                          =20
+(1 row)
+
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+ snapshot_ok=20
+-------------
+ t
+(1 row)
+
+-- Snapshot cleared.
+SET LOCAL stats_fetch_consistency =3D none;
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+ snapshot_ok=20
+-------------
+ f
+(1 row)
+
+SELECT pg_stat_get_function_calls(0);
+ pg_stat_get_function_calls=20
+----------------------------
+                          =20
+(1 row)
+
+SELECT pg_stat_get_snapshot_timestamp() IS NOT NULL AS snapshot_ok;
+ snapshot_ok=20
+-------------
+ 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;
=20
+----
+-- Changing stats_fetch_consistency in a transaction.
+----
+BEGIN;
+-- Stats filled under the cache mode
+SET LOCAL stats_fetch_consistency =3D 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 =3D 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 =3D 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' WI=
TH csv;
         called. When set to <literal>snapshot</literal>, the first statist=
ics
         access caches all statistics accessible in the current database, u=
ntil
         the end of the transaction unless
-        <function>pg_stat_clear_snapshot()</function> is called. The defau=
lt
-        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>
--=20
2.40.1


--4PEc7oYrW9N10Y5a--

--cx1P3/2YRQBVo1sz
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmRPS4gACgkQnvQgOdby
QH0toQ//RRoSgC5tkoYUlozO418mxKjiA0BNV+As+n5l5E+TOniivRiX7jEPpgjG
ZpwCAWaCcsIom/mev3SMwudSD8QnfmM68olLBq9tkWVL/rRg5Z7wl/iHIzJ9R5u2
/eyMw13nDTQ1rkZXSM9WacIlFXZxPgl/JW8rH3myW4SYtvGE8CkZPJLHG6Y3iFZH
oVAWRl0NplXWaUKRrZ+cs4MGVk1vGoJ8Nbm+tF3fPHG28EG1ihRPM4r7T71uf8LS
MVmwuNTKTTIUSjRTRG4uzRLWQ8awXCuq839X3F0Upmsgg45rWDRWVhNaQfMoLMk+
T2nhkSJEggrBUhwZ9yRP4pRXpddzw2B9qIhdoCRi24FyTafm0JwqtAEfxxuqPgVc
H8idK64TgTkfN0yc7C03f7xPi0zychqCLTxc9nX1gsUFmrVN49MQzEBn0c/LmjLY
fb4WlOeEc5WZEJJ4YShtHAMl7zAvwhiMiYoSqm05hRkbgnuZ6ZH2Yiroj114sbXk
Kh63AWIIb6gmlQuT28E48d29nIqZO4irEPBRAKxozcCh4UBGRXPgrUb0CN05Q9Uh
MdhDID4xBmIZHkKTE94y9l/gCleQbrXeZqUlLoG6GB6BQL1PMVB6Txn99LFk3vBY
eOr9Pfakr2bajvhaExv/zedY9nAeJ+6PQc3kDqgdj+WSJfp/bDI=
=DM5b
-----END PGP SIGNATURE-----

--cx1P3/2YRQBVo1sz--