From: Floris Van Nee <florisvannee@Optiver.com>
To: Andres Freund <andres@anarazel.de>,
        Kyotaro Horiguchi
	<horikyota.ntt@gmail.com>
CC: "exclusion@gmail.com" <exclusion@gmail.com>,
        "pgsql-bugs@lists.postgresql.org" <pgsql-bugs@lists.postgresql.org>
Thread-Topic: [External] Re: BUG #17947: Combination of replslots pgstat
 issues causes error/assertion failure [External]
Thread-Index: AQHZlBmhKd6ff+auW0KpzehwxH4Xw7GK7OSAgCxGHIA=
Date: Sun, 2 Jun 2024 21:31:56 +0000
Message-ID: <dd4c68beba5c40c5a7946569d450ec59@Optiver.com>
References: <17947-b9554521ad963c9c@postgresql.org>
 <20230601.084245.1524588196504400333.horikyota.ntt@gmail.com>
 <20240505183741.5tefii3razzefvtc@awork3.anarazel.de>
In-Reply-To: <20240505183741.5tefii3razzefvtc@awork3.anarazel.de>
Accept-Language: en-US
Content-Language: en-US
Content-Type: multipart/mixed;
	boundary="_002_dd4c68beba5c40c5a7946569d450ec59Optivercom_"
MIME-Version: 1.0
Subject: RE: BUG #17947: Combination of replslots pgstat issues causes
 error/assertion failure
Archived-At: 
 <https://www.postgresql.org/message-id/dd4c68beba5c40c5a7946569d450ec59%40Optiver.com>
Precedence: bulk

--_002_dd4c68beba5c40c5a7946569d450ec59Optivercom_
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable

>=20
> 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 effec=
tively
> unbound amount of time. If pgstat_request_entry_refs_gc() had been
> called, the local entry would have been released, before we reused the sl=
ot'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?
=20
It looks like pgstat_drop_entry expects callers to take care of this. All c=
allers in pgstat_xact.c (AtEOXact_PgStat_DroppedStats for example) are drop=
ping stats for several relations at once, and then at the end they call pgs=
tat_request_entry_refs_gc once if needed. I'm guessing this was an optimiza=
tion.

However, pgstat_drop_replslot didn't get that memo. Attached patch adds a c=
all 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.

>=20
> 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 w=
e 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 t=
he shared memory stats. I can reason why it *should* be safe, but I can't f=
ind a discussion around why every single function except pgstat_drop_entry =
uses discard_pending=3Dfalse if it would be safe to pass true everywhere. I=
t seems strange that it'd have been chosen without reason.

This does mean it's unfortunately unrelated to the issue I reported here ht=
tps://www.postgresql.org/message-id/flat/20240505223546.6yvjzgqifuoiii3e%40=
awork3.anarazel.de#1dfe3ecf6a75ace833444bdc0d268f4a , because the issue in =
the current thread is fixed by changing a replication-slot call (which is n=
ot used in the database for which I reported it).

-Floris


--_002_dd4c68beba5c40c5a7946569d450ec59Optivercom_
Content-Type: application/octet-stream;
	name="0001-Notify-stats-gc-when-dropping-logical-replication-sl.patch"
Content-Description: 
 0001-Notify-stats-gc-when-dropping-logical-replication-sl.patch
Content-Disposition: attachment;
	filename="0001-Notify-stats-gc-when-dropping-logical-replication-sl.patch";
	size=2059; creation-date="Sun, 02 Jun 2024 20:44:42 GMT";
	modification-date="Sun, 02 Jun 2024 20:43:50 GMT"
Content-Transfer-Encoding: base64

