From: Floris Van Nee <florisvannee@Optiver.com>
To: Andres Freund <andres@anarazel.de>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: exclusion@gmail.com <exclusion@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Subject: RE: BUG #17947: Combination of replslots pgstat issues causes error/assertion failure
Date: Sun, 2 Jun 2024 21:31:56 +0000
Message-ID: <dd4c68beba5c40c5a7946569d450ec59@Optiver.com> (raw)
In-Reply-To: <20240505183741.5tefii3razzefvtc@awork3.anarazel.de>
References: <17947-b9554521ad963c9c@postgresql.org>
<20230601.084245.1524588196504400333.horikyota.ntt@gmail.com>
<20240505183741.5tefii3razzefvtc@awork3.anarazel.de>
>
> I think the problem might actually be much simpler - when
> pgstat_drop_entry() can't free the entry, we don't call
> pgstat_request_entry_refs_gc(), in contrast to
> pgstat_drop_database_and_contents(), pgstat_drop_all_entries().
> Because of that, entry refs to dropped stats entries can survive an effectively
> unbound amount of time. If pgstat_request_entry_refs_gc() had been
> called, the local entry would have been released, before we reused the slot's
> stats entry. There's no race around the entry being dropped concurrently
> with acquiring an entry references, as external locking needs to prevent an
> entry from being dropped while new references are being established.
This sounds like a reasonable root cause for this thread.
> I can't explain why pgstat_drop_entry() doesn't call
> pgstat_request_entry_refs_gc(). It seems fairly obvious that it ought to?
It looks like pgstat_drop_entry expects callers to take care of this. All callers in pgstat_xact.c (AtEOXact_PgStat_DroppedStats for example) are dropping stats for several relations at once, and then at the end they call pgstat_request_entry_refs_gc once if needed. I'm guessing this was an optimization.
However, pgstat_drop_replslot didn't get that memo. Attached patch adds a call to gc in pgstat_drop_replslot to bring it in line with usage in pgstat_xact.c. On my setup this fixes the issue that occurs on this thread.
>
> I think there would be a leftover edge-case, namely that
> pgstat_gc_entry_refs() doesn't gc entries with pending data. But we could
> just do that - and it should be correct, afaict. I wonder if the reason we didn't
> is that we introduced PgStat_KindInfo.delete_pending_cb later?
Can you clarify why this would be safe to do? Looking at commit history, it looks like they were added in the same commit 5891c7a8e which introduced the shared memory stats. I can reason why it *should* be safe, but I can't find a discussion around why every single function except pgstat_drop_entry uses discard_pending=false if it would be safe to pass true everywhere. It seems strange that it'd have been chosen without reason.
This does mean it's unfortunately unrelated to the issue I reported here https://www.postgresql.org/message-id/flat/20240505223546.6yvjzgqifuoiii3e%40awork3.anarazel.de#1dfe... , 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).
-Floris
Attachments:0001-Notify-stats-gc-when-dropping-logical-replication-sl.patch (application/octet-stream, 2.0K) download
From 3fc4823142ee12f853247ccd1c96d896748170b5 Mon Sep 17 00:00:00 2001
From: Floris van Nee <florisvannee@optiver.com>
Date: Sun, 2 Jun 2024 22:35:44 +0200
Subject: [PATCH] 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 | 7 +++++++
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/activity/pgstat_replslot.c b/src/backend/utils/activity/pgstat_replslot.cindex 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));
- pgstat_drop_entry(PGSTAT_KIND_REPLSLOT, InvalidOid,- ReplicationSlotIndex(slot));+ if (!pgstat_drop_entry(PGSTAT_KIND_REPLSLOT, InvalidOid,+ ReplicationSlotIndex(slot)))+ pgstat_request_entry_refs_gc();
}
/*
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.cindex 91591da395..a44ac559b2 100644--- a/src/backend/utils/activity/pgstat_shmem.c+++ b/src/backend/utils/activity/pgstat_shmem.c@@ -855,6 +855,13 @@ pgstat_drop_database_and_contents(Oid dboid)
pgstat_request_entry_refs_gc();
}
+/*+ * This function expects callers to call pgstat_request_entry_refs_gc if necessary.+ * Generally this must be called if the return value is false, when the object+ * couldn't be freed.+ * Note this is different behavior than other drop functions like pgstat_drop_all_entries+ * which do call pgstat_request_entry_refs_gc+ */
bool
pgstat_drop_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
{
--
2.30.1
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: florisvannee@Optiver.com, andres@anarazel.de, horikyota.ntt@gmail.com, exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: RE: BUG #17947: Combination of replslots pgstat issues causes error/assertion failure
In-Reply-To: <dd4c68beba5c40c5a7946569d450ec59@Optiver.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