Re: exposing wait events for non-backends (was: Tracking wait event for latches)

On Fri, Mar 24, 2017 at 9:23 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Mar 23, 2017 at 7:29 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Thu, Mar 23, 2017 at 8:19 PM, Kuntal Ghosh
>> <kuntalghosh(dot)2007(at)gmail(dot)com> wrote:
>>> Hence, to be consistent with others, bgworker processes can be
>>> initialized from BackgroundWorkerInitializeConnectionBy[Oid].
>>
>> Yeah, I am fine with that. Thanks for the updated versions.
>
> I think this is still not good. The places where pgstat_bestart() has
> been added are not even correct. For example, the call added to
> BackgroundWriterMain() occurs after the section that does
> error-recovery, so it would get repeated every time the background
> writer recovers from an error. There are similar problems elsewhere.
> Furthermore, although in theory there's an idea here that we're making
> it no longer the responsibility of InitPostgres() to call
> pgstat_bestart(), the patch as proposed only removes one of the two
> calls, so we really don't even have a consistent practice. I think
> it's better to go with the idea of having InitPostgres() be
> responsible for calling this for regular backends, and
> AuxiliaryProcessMain() for auxiliary backends. That involves
> substantially fewer calls to pgstat_bestart() and they are spread
> across only two functions, which IMHO makes fewer bugs of omission a
> lot less likely.
Agreed. Calling it from InitPostgres() and AuxiliaryProcessMain()
seems correct because of the following two reasons as you've mentioned
up in the thread:
1. security-filtering should be left to some higher-level facility
that can make policy decisions rather than being hard-coded in the
individual modules.
2. makes fewer bugs of omission a lot less likely.

> Updated patch with that change attached. In addition to that
> modification, I made some other alterations:
>
> - I changed the elog() for the can't-happen case in pgstat_bestart()
> from PANIC to FATAL. The contents of shared memory aren't corrupted,
> so I think PANIC is overkill.
Agreed and duly noted for future.

> - I tweaked the comment in WalSndLoop() just before the
> pgstat_report_activity() call to accurately reflect what the effect of
> that call now is.
>
> - I changed the column ordering in pg_stat_get_activity() to put
> backend_type with the other columns that appear in pg_stat_activity,
> rather than (as the patch did) grouping it with the ones that appear
> in pg_stat_ssl.
Thank you.

> - I modified the code to tolerate a NULL return from
> AuxiliaryPidGetProc(). I am pretty sure that without that there's a
> race condition that could lead to a crash if somebody tried to call
> this function just as an auxiliary process was terminating.
Wow. Haven't thought of that. If it's called after
AuxiliaryProcKill(), a crash is evident.

> - I updated the documentation slightly.
Looks good to me.

> - I rebased over some conflicting commits.
Sorry for the inconveniences. It seems that the conflicting changes
occurred within few hours after I've posted the patch.

> If there aren't objections, I plan to commit this version.
Thank you for looking into the patch and doing the necessary changes.
All the changes look good to me and I've tested the feature and it has
passed all the regression tests.

--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com