Issue with bgworker, SPI and pgstat_report_stat
Hello,
While investigating on a bloat issue with a colleague, we found that if
a bgworker executes some queries with SPI, the statistic changes will
never be reported, since pgstat_report_stat() is only called in regular
backends.
In our case, the bgworker is the only process inserting and deleting a
large amount of data on some tables, so the autovacuum never tried to do
any maintenance on these tables.
Should a bgworker modifing data have to call pgstat_report_stat() to
avoid this problem? I don't find any documentation suggesting it, and it
seems that worker_spi (used as a template for this bgworker and I
suppose a lot of other one) is also affected.
If yes, I think at least worker_spi should be fixed (patched attached).
Regards.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
Attachments:
worker_spi_report_stat-v1.difftext/x-diff; name=worker_spi_report_stat-v1.diffDownload
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index 314e371..7c9a3eb 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -292,6 +292,7 @@ worker_spi_main(Datum main_arg)
SPI_finish();
PopActiveSnapshot();
CommitTransactionCommand();
+ pgstat_report_stat(false);
pgstat_report_activity(STATE_IDLE, NULL);
}
On Thu, Jul 7, 2016 at 1:52 PM, Julien Rouhaud
<julien.rouhaud@dalibo.com> wrote:
While investigating on a bloat issue with a colleague, we found that if
a bgworker executes some queries with SPI, the statistic changes will
never be reported, since pgstat_report_stat() is only called in regular
backends.In our case, the bgworker is the only process inserting and deleting a
large amount of data on some tables, so the autovacuum never tried to do
any maintenance on these tables.
Ouch.
Should a bgworker modifing data have to call pgstat_report_stat() to
avoid this problem? I don't find any documentation suggesting it, and it
seems that worker_spi (used as a template for this bgworker and I
suppose a lot of other one) is also affected.
That certainly seems like the simplest fix. Not sure if there's a better one.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-07-07 14:04:36 -0400, Robert Haas wrote:
On Thu, Jul 7, 2016 at 1:52 PM, Julien Rouhaud
<julien.rouhaud@dalibo.com> wrote:Should a bgworker modifing data have to call pgstat_report_stat() to
avoid this problem? I don't find any documentation suggesting it, and it
seems that worker_spi (used as a template for this bgworker and I
suppose a lot of other one) is also affected.That certainly seems like the simplest fix. Not sure if there's a better one.
I think a better fix would be to unify the startup & error handling
code. We have way to many slightly diverging copies. But that's a bigger
task, so I'm not protesting against making a more localized fix.
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jul 8, 2016 at 3:06 AM, Andres Freund <andres@anarazel.de> wrote:
On 2016-07-07 14:04:36 -0400, Robert Haas wrote:
On Thu, Jul 7, 2016 at 1:52 PM, Julien Rouhaud
<julien.rouhaud@dalibo.com> wrote:Should a bgworker modifing data have to call pgstat_report_stat() to
avoid this problem? I don't find any documentation suggesting it, and it
seems that worker_spi (used as a template for this bgworker and I
suppose a lot of other one) is also affected.That certainly seems like the simplest fix. Not sure if there's a better one.
I think a better fix would be to unify the startup & error handling
code. We have way to many slightly diverging copies. But that's a bigger
task, so I'm not protesting against making a more localized fix.
It seems to me that the only fix is to have the bgworker call
pgstat_report_stat() and not mess up with the in-core backend code.
Personally, I always had the image of a bgworker to be an independent
process, so when it wants to do something, be it mimicking normal
backends, it should do it by itself. Take the example of SIGHUP
processing. If the bgworker does not ProcessConfigFile() no parameters
updates will happen in the context of the bgworker.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 08/07/2016 01:53, Michael Paquier wrote:
On Fri, Jul 8, 2016 at 3:06 AM, Andres Freund <andres@anarazel.de> wrote:
On 2016-07-07 14:04:36 -0400, Robert Haas wrote:
On Thu, Jul 7, 2016 at 1:52 PM, Julien Rouhaud
<julien.rouhaud@dalibo.com> wrote:Should a bgworker modifing data have to call pgstat_report_stat() to
avoid this problem? I don't find any documentation suggesting it, and it
seems that worker_spi (used as a template for this bgworker and I
suppose a lot of other one) is also affected.That certainly seems like the simplest fix. Not sure if there's a better one.
I think a better fix would be to unify the startup & error handling
code. We have way to many slightly diverging copies. But that's a bigger
task, so I'm not protesting against making a more localized fix.It seems to me that the only fix is to have the bgworker call
pgstat_report_stat() and not mess up with the in-core backend code.
Personally, I always had the image of a bgworker to be an independent
process, so when it wants to do something, be it mimicking normal
backends, it should do it by itself. Take the example of SIGHUP
processing. If the bgworker does not ProcessConfigFile() no parameters
updates will happen in the context of the bgworker.
I'm not opposed, but in this case we should also provide a proper
documentation of all the required actions to mimick normal backends.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jul 11, 2016 at 3:36 AM, Julien Rouhaud
<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.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 11 July 2016 at 11:49, Michael Paquier <michael.paquier@gmail.com> wrote:
On Mon, Jul 11, 2016 at 3:36 AM, Julien Rouhaud
<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.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Sep 3, 2016 at 10:02 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
In any case, I think adding the pgstat_report_stat() into worker_spi seems
like a reasonable (and backpatchable) fix.
Doing just that sounds reasonable seen from here. I am wondering also
if it would not be worth mentioning in the documentation of the
bgworkers that users trying to emulate somewhat the behavior of a
backend should look at PostgresMain(). The code in itself is full of
hints as well.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Sep 3, 2016 at 12:29 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Sat, Sep 3, 2016 at 10:02 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:In any case, I think adding the pgstat_report_stat() into worker_spi seems
like a reasonable (and backpatchable) fix.Doing just that sounds reasonable seen from here. I am wondering also
if it would not be worth mentioning in the documentation of the
bgworkers that users trying to emulate somewhat the behavior of a
backend should look at PostgresMain(). The code in itself is full of
hints as well.
Everybody seems happy with this fix for a first step, so I've
committed it and back-patched it to 9.3.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 28/09/2016 18:46, Robert Haas wrote:
Everybody seems happy with this fix for a first step, so I've
committed it and back-patched it to 9.3.
Thanks!
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers