Thread: Re: [HACKERS] logical replication worker and statistics
On Wed, Apr 05, 2017 at 05:02:18PM +0300, Stas Kelvich wrote: > > On 27 Mar 2017, at 18:59, Robert Haas <robertmhaas@gmail.com> wrote: > > On Mon, Mar 27, 2017 at 11:14 AM, Fujii Masao <masao.fujii@gmail.com> wrote: > >> Logical replication worker should call pgstat_report_stat()? > >> Currently it doesn't seem to do that and no statistics about > >> table accesses by logical replication workers are collected. > >> For example, this can prevent autovacuum from working on > >> those tables properly. > > > > Yeah, that doesn't sound good. > > Seems that nobody is working on this, so i’m going to create the patch. [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Peter, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> On 10 Apr 2017, at 05:20, Noah Misch <noah@leadboat.com> wrote: > > On Wed, Apr 05, 2017 at 05:02:18PM +0300, Stas Kelvich wrote: >>> On 27 Mar 2017, at 18:59, Robert Haas <robertmhaas@gmail.com> wrote: >>> On Mon, Mar 27, 2017 at 11:14 AM, Fujii Masao <masao.fujii@gmail.com> wrote: >>>> Logical replication worker should call pgstat_report_stat()? >>>> Currently it doesn't seem to do that and no statistics about >>>> table accesses by logical replication workers are collected. >>>> For example, this can prevent autovacuum from working on >>>> those tables properly. >>> >>> Yeah, that doesn't sound good. >> >> Seems that nobody is working on this, so i’m going to create the patch. > > [Action required within three days. This is a generic notification.] > > The above-described topic is currently a PostgreSQL 10 open item. Peter, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > v10 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within three calendar days of > this message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping v10. Consequently, I will appreciate your efforts > toward speedy resolution. Thanks. > > [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com Here is small patch to call statistics in logical worker. Originally i thought that stat collection during logical replication should manually account amounts of changed tuples, but seems that it is already smoothly handled on relation level. So call to pgstat_report_stat() is enough. Also i’ve added statistics checks to logrep tap tests, but that is probably quite fragile without something like wait_for_stats() from regression test stats.sql. Stas Kelvich Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 4/10/17 05:49, Stas Kelvich wrote: > Here is small patch to call statistics in logical worker. Originally i thought that stat > collection during logical replication should manually account amounts of changed tuples, > but seems that it is already smoothly handled on relation level. So call to > pgstat_report_stat() is enough. I wonder whether we need a similar call somewhere in tablesync.c. It seems to work without it, though. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 4/9/17 22:20, Noah Misch wrote: > The above-described topic is currently a PostgreSQL 10 open item. Peter, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > v10 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within three calendar days of > this message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping v10. Consequently, I will appreciate your efforts > toward speedy resolution. Thanks. Discussion is ongoing, patch is proposed, should be resolved this week. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On 10 Apr 2017, at 19:50, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > On 4/10/17 05:49, Stas Kelvich wrote: >> Here is small patch to call statistics in logical worker. Originally i thought that stat >> collection during logical replication should manually account amounts of changed tuples, >> but seems that it is already smoothly handled on relation level. So call to >> pgstat_report_stat() is enough. > > I wonder whether we need a similar call somewhere in tablesync.c. It > seems to work without it, though. I thought it spins up the same worker from worker.c. Stas Kelvich Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
On 4/10/17 13:06, Stas Kelvich wrote: > >> On 10 Apr 2017, at 19:50, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: >> >> On 4/10/17 05:49, Stas Kelvich wrote: >>> Here is small patch to call statistics in logical worker. Originally i thought that stat >>> collection during logical replication should manually account amounts of changed tuples, >>> but seems that it is already smoothly handled on relation level. So call to >>> pgstat_report_stat() is enough. >> >> I wonder whether we need a similar call somewhere in tablesync.c. It >> seems to work without it, though. > > I thought it spins up the same worker from worker.c. It depends on which of the various tablesync scenarios happens. If the apply lags behind the tablesync, then the apply doesn't process commit messages until it has caught up. So the part of the code you patched wouldn't be called. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 12/04/17 05:41, Peter Eisentraut wrote: > On 4/10/17 13:06, Stas Kelvich wrote: >> >>> On 10 Apr 2017, at 19:50, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: >>> >>> On 4/10/17 05:49, Stas Kelvich wrote: >>>> Here is small patch to call statistics in logical worker. Originally i thought that stat >>>> collection during logical replication should manually account amounts of changed tuples, >>>> but seems that it is already smoothly handled on relation level. So call to >>>> pgstat_report_stat() is enough. >>> >>> I wonder whether we need a similar call somewhere in tablesync.c. It >>> seems to work without it, though. >> >> I thought it spins up the same worker from worker.c. > > It depends on which of the various tablesync scenarios happens. If the > apply lags behind the tablesync, then the apply doesn't process commit > messages until it has caught up. So the part of the code you patched > wouldn't be called. > Indeed, if catchup phase didn't happen (because tablesync was faster than apply) then the commit handler is never called so all the changes made by copy would be forgotten. Also the tablesync worker might exit from process_syncing_tables() call which is called before we report stats in the commit handler so again some changes might be forgotten. I attached modified version of the patch that also reports stats in finish_sync_worker() when there is outstanding transaction. The test can stay the same. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Attachment
On 4/14/17 12:09, Petr Jelinek wrote: > Indeed, if catchup phase didn't happen (because tablesync was faster > than apply) then the commit handler is never called so all the changes > made by copy would be forgotten. Also the tablesync worker might exit > from process_syncing_tables() call which is called before we report > stats in the commit handler so again some changes might be forgotten. > > I attached modified version of the patch that also reports stats in > finish_sync_worker() when there is outstanding transaction. The test can > stay the same. committed (without the tests) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services