Time based lag tracking for logical replication

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com 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]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com

--
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, 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] /messages/by-id/20170404140717.GA2675809@tornado.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:
/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com

--
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, 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] /messages/by-id/20170404140717.GA2675809@tornado.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:
/messages/by-id/20170404140717.GA2675809@tornado.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]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com 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]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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?

--
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 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

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

--
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 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

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 push today. I'd also like to take a look before...

Thanks,

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

--
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 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 push today. I'd also like to take a look before...

Sure.

--
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

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 push 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.

Cheers

--
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

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 to push 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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 to push 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

--
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, 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