RnJvbSAzZmM0ODIzMTQyZWUxMmY4NTMyNDdjY2QxYzk2ZDg5Njc0ODE3MGI1IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBGbG9yaXMgdmFuIE5lZSA8ZmxvcmlzdmFubmVlQG9wdGl2ZXIu
Y29tPgpEYXRlOiBTdW4sIDIgSnVuIDIwMjQgMjI6MzU6NDQgKzAyMDAKU3ViamVjdDogW1BBVENI
XSBOb3RpZnkgc3RhdHMgZ2Mgd2hlbiBkcm9wcGluZyBsb2dpY2FsIHJlcGxpY2F0aW9uIHNsb3QK
CnBnc3RhdF9kcm9wX3JlcGxzbG90IGNhbGxzIHBnc3RhdF9kcm9wX2VudHJ5IHdpdGhvdXQgY2hl
Y2tpbmcgdGhlIHJlc3VsdC4gSWYgcGdzdGF0X2Ryb3BfZW50cnkgY2Fubm90IGZyZWUgdGhlIGVu
dHJ5IGZyb20gc2hhcmVkIG1lbW9yeSBjb21wbGV0ZWx5LCB0aGUgZ2MgZnVuY3Rpb24gbXVzdCBi
ZSBjYWxsZWQgdG8gZmxhZyBpdCBmb3Igb3RoZXIgYmFja2VuZHMuIFRoaXMgaXMgc2ltaWxhciB0
byB3aGF0IEF0RU9YYWN0X1BnU3RhdF9Ecm9wcGVkU3RhdHMgYW5kIHNpbWlsYXIgZnVuY3Rpb25z
IGRvLgotLS0KIHNyYy9iYWNrZW5kL3V0aWxzL2FjdGl2aXR5L3Bnc3RhdF9yZXBsc2xvdC5jIHwg
NSArKystLQogc3JjL2JhY2tlbmQvdXRpbHMvYWN0aXZpdHkvcGdzdGF0X3NobWVtLmMgICAgfCA3
ICsrKysrKysKIDIgZmlsZXMgY2hhbmdlZCwgMTAgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMo
LSkKCmRpZmYgLS1naXQgYS9zcmMvYmFja2VuZC91dGlscy9hY3Rpdml0eS9wZ3N0YXRfcmVwbHNs
b3QuYyBiL3NyYy9iYWNrZW5kL3V0aWxzL2FjdGl2aXR5L3Bnc3RhdF9yZXBsc2xvdC5jCmluZGV4
IDg4OWU4NmFjNWEuLmRhMTFiODY3NDQgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3V0aWxzL2Fj
dGl2aXR5L3Bnc3RhdF9yZXBsc2xvdC5jCisrKyBiL3NyYy9iYWNrZW5kL3V0aWxzL2FjdGl2aXR5
L3Bnc3RhdF9yZXBsc2xvdC5jCkBAIC0xNTcsOCArMTU3LDkgQEAgcGdzdGF0X2Ryb3BfcmVwbHNs
b3QoUmVwbGljYXRpb25TbG90ICpzbG90KQogewogCUFzc2VydChMV0xvY2tIZWxkQnlNZUluTW9k
ZShSZXBsaWNhdGlvblNsb3RBbGxvY2F0aW9uTG9jaywgTFdfRVhDTFVTSVZFKSk7CgotCXBnc3Rh
dF9kcm9wX2VudHJ5KFBHU1RBVF9LSU5EX1JFUExTTE9ULCBJbnZhbGlkT2lkLAotCQkJCQkgIFJl
cGxpY2F0aW9uU2xvdEluZGV4KHNsb3QpKTsKKwlpZiAoIXBnc3RhdF9kcm9wX2VudHJ5KFBHU1RB
VF9LSU5EX1JFUExTTE9ULCBJbnZhbGlkT2lkLAorCQkJCQkJICAgUmVwbGljYXRpb25TbG90SW5k
ZXgoc2xvdCkpKQorCQlwZ3N0YXRfcmVxdWVzdF9lbnRyeV9yZWZzX2djKCk7CiB9CgogLyoKZGlm
ZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3V0aWxzL2FjdGl2aXR5L3Bnc3RhdF9zaG1lbS5jIGIvc3Jj
L2JhY2tlbmQvdXRpbHMvYWN0aXZpdHkvcGdzdGF0X3NobWVtLmMKaW5kZXggOTE1OTFkYTM5NS4u
YTQ0YWM1NTliMiAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvdXRpbHMvYWN0aXZpdHkvcGdzdGF0
X3NobWVtLmMKKysrIGIvc3JjL2JhY2tlbmQvdXRpbHMvYWN0aXZpdHkvcGdzdGF0X3NobWVtLmMK
QEAgLTg1NSw2ICs4NTUsMTMgQEAgcGdzdGF0X2Ryb3BfZGF0YWJhc2VfYW5kX2NvbnRlbnRzKE9p
ZCBkYm9pZCkKIAkJcGdzdGF0X3JlcXVlc3RfZW50cnlfcmVmc19nYygpOwogfQoKKy8qCisgKiBU
aGlzIGZ1bmN0aW9uIGV4cGVjdHMgY2FsbGVycyB0byBjYWxsIHBnc3RhdF9yZXF1ZXN0X2VudHJ5
X3JlZnNfZ2MgaWYgbmVjZXNzYXJ5LgorICogR2VuZXJhbGx5IHRoaXMgbXVzdCBiZSBjYWxsZWQg
aWYgdGhlIHJldHVybiB2YWx1ZSBpcyBmYWxzZSwgd2hlbiB0aGUgb2JqZWN0CisgKiBjb3VsZG4n
dCBiZSBmcmVlZC4KKyAqIE5vdGUgdGhpcyBpcyBkaWZmZXJlbnQgYmVoYXZpb3IgdGhhbiBvdGhl
ciBkcm9wIGZ1bmN0aW9ucyBsaWtlIHBnc3RhdF9kcm9wX2FsbF9lbnRyaWVzCisgKiB3aGljaCBk
byBjYWxsIHBnc3RhdF9yZXF1ZXN0X2VudHJ5X3JlZnNfZ2MKKyAqLwogYm9vbAogcGdzdGF0X2Ry
b3BfZW50cnkoUGdTdGF0X0tpbmQga2luZCwgT2lkIGRib2lkLCBPaWQgb2Jqb2lkKQogewotLQoy
LjMwLjEKCg==

--_002_dd4c68beba5c40c5a7946569d450ec59Optivercom_--