Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Smart replication feedback #913

Merged
merged 4 commits into from May 12, 2019

Conversation

Projects
None yet
4 participants
@CyberDem0n
Copy link
Contributor

commented May 6, 2019

This commit makes psycopg2 responsible for sending the status update (feedback) messages to the server regardless of whether a synchronous or asynchronous connection is used.

Feedback is sent every status_interval (default value is 10) seconds, which could be configured by passing a corresponding parameter to the start_replication() or start_replication_expert() methods.
The actual feedback message is sent by the pq_read_replication_message() when the status_interval timeout is reached.

The default behavior of the send_feedback() method is changed. It doesn't send a feedback message on every call anymore but just updates internal structures. There is still a way to force sending a message if force or reply parameters are set.

The new approach has certain advantages:

  1. The client can simply call the send_feedback() for every processed message and the library will take care of not overwhelming the server. Actually, in the synchronous mode it is even mandatory to confirm every processed message.
  2. The library tracks internally the pointer of the last received message which is not keepalive. If the client confirmed the last message and after that server sends only keepalives with increasing wal_end, the library can safely move forward flush position to the wal_end and later automatically report it to the server.

Reporting of the wal_end received from keepalive messages is very important. Not doing so casing:

  1. Excessive disk usage, because the replication slot prevents from WAL being cleaned up.
  2. The smart and fast shutdown of the server could last indefinitely because walsender waits until the client report flush position equal to the wal_end.

The new way of sending status update (feedback) messages is inspired by reading the source code of the pg_recvlogical and pgjdbc.

This implementation is only extending the existing API and therefore should not break any of the existing code.

Smart replication feedback
This commit makes psycopg2 responsible for sending the status update
(feedback) messages to the server regardless of whether a synchronous or
asynchronous connection is used.

Feedback is sent every *status_update* (default value is 10) seconds,
which could be configured by passing a corresponding parameter to the
`start_replication()` or `start_replication_expert()` methods.
The actual feedback message is sent by the
`pq_read_replication_message()` when the *status_update* timeout is
reached.

The default behavior of the `send_feedback()` method is changed.
It doesn't send a feedback message on every call anymore but just
updates internal structures. There is still a way to *force* sending
a message if *force* or *reply* parameters are set.

The new approach has certain advantages:
1. The client can simply call the `send_feedback()` for every
   processed message and the library will take care of not overwhelming
   the server. Actually, in the synchronous mode it is even mandatory
   to confirm every processed message.
2. The library tracks internally the pointer of the last received
   message which is not keepalive. If the client confirmed the last
   message and after that server sends only keepalives with increasing
   *wal_end*, the library can safely move forward *flush* position to
   the *wal_end* and later automatically report it to the server.

Reporting of the *wal_end* received from keepalive messages is very
important. Not doing so casing:
1. Excessive disk usage, because the replication slot prevents from
   WAL being cleaned up.
2. The smart and fast shutdown of the server could last indefinitely
   because walsender waits until the client report *flush* position
   equal to the *wal_end*.

This implementation is only extending the existing API and therefore
should not break any of the existing code.
@dvarrazzo

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Thank you for the MR. It seems important enough that we could release the change in a bugfix release, if we are sure no code is broken by the change.

I would like to ask for feedback to @a1exsh and @ringerc, who are the authors of the original streaming replication support (#322)


def consume(msg):
pass
# we don't see the error from the server before we try to read the data

This comment has been minimized.

Copy link
@a1exsh

a1exsh May 6, 2019

Contributor

This was the behavior with older server versions, which is corrected in newer ones, e.g. v11. No reason to test specific server behavior if we care about the client being able to recover from error.

@a1exsh

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@dvarrazzo we've discussed the need for the change and specific approach taken (maintaining backwards compatibility was a priority) with @CyberDem0n before he has implemented it.

@dvarrazzo
Copy link
Member

left a comment

The new parameters would require a versionchanged in the function docs, and the new attributes a versionadded.

@dvarrazzo

This comment has been minimized.

Copy link
Member

commented May 6, 2019

@dvarrazzo we've discussed the need for the change and specific approach taken (maintaining backwards compatibility was a priority) with @CyberDem0n before he has implemented it.

Thank you @a1exsh: really appreciated!

CyberDem0n added some commits May 6, 2019

Change the default value of keepalive_interval parameter to None
The previous default value was 10 seconds, what might cause silent
overwrite of the *status_interval* specified in the `start_replication()`
@dvarrazzo

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Looks good to me. I will add a NEWS entry myself on merging. Is this in a mergeable state for you, @CyberDem0n, @a1exsh?

@a1exsh

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Looks good to me!

@dvarrazzo dvarrazzo merged commit 90755e6 into psycopg:master May 12, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Jan-M

This comment has been minimized.

Copy link

commented May 13, 2019

Thanks for the quick help and merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.