On Thu, Mar 23, 2017 at 7:29 AM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Thu, Mar 23, 2017 at 8:19 PM, Kuntal Ghosh > <kuntalghosh.2...@gmail.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. 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. - 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. - 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. - I updated the documentation slightly. - I rebased over some conflicting commits. If there aren't objections, I plan to commit this version. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
more-pg-stat-activity.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
