Re: elog.c query_id support vs shutdown

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(at)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