Re: elog.c query_id support vs shutdown

On Mon, Aug 9, 2021 at 3:06 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> On 2021-08-08 11:53:39 -0700, Andres Freund wrote:
> > On 2021-08-08 13:46:39 +0800, Julien Rouhaud wrote:
> > > > 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...

Agreed, and I'm also looking into it.

> Not a problem with unsetting MyBEEntry. But the search for problems made me
> reread the following comment:
>
> /*
> * There's no need for a lock around pgstat_begin_read_activity /
> * pgstat_end_read_activity here as it's only called from
> * pg_stat_get_activity which is already protected, or from the same
> * backend which means that there won't be concurrent writes.
> */
>
> I don't understand the pg_stat_get_activity() part of this comment?
> pgstat_get_my_query_id() hardcodes MyBEEntry->st_query_id, so it can't be
> useful to pg_stat_get_activity(), nor is it used there?
>
> I assume it's just a remnant from an earlier iteration of the code?

Ah indeed! This was quite a long thread so I didn't try to see when
that changed. I also now realize that I made a typo in the patch
where I s/loop/look/ which was then changed to s/look/lock/. The
comment should be something like:

/*
* There's no need for a loop around pgstat_begin_read_activity /
* pgstat_end_read_activity here as it's only called from the same backend
* which means that there won't be concurrent writes.
*/