On Tue, Mar 21, 2017 at 10:52 AM, Michael Paquier <michael.paqu...@gmail.com> wrote: Thank you for the review.
> Unfortunately this is true only for background workers that connect to
> a database. And this would break for bgworkers that do not do that.
> The point to fix is here:
> + if (MyBackendId != InvalidBackendId)
> + {
> [...]
> + else if (IsBackgroundWorker)
> + {
> + /* bgworker */
> + beentry->st_backendType = B_BG_WORKER;
> + }
> Your code is assuming that a bgworker will always be setting
> MyBackendId which is not necessarily true, and you would trigger the
> assertion down:
> Assert(MyAuxProcType != NotAnAuxProcess);
> So you need to rely on IsBackgroundWorker in priority I think. I would
> suggest as well to give up calling pgstat_bestart() in
> BackgroundWorkerInitializeConnection[ByOid] and let the workers do
> this work by themselves. This gives more flexibility. You would need
> to update the logical replication worker and worker_spi for that as
> well.
We reserve a slot for each possible BackendId, plus one for each
possible auxiliary process type. For a non-auxiliary process,
BackendId is used to refer the backend status in PgBackendStatus
array. So, a bgworker without any BackendId can't initialize its'
entry in PgBackendStatus array. In simple terms, it will not be shown
in pg_stat_activity. I've added some comments regarding this in
pgstat_bestart().
And, any bgworker having valid BackendId will have either a valid
userid or BOOTSTRAP_SUPERUSERID.
> If you want to test this configuration, feel free to use this background
> worker:
> https://github.com/michaelpq/pg_plugins/tree/master/hello_world
> This just prints an entry to the logs every 10s, and does not connect
> to any database. Adding a call to pgstat_bestart() in hello_main
> triggers the assertion.
>
In this case, pgstat_bestart() shouldn't be called from this module as
the spawned bgworker will have invalid BackendId. By the way, thanks
for sharing the repo link. Found a lot of interesting things to
explore and learn. :)
>>> @@ -808,6 +836,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
>>> nulls[12] = true;
>>> nulls[13] = true;
>>> nulls[14] = true;
>>> + nulls[23] = true;
>>> }
>>> That's not possible to have backend_type set as NULL, no?
>>
>> Yes, backend_type can't be null. But, I'm not sure whether it should
>> be visible to a user with insufficient privileges. Anyway, I've made
>> it visible to all user for now.
>>
>> Please find the updated patches in the attachment.
>
> Yeah, hiding it may make sense...
Modified.
> /* The autovacuum launcher is done here */
> if (IsAutoVacuumLauncherProcess())
> + {
> return;
> + }
> Unnecessary noise here.
>
Fixed.
> Except for the two issues pointed out in this email, I am pretty cool
> with this patch. Nice work.
Thank you. :)
Please find the updated patches.
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com
0001-Infra-to-expose-all-backend-processes-in-pg_stat_get.patch
Description: application/download
0002-Expose-stats-for-all-backends.patch
Description: application/download
0003-Add-backend_type-column-in-pg_stat_get_activity.patch
Description: application/download
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
