On Mon, Aug 9, 2021 at 3:06 AM Andres Freund <and...@anarazel.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. */
