Thread: [HACKERS] Time based lag tracking for logical replication
Hi, The time based lag tracking commit [1] added interface for logging progress of replication so that we can report lag as time interval instead of just bytes. But the patch didn't contain patch for the builtin logical replication. So I wrote something that implements this. I didn't like all that much the API layering in terms of exporting the walsender's LagTrackerWrite() for use by plugin directly. Normally output plugin does not have to care if it's running under walsender or not, it uses abstracted write interface for that which can be implemented in various ways (that's how we implement SQL interface to logical decoding after all). So I decided to add another function to the logical decoding write api called update_progress and call that one from the output plugin. The walsender then implements that new API to call the LagTrackerWrite() while the SQL interface just does not implement it at all. This seems like cleaner way of doing it. Thoughts? -- 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 23/04/17 01:10, Petr Jelinek wrote: > Hi, > > The time based lag tracking commit [1] added interface for logging > progress of replication so that we can report lag as time interval > instead of just bytes. But the patch didn't contain patch for the > builtin logical replication. > > So I wrote something that implements this. I didn't like all that much > the API layering in terms of exporting the walsender's LagTrackerWrite() > for use by plugin directly. Normally output plugin does not have to care > if it's running under walsender or not, it uses abstracted write > interface for that which can be implemented in various ways (that's how > we implement SQL interface to logical decoding after all). So I decided > to add another function to the logical decoding write api called > update_progress and call that one from the output plugin. The walsender > then implements that new API to call the LagTrackerWrite() while the SQL > interface just does not implement it at all. This seems like cleaner way > of doing it. > The original no longer applies after Andres committed some of my fixes for snapshot builder so here is rebased version. -- 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 23 April 2017 at 01:10, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote: > Hi, > > The time based lag tracking commit [1] added interface for logging > progress of replication so that we can report lag as time interval > instead of just bytes. But the patch didn't contain patch for the > builtin logical replication. > > So I wrote something that implements this. I didn't like all that much > the API layering in terms of exporting the walsender's LagTrackerWrite() > for use by plugin directly. Normally output plugin does not have to care > if it's running under walsender or not, it uses abstracted write > interface for that which can be implemented in various ways (that's how > we implement SQL interface to logical decoding after all). So I decided > to add another function to the logical decoding write api called > update_progress and call that one from the output plugin. The walsender > then implements that new API to call the LagTrackerWrite() while the SQL > interface just does not implement it at all. This seems like cleaner way > of doing it. > > Thoughts? Agree cleaner. I don't see any pacing or comments about it, nor handling of intermediate messages while we process a large transaction. I'll look at adding some pacing code in WalSndUpdateProgress() -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, May 3, 2017 at 6:28 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 23 April 2017 at 01:10, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote: >> Hi, >> >> The time based lag tracking commit [1] added interface for logging >> progress of replication so that we can report lag as time interval >> instead of just bytes. But the patch didn't contain patch for the >> builtin logical replication. >> >> So I wrote something that implements this. I didn't like all that much >> the API layering in terms of exporting the walsender's LagTrackerWrite() >> for use by plugin directly. Normally output plugin does not have to care >> if it's running under walsender or not, it uses abstracted write >> interface for that which can be implemented in various ways (that's how >> we implement SQL interface to logical decoding after all). So I decided >> to add another function to the logical decoding write api called >> update_progress and call that one from the output plugin. The walsender >> then implements that new API to call the LagTrackerWrite() while the SQL >> interface just does not implement it at all. This seems like cleaner way >> of doing it. >> >> Thoughts? > > Agree cleaner. +1 > I don't see any pacing or comments about it, nor handling of > intermediate messages while we process a large transaction. > > I'll look at adding some pacing code in WalSndUpdateProgress() By the way, I have a small improvement to the interpolation to propose. Right now, after a period of idleness it can report a silly large number based on an ancient time, but you won't usually see it because it's quickly replaced by a sensible number. I think this thinko will affect logical rep with Petr's patch more. I had been meaning to post the improvement but got sidetracked by that recovery test failure problem. I'll post that in the next few days. -- Thomas Munro http://www.enterprisedb.com
On 03/05/17 08:28, Simon Riggs wrote: > On 23 April 2017 at 01:10, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote: >> Hi, >> >> The time based lag tracking commit [1] added interface for logging >> progress of replication so that we can report lag as time interval >> instead of just bytes. But the patch didn't contain patch for the >> builtin logical replication. >> >> So I wrote something that implements this. I didn't like all that much >> the API layering in terms of exporting the walsender's LagTrackerWrite() >> for use by plugin directly. Normally output plugin does not have to care >> if it's running under walsender or not, it uses abstracted write >> interface for that which can be implemented in various ways (that's how >> we implement SQL interface to logical decoding after all). So I decided >> to add another function to the logical decoding write api called >> update_progress and call that one from the output plugin. The walsender >> then implements that new API to call the LagTrackerWrite() while the SQL >> interface just does not implement it at all. This seems like cleaner way >> of doing it. >> >> Thoughts? > > Agree cleaner. > > I don't see any pacing or comments about it, nor handling of > intermediate messages while we process a large transaction. Agreed, pacing is good idea because on busy server storing info for every commit could get expensive. Don't understand what you mean by "handling of intermediate messages while we process a large transaction". Logical replication is transaction based so far, it does not stream like physical replication so it seems like there is limited usefulness in doing this outside of commit no? -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Wed, May 03, 2017 at 08:28:53AM +0200, Simon Riggs wrote: > On 23 April 2017 at 01:10, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote: > > Hi, > > > > The time based lag tracking commit [1] added interface for logging > > progress of replication so that we can report lag as time interval > > instead of just bytes. But the patch didn't contain patch for the > > builtin logical replication. > > > > So I wrote something that implements this. I didn't like all that much > > the API layering in terms of exporting the walsender's LagTrackerWrite() > > for use by plugin directly. Normally output plugin does not have to care > > if it's running under walsender or not, it uses abstracted write > > interface for that which can be implemented in various ways (that's how > > we implement SQL interface to logical decoding after all). So I decided > > to add another function to the logical decoding write api called > > update_progress and call that one from the output plugin. The walsender > > then implements that new API to call the LagTrackerWrite() while the SQL > > interface just does not implement it at all. This seems like cleaner way > > of doing it. > > > > Thoughts? > > Agree cleaner. > > I don't see any pacing or comments about it, nor handling of > intermediate messages while we process a large transaction. > > I'll look at adding some pacing code in WalSndUpdateProgress() [Action required within three days.] Simon, I understand, from an annotation on the open items list, that you have volunteered to own this item. 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 Fri, May 05, 2017 at 07:07:26AM +0000, Noah Misch wrote: > On Wed, May 03, 2017 at 08:28:53AM +0200, Simon Riggs wrote: > > On 23 April 2017 at 01:10, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote: > > > Hi, > > > > > > The time based lag tracking commit [1] added interface for logging > > > progress of replication so that we can report lag as time interval > > > instead of just bytes. But the patch didn't contain patch for the > > > builtin logical replication. > > > > > > So I wrote something that implements this. I didn't like all that much > > > the API layering in terms of exporting the walsender's LagTrackerWrite() > > > for use by plugin directly. Normally output plugin does not have to care > > > if it's running under walsender or not, it uses abstracted write > > > interface for that which can be implemented in various ways (that's how > > > we implement SQL interface to logical decoding after all). So I decided > > > to add another function to the logical decoding write api called > > > update_progress and call that one from the output plugin. The walsender > > > then implements that new API to call the LagTrackerWrite() while the SQL > > > interface just does not implement it at all. This seems like cleaner way > > > of doing it. > > > > > > Thoughts? > > > > Agree cleaner. > > > > I don't see any pacing or comments about it, nor handling of > > intermediate messages while we process a large transaction. > > > > I'll look at adding some pacing code in WalSndUpdateProgress() > > [Action required within three days.] > > Simon, I understand, from an annotation on the open items list, that you have > volunteered to own this item. 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 This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On Mon, May 08, 2017 at 10:30:45PM -0700, Noah Misch wrote: > On Fri, May 05, 2017 at 07:07:26AM +0000, Noah Misch wrote: > > On Wed, May 03, 2017 at 08:28:53AM +0200, Simon Riggs wrote: > > > On 23 April 2017 at 01:10, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote: > > > > Hi, > > > > > > > > The time based lag tracking commit [1] added interface for logging > > > > progress of replication so that we can report lag as time interval > > > > instead of just bytes. But the patch didn't contain patch for the > > > > builtin logical replication. > > > > > > > > So I wrote something that implements this. I didn't like all that much > > > > the API layering in terms of exporting the walsender's LagTrackerWrite() > > > > for use by plugin directly. Normally output plugin does not have to care > > > > if it's running under walsender or not, it uses abstracted write > > > > interface for that which can be implemented in various ways (that's how > > > > we implement SQL interface to logical decoding after all). So I decided > > > > to add another function to the logical decoding write api called > > > > update_progress and call that one from the output plugin. The walsender > > > > then implements that new API to call the LagTrackerWrite() while the SQL > > > > interface just does not implement it at all. This seems like cleaner way > > > > of doing it. > > > > > > > > Thoughts? > > > > > > Agree cleaner. > > > > > > I don't see any pacing or comments about it, nor handling of > > > intermediate messages while we process a large transaction. > > > > > > I'll look at adding some pacing code in WalSndUpdateProgress() > > > > [Action required within three days.] > > > > Simon, I understand, from an annotation on the open items list, that you have > > volunteered to own this item. 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 > > This PostgreSQL 10 open item is past due for your status update. Kindly send > a status update within 24 hours, and include a date for your subsequent status > update. Refer to the policy on open item ownership: > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com IMMEDIATE ATTENTION REQUIRED. This PostgreSQL 10 open item is long past due for your status update. Please reacquaint yourself with the policy on open item ownership[1] and then reply immediately. If I do not hear from you by 2017-05-11 06:00 UTC, I will transfer this item to release management team ownership without further notice. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On Sun, Apr 23, 2017 at 01:10:32AM +0200, Petr Jelinek wrote: > The time based lag tracking commit [1] added interface for logging > progress of replication so that we can report lag as time interval > instead of just bytes. But the patch didn't contain patch for the > builtin logical replication. > > So I wrote something that implements this. This is listed as a PostgreSQL 10 open item, but the above makes it sound like a feature to consider for v11, not a defect in v10. Why is this an open item?
On 11 May 2017 at 08:32, Noah Misch <noah@leadboat.com> wrote: > On Sun, Apr 23, 2017 at 01:10:32AM +0200, Petr Jelinek wrote: >> The time based lag tracking commit [1] added interface for logging >> progress of replication so that we can report lag as time interval >> instead of just bytes. But the patch didn't contain patch for the >> builtin logical replication. >> >> So I wrote something that implements this. > > This is listed as a PostgreSQL 10 open item, but the above makes it sound like > a feature to consider for v11, not a defect in v10. Why is this an open item? It's an open item because at the time of the Lag Tracker commit it was believed to be a single line of code that needed to be added to Logical Replication to make it work with the Lag Tracker functionality. It didn't make sense for me to add it while the LR code was still being changed, even though we had code to do that. Petr's new patch is slightly longer and needed review and some minor code to add pacing delay. My own delay in responding has been because of illness. You'll note that I'd missed response on at least one other mail from you. Apologies for that, it has set me back some way but I'm better now and have caught up with other matters. Petr nudged me to look at this thread again yesterday, so I had been looking at this over last few days. Attached patch is Petr's patch, slightly rebased with added pacing delay, similar to that used by HSFeedback. -- Simon Riggs 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
Attachment
On 11/05/17 15:01, Simon Riggs wrote: > On 11 May 2017 at 08:32, Noah Misch <noah@leadboat.com> wrote: >> On Sun, Apr 23, 2017 at 01:10:32AM +0200, Petr Jelinek wrote: >>> The time based lag tracking commit [1] added interface for logging >>> progress of replication so that we can report lag as time interval >>> instead of just bytes. But the patch didn't contain patch for the >>> builtin logical replication. >>> >>> So I wrote something that implements this. >> >> This is listed as a PostgreSQL 10 open item, but the above makes it sound like >> a feature to consider for v11, not a defect in v10. Why is this an open item? > > It's an open item because at the time of the Lag Tracker commit it was > believed to be a single line of code that needed to be added to > Logical Replication to make it work with the Lag Tracker > functionality. It didn't make sense for me to add it while the LR code > was still being changed, even though we had code to do that. > > Petr's new patch is slightly longer and needed review and some minor > code to add pacing delay. > > My own delay in responding has been because of illness. You'll note > that I'd missed response on at least one other mail from you. > Apologies for that, it has set me back some way but I'm better now and > have caught up with other matters. Petr nudged me to look at this > thread again yesterday, so I had been looking at this over last few > days. > > Attached patch is Petr's patch, slightly rebased with added pacing > delay, similar to that used by HSFeedback. > This looks reasonable. I would perhaps change: > + /* > + * Track lag no more than once per WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS > + */ to something like this for extra clarity: > + /* > + * Track lag no more than once per WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS > + * to avoid flooding the lag tracker on busy servers. > + */ -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 11 May 2017 at 14:12, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote: >> Attached patch is Petr's patch, slightly rebased with added pacing >> delay, similar to that used by HSFeedback. >> > > This looks reasonable. I would perhaps change: >> + /* >> + * Track lag no more than once per WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS >> + */ > > to something like this for extra clarity: >> + /* >> + * Track lag no more than once per WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS >> + * to avoid flooding the lag tracker on busy servers. >> + */ New patch, v3. Applying in 90 minutes, barring objections. -- Simon Riggs 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
Attachment
On May 11, 2017 8:08:11 AM PDT, Simon Riggs <simon@2ndquadrant.com> wrote: >On 11 May 2017 at 14:12, Petr Jelinek <petr.jelinek@2ndquadrant.com> >wrote: > >>> Attached patch is Petr's patch, slightly rebased with added pacing >>> delay, similar to that used by HSFeedback. >>> >> >> This looks reasonable. I would perhaps change: >>> + /* >>> + * Track lag no more than once per >WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS >>> + */ >> >> to something like this for extra clarity: >>> + /* >>> + * Track lag no more than once per >WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS >>> + * to avoid flooding the lag tracker on busy servers. >>> + */ > >New patch, v3. > >Applying in 90 minutes, barring objections. Could you please wait till tomorrow? I've bigger pending fixes for related code pending/being tested that I plan to pushtoday. I'd also like to take a look before... Thanks, Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
On 11 May 2017 at 18:13, Andres Freund <andres@anarazel.de> wrote: >>New patch, v3. >> >>Applying in 90 minutes, barring objections. > > Could you please wait till tomorrow? I've bigger pending fixes for related code pending/being tested that I plan to pushtoday. I'd also like to take a look before... Sure. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 11 May 2017 at 18:29, Simon Riggs <simon@2ndquadrant.com> wrote: > On 11 May 2017 at 18:13, Andres Freund <andres@anarazel.de> wrote: > >>>New patch, v3. >>> >>>Applying in 90 minutes, barring objections. >> >> Could you please wait till tomorrow? I've bigger pending fixes for related code pending/being tested that I plan to pushtoday. I'd also like to take a look before... > > Sure. The changes I've added are very minor, so I'm not expecting debate. The main part of the patch is the same as Petr posted 19days ago. I'm travelling now, so after waiting till tomorrow as you requested I have committed the patch. Cheers -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, May 12, 2017 at 8:19 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > > On 11 May 2017 at 18:29, Simon Riggs <simon@2ndquadrant.com> wrote: > > On 11 May 2017 at 18:13, Andres Freund <andres@anarazel.de> wrote: > > > >>>New patch, v3. > >>> > >>>Applying in 90 minutes, barring objections. > >> > >> Could you please wait till tomorrow? I've bigger pending fixes for related code pending/being tested that I plan topush today. I'd also like to take a look before... > > > > Sure. > > The changes I've added are very minor, so I'm not expecting debate. > The main part of the patch is the same as Petr posted 19days ago. > > I'm travelling now, so after waiting till tomorrow as you requested I > have committed the patch. > Prior to this commit CREATE SUBSCRIPTION used to work smoothly. After this commit 024711bb544645c8b1061e9f02b261e2e336981d I get following error while executing CREATE SUBSCRIPTION: CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=postgres host=localhost user=neha port=5432' PUBLICATION mypub; NOTICE: synchronized table states ERROR: could not create replication slot "sub1": ERROR: could not load library "/home/neha/postgres/PGCurrentInstall/lib/pgoutput.so": /home/neha/postgres/PGCurrentInstall/lib/pgoutput.so: undefined symbol: OutputPluginUpdateProgress Regards Neha
On 12/05/17 15:09, Neha Khatri wrote: > On Fri, May 12, 2017 at 8:19 PM, Simon Riggs <simon@2ndquadrant.com> wrote: >> >> On 11 May 2017 at 18:29, Simon Riggs <simon@2ndquadrant.com> wrote: >>> On 11 May 2017 at 18:13, Andres Freund <andres@anarazel.de> wrote: >>> >>>>> New patch, v3. >>>>> >>>>> Applying in 90 minutes, barring objections. >>>> >>>> Could you please wait till tomorrow? I've bigger pending fixes for related code pending/being tested that I plan topush today. I'd also like to take a look before... >>> >>> Sure. >> >> The changes I've added are very minor, so I'm not expecting debate. >> The main part of the patch is the same as Petr posted 19days ago. >> >> I'm travelling now, so after waiting till tomorrow as you requested I >> have committed the patch. >> > > Prior to this commit CREATE SUBSCRIPTION used to work smoothly. > > After this commit 024711bb544645c8b1061e9f02b261e2e336981d I get > following error while executing CREATE SUBSCRIPTION: > > CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=postgres host=localhost > user=neha port=5432' PUBLICATION mypub; > NOTICE: synchronized table states > ERROR: could not create replication slot "sub1": ERROR: could not > load library "/home/neha/postgres/PGCurrentInstall/lib/pgoutput.so": > /home/neha/postgres/PGCurrentInstall/lib/pgoutput.so: undefined > symbol: OutputPluginUpdateProgress > Hmm, that sounds like partial rebuild/install (old postgres binary with new pgoutput one). -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Sat, May 13, 2017 at 12:04 AM, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:
> After this commit 024711bb544645c8b1061e9f02b261e2e336981d I get
> following error while executing CREATE SUBSCRIPTION:
>
> CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=postgres host=localhost
> user=neha port=5432' PUBLICATION mypub;
> NOTICE: synchronized table states
> ERROR: could not create replication slot "sub1": ERROR: could not
> load library "/home/neha/postgres/PGCurrentInstall/lib/pgoutput. so":
> /home/neha/postgres/PGCurrentInstall/lib/pgoutput. so: undefined
> symbol: OutputPluginUpdateProgress
>
Hmm, that sounds like partial rebuild/install (old postgres binary with
new pgoutput one).
That's right. Thanks.
Neha