Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 5 Jun 2024 15:23:21 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Floris Van Nee <florisvannee@optiver.com>
Cc: Andres Freund <andres@anarazel.de>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	"exclusion@gmail.com" <exclusion@gmail.com>,
	"pgsql-bugs@lists.postgresql.org" <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #17947: Combination of replslots pgstat issues causes
 error/assertion failure
Message-ID: <ZmAEWcFYIhnHMs7C@paquier.xyz>
References: <17947-b9554521ad963c9c@postgresql.org>
 <20230601.084245.1524588196504400333.horikyota.ntt@gmail.com>
 <20240505183741.5tefii3razzefvtc@awork3.anarazel.de>
 <dd4c68beba5c40c5a7946569d450ec59@Optiver.com>
 <Zl6lQ_K0-QOfqUS4@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="nXsFHUWRReX2YR78"
Content-Disposition: inline
In-Reply-To: <Zl6lQ_K0-QOfqUS4@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/ZmAEWcFYIhnHMs7C%40paquier.xyz>
Precedence: bulk


--nXsFHUWRReX2YR78
Content-Type: multipart/mixed; boundary="NTpIiG+6eBsRTxeC"
Content-Disposition: inline


--NTpIiG+6eBsRTxeC
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Jun 04, 2024 at 02:25:23PM +0900, Michael Paquier wrote:
>> This does mean it's unfortunately unrelated to the issue I reported
>> here
>> https://www.postgresql.org/message-id/flat/20240505223546.6yvjzgqifuoiii=
3e%40awork3.anarazel.de#1dfe3ecf6a75ace833444bdc0d268f4a
>> , because the issue in the current thread is fixed by changing a
>> replication-slot call (which is not used in the database for which I
>> reported it).
>=20
> Yeah, It's clear that we don't have enough information to proceed
> here.  It seems to me that there is a good argument for addressing
> this one first, separately of the other one.

I have been playing with this patch and scenarios of replslot
create/drop similar to what's been sent at the top of this thread, and
the solution proposed is proving to be stable enough to not see a
problem in the pgstats machinery after letting the test run 30 minutes
or so (switched sleep intervals for one).

Attached is an updated patch with refined comments at the top of
pgstat_drop_entry(), to be applied to 15~.

I'm kind of feeling that this is not the end of it, even if the use of
the drop routine is inconsistent in this code path.  Perhaps that's
just my spidey sense being incorrect here..
--
Michael

--NTpIiG+6eBsRTxeC
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-Notify-stats-gc-when-dropping-logical-replication.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 290cc849f222680bf5f69303ed368ada6bb3eed4 Mon Sep 17 00:00:00 2001
=46rom: Floris van Nee <florisvannee@optiver.com>
Date: Sun, 2 Jun 2024 22:35:44 +0200
Subject: [PATCH v2] Notify stats gc when dropping logical replication slot

pgstat_drop_replslot calls pgstat_drop_entry without checking the
result. If pgstat_drop_entry cannot free the entry from shared memory
completely, the gc function must be called to flag it for other
backends. This is similar to what AtEOXact_PgStat_DroppedStats and
similar functions do.
---
 src/backend/utils/activity/pgstat_replslot.c |  5 +++--
 src/backend/utils/activity/pgstat_shmem.c    | 11 +++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/backend/utils/activity/pgstat_replslot.c b/src/backend/uti=
ls/activity/pgstat_replslot.c
index 889e86ac5a..da11b86744 100644
--- a/src/backend/utils/activity/pgstat_replslot.c
+++ b/src/backend/utils/activity/pgstat_replslot.c
@@ -157,8 +157,9 @@ pgstat_drop_replslot(ReplicationSlot *slot)
 {
 	Assert(LWLockHeldByMeInMode(ReplicationSlotAllocationLock, LW_EXCLUSIVE));
=20
-	pgstat_drop_entry(PGSTAT_KIND_REPLSLOT, InvalidOid,
-					  ReplicationSlotIndex(slot));
+	if (!pgstat_drop_entry(PGSTAT_KIND_REPLSLOT, InvalidOid,
+						   ReplicationSlotIndex(slot)))
+		pgstat_request_entry_refs_gc();
 }
=20
 /*
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/=
activity/pgstat_shmem.c
index 91591da395..4a4b69891d 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -855,6 +855,17 @@ pgstat_drop_database_and_contents(Oid dboid)
 		pgstat_request_entry_refs_gc();
 }
=20
+/*
+ * Drop a single stats entry.
+ *
+ * This routine returns false if the stats entry of the dropped object cou=
ld
+ * not be freed, true otherwise.
+ *
+ * The callers of this function should call pgstat_request_entry_refs_gc()
+ * if the stats entry could not be freed, to ensure that this entry's memo=
ry
+ * can be reclaimed later by a different backend calling
+ * pgstat_gc_entry_refs().
+ */
 bool
 pgstat_drop_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
 {
--=20
2.43.0


--NTpIiG+6eBsRTxeC--

--nXsFHUWRReX2YR78
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmZgBFkACgkQnvQgOdby
QH3zlxAAlU+CzOvwz7Daja9ggL45q+Bxsm6Dx2RR/X3GgDhnGYdVtOfLwSeecO3r
wfwodrM2q0slA3ZxoQZTl+CEJOBpGayEtxp6lgudwSzs4ofW/O16iZ28FKO2CVxp
Gkvq6HqgWzwhP9nb8QJBrqrUQWRj8BdC/waPVendza7CsAceLeg4uhciUbOUwk9u
hy2DRLAiue19KXr3Vmg/ZW8TzxQgURfsXhQmXqgaDtyxlZ4IjVduq08dev63/UZ5
7F0UVRwBrfJ8gKaxR+0SoSsHEua4YppEs38plYUpXSV80VP9Hj7/6V547jMiUNsQ
pQmUX62XF0lC5JBZ3G7Fgyu/obqPZAWT7XVGyGBeiCS3JqLi12SF/WpUH4Kvd9JG
5xOE1Sf3CoXVjta/aLrZUX/RpbMw8LeWpWci4Z8PAm7FuwOf3HrgxQEg2uL++YLK
FeGFt2ZeDoeACExm55wdKTcsritBr13eb2PBB6rx0hn45fTD8YOFNvxtopHjGCYK
vpOmWMAAmUSXq/O1KA4cKdUU8LGIzXsl0LXQqNHaFMcCYXCABsVKaWd5PCH9Vpkv
pBsXoqqdcOCyh4AqCDgwDOsSBqJrQCnVQRZvMoKybb74PGtDIZOQ7+AP02hxLkEZ
uIx5+o8YOptgzqj+v3z0UVvowMFphmPiA2LtEaF8U2qsDfLlDjA=
=+9F1
-----END PGP SIGNATURE-----

--nXsFHUWRReX2YR78--