On Sat, Mar 25, 2017 at 5:26 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> On Fri, Mar 24, 2017 at 9:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> 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.
Okay, fine for me.
>> - 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.
This one is a good catch.
--
Michael