Hi, On 2021-08-08 13:46:39 +0800, Julien Rouhaud wrote: > On Sat, Aug 07, 2021 at 04:44:07PM -0700, Andres Freund wrote: > > > > As currently implemented those pgstat_get_my_query_id() calls are not > > safe. It's fine during backend startup because MyBEEntry is not set, but > > during shutdown that's not ok, because we never unset MyBEEntry. > > > > andres@awork3:~/src/postgresql$ > > /home/andres/build/postgres/dev-assert/vpath/src/backend/postgres --single > > postgres -D /srv/dev/pgdev-dev/ -c 'log_line_prefix=%Q' -c > > log_min_messages=debug1 > > [...] > > PostgreSQL stand-alone backend 15devel > > backend> 0NOTICE: shutting down > > 0DEBUG: performing replication slot checkpoint > > Segmentation fault > > Ouch > > > I suspect that to make the elog.c usage safe, we'll have to clear MyBEEntry > > in > > pgstat_beshutdown_hook(). > > I agree, and a quick test indeed fix your scenario. It also seems like a good > thing to do overall.
Yea, it does seem like a good thing. But we should do a search for the problems it could cause... > I didn't find any other problematic corner cases, but I'm not that familiar > with pgstat, especially after the recent activity. I don't think anything relevant to this issue has changed so far... And there shouldn't be a meaningful amount of change to backend_status.c anyway - the "what is currently happening" stuff that backend_status.c implements is mostly independent from the "what has happened so far" that pgstats.c implements. It probably would be a good idea to separate out the two namespaces more clearly. Even with things like pgstat_report_activity() not being entirely clear cut (because of the pgstat_count_conn_* calls) it still seems like it would be an improvement. But I don't want to do that before the shared memory stuff is in. Greetings, Andres Freund
