MIME-Version: 1.0
References: <19520-73873648d44793cf@postgresql.org>
 <CAJTYsWVqtWF+KJUdoKrzUM9QPQA5qD225AoeBeN7cwT4V1qd6A@mail.gmail.com>
 <ajCJw956W3xsl8XP@paquier.xyz> <ajIfUavZvZqep3EQ@paquier.xyz>
 <CAJTYsWWJEchycUwkt6=Hk3j3ROOJKVp2n5yaU7JNqwqOmyoEMA@mail.gmail.com>
In-Reply-To: 
 <CAJTYsWWJEchycUwkt6=Hk3j3ROOJKVp2n5yaU7JNqwqOmyoEMA@mail.gmail.com>
From: Sami Imseih <samimseih@gmail.com>
Date: Wed, 17 Jun 2026 15:26:33 -0500
Message-ID: 
 <CAA5RZ0tuiNhkL30Juxwcox0C6pBCX6H-EVXZoRRQc4X6g6HTxQ@mail.gmail.com>
Subject: Re: BUG #19520: PANIC when concurrently manipulating stored
 procedures with pg_stat_statements and track_functions =
To: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>, zlh21343@163.com,
 pgsql-bugs@lists.postgresql.org,
	Andres Freund <andres@anarazel.de>,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA5RZ0tuiNhkL30Juxwcox0C6pBCX6H-EVXZoRRQc4X6g6HTxQ%40mail.gmail.com>
Precedence: bulk

> This patch is very close to what Sami has posted on his PGSS thread,
> v3-0002, using a missing_ok instead of a skip_dropped:
> https://www.postgresql.org/message-id/CAA5RZ0uoxiQ2_=xHGRnyc4WdM9aR0fzdMhBubnw97po==--yGQ@mail.gmail.com
> I didn't suspect that we would need something like that for a
> backpatch, but well.

Right, my intention was just adding infrastructure to make tolerating
a dropped entry possible and to be used by an extension in the future.
But, this bug report is timely and now it looks like we need this for
race conditions that are possible in core.

> A couple of things which I'm not clear about (these are not blockers
> just questions for my understanding):
>
> - With the check moved into the wrapper, pgstat_drop_entry_internal()
>   still keeps its own "already dropped" elog().  Every path into
>   _internal now seems to guarantee the entry isn't dropped, so
>   _internal's copy looks unreachable after the patch
>   ,and it's the one with the richer refcount/generation detail.

Right. Michael's approach of moving the ERROR into the wrapper is better
than keeping it in _internal. Since after the patch no caller enters
pgstat_drop_entry_internal() with a dropped entry
(pgstat_drop_database_and_contents() and pgstat_drop_matching_entries()
already filtered them out, and now the wrapper does too)

> Was the idea to leave it as a backstop, or would folding the handling into
> one place (or making _internal's an Assert) be cleaner?

The check in _internal should be converted to an Assert. This documents
that callers  must only pass "live" entries, which will be the case
for all callers after
the patch

> - In the missing_ok path the wrapper returns true, so the post-commit
>   caller skips the not_freed_count++/GC request that a "real" not-freed
>   drop would do.  That seems harmless since the entry self-heals
>   but was returning true there a deliberate choice over mirroring
>   the not-freed/false path? I need to take a look again at this, maybe
>   I missed something.

Finding an already dropped entry tells me that the first caller to drop the
entry also triggered a gc request, so we should not request it again.

--
Sami Imseih
Amazon Web Services (AWS)