Re: Issue with bgworker, SPI and pgstat_report_stat

On 07/11/2016 06:53 AM, Craig Ringer wrote:
> On 11 July 2016 at 11:49, Michael Paquier <michael.paquier@gmail.com
> <mailto:michael.paquier@gmail.com>> wrote:
>
>     On Mon, Jul 11, 2016 at 3:36 AM, Julien Rouhaud
>     <julien.rouhaud@dalibo.com <mailto:julien.rouhaud@dalibo.com>> wrote:
>     > I'm not opposed, but in this case we should also provide a proper
>     > documentation of all the required actions to mimick normal backends.
>
>     I'd rather just add a note like "Have a look at PostgresMain if you
>     want to imitate a backend able to run queries!" instead of listing all
>     the actions doable. If low-level things are added or removed in the
>     future in PostgresMain, it is very likely that the documentation will
>     not be updated at the same time, killing its purpose at the end.
>
>
> That sounds like a bug breeding ground. Especially with extensions whose
> bgworkers operate across Pg versions, extensions that get updated
> without re-checking PostgresMain and don't notice some new housekeeping
> task, etc.
>
> Rather than encouraging every extension author to copy and paste random
> chunks of PostgresMain, probably incorrectly, I really think factoring
> the required logic out into something reusable by bgworkers is going to
> be the way to go.
>

I'm not sure I agree with this. Clearly, the fact that worker_spi does 
not invoke pgstat_report_stat() is a bug, but as Michael points out, we 
don't have much insight into what is happening in bgworkers.

Following the changes in PostgresMain() - particularly if your bgworker 
needs to support multiple versions - is difficult, sure. But well, if 
you decided to implement a bgworker operating at such low level, you 
voluntarily accepts that responsibility.

That does not mean we can't make that easier. For example, what about 
extending the bgworker API with

(a) a set of flags (similar to bgw_flags) identifying which maintenance 
tasks the bgworker requests, and

(b) a BackgroundWorkerCleanup() function the bgworker might place at a 
suitable place, invoking all the requested maintenance tasks?

Sure, this may only help for bgworkers that do stuff fairly close to 
regular backends, but maybe that's enough.

In any case, I think adding the pgstat_report_stat() into worker_spi 
seems like a reasonable (and backpatchable) fix.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services