-
-
Notifications
You must be signed in to change notification settings - Fork 443
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
Support CopyBoth queries and replication mode in config #778
base: master
Are you sure you want to change the base?
Conversation
5db9588
to
71ad03e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. A few questions/concerns.
tokio-postgres/src/copy_both.rs
Outdated
/// coming from the server. If it is not, `Sink::close` may hang forever waiting for the stream | ||
/// messages to be consumed. | ||
/// | ||
/// The copy should be explicitly completed via the `Sink::close` method to ensure all data is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like dropping it is enough?
tokio-postgres/src/copy_both.rs
Outdated
/// The stream side *must* be consumed even if not required in order to process the messages | ||
/// coming from the server. If it is not, `Sink::close` may hang forever waiting for the stream | ||
/// messages to be consumed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand on the reasoning here? close()
or drop()
should certainly ensure that a CopyDone
is sent. And if the receiver is closed, that should cause future messages received in this sub-protocol to be discarded, eventually allowing the protocol to resume normal operations. Right?
tokio-postgres/src/copy_both.rs
Outdated
// Indicate to CopyBothReceiver to produce a Sync message instead of CopyDone | ||
let _ = this.error_sender.take().unwrap().send(()); | ||
return Poll::Ready(Some(Err(Error::db(error)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this assume that the error happened during START_REPLICATION
? What about an error that happened during the stream; shouldn't we still send a CopyDone
?
@@ -449,6 +451,15 @@ impl Client { | |||
copy_out::copy_out(self.inner(), statement).await | |||
} | |||
|
|||
/// Executes a CopyBoth query, returning a combined Stream+Sink type to read and write copy | |||
/// data. | |||
pub async fn copy_both_simple<T>(&self, query: &str) -> Result<CopyBothDuplex<T>, Error> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the replication stream, if the timeline is historical, Postgres will send a tuple as a response. So we actually need a function that returns something like Result<(CopyBothDuplex<T>, Option<SimpleQueryMessage>), Error>
(or maybe Result<(CopyBothDuplex<T>, Option<Vec<SimpleQueryMessage>>), Error>
in case other commands are added in the future which use CopyBoth
and return a set).
It's actually very specific to START_REPLICATION
(and even more specifically, to physical replication), so it might make sense to have a more specific name or at least clarify what it's expecting the command to do. Maybe something like copy_both_simple_with_result()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, I'll take a look on how we can expose this to users, ideally in a generic way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case you missed my other comment, there's a similar issue for BASE_BACKUP
, except with CopyOut
instead. That can be a separate PR, though.
Supporting these commands requires some special methods on |
In my PR, I had the above comment. Does that apply to this PR, as well? |
It would be good to have a way to send Standby Status Updates and Hot Standby Feedback. Not all of this has to be in this PR, though. I'm just commenting on everything necessary to make a full-featured and independent crate for replication. |
935bd20
to
f5e018f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeff-davis thank you for the thorough review!
Unfortunately last night I realised that the original design had a big hole that allowed de-syncing the protocol. I just push a new version that I think is Correct (tm) now.
When doing a CopyBoth query the architecture looks something like this:
|
<tokio_postgres owned> | <userland owned>
|
pg -> Connection -> CopyBothReceiver ---+---> CopyBothDuplex
| / \
| v v
| Sink Stream
The original version of the feature handled the state machine of the CopyBoth sub-protocol as part of the Stream implementation of CopyBothDuplex
and treated CopyBothReceiver
as a dumb relay of messages into the connection. Therein lies the problem. A user could create a CopyBothDuplex
and drop it immediately. In that case the dumb CopyBothReceiver
would unconditionally send a CopyDone
to the server and finish. But what if the server sent an error in the meantime? There was nothing to handle this fact and no Sync message was being sent.
So this re-work of the feature flips this relationship around. CopyBothReceiver
contains all the logic to drive the sub-protocol forward and ensures that no matter what the correct messages are being exchanged with the server. The user is free to drop their end at any point.
I've also added a ton of comments and a few diagrams to make reading and reviewing the code easier. If it's not a lot of ask I'd hugely appreciate another round of review, I think we're close!
Next week I plan to compile a modified version of postgres that errors out on purpose to test out the error paths of this PR and also see how we can incorporate getting results back for things like timeline changes etc.
@@ -449,6 +451,15 @@ impl Client { | |||
copy_out::copy_out(self.inner(), statement).await | |||
} | |||
|
|||
/// Executes a CopyBoth query, returning a combined Stream+Sink type to read and write copy | |||
/// data. | |||
pub async fn copy_both_simple<T>(&self, query: &str) -> Result<CopyBothDuplex<T>, Error> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, I'll take a look on how we can expose this to users, ideally in a generic way.
It looks like the general pattern is that the database can respond with result sets, copy outs, or copy boths. Based on that I think we need here is to define something like: enum ResponsePart {
RowStream(RowStream),
SimpleQueryStream(SimpleQueryStream),
CopyOutStream(CopyOutStream),
CopyBothStream(CopyBothStream),
} And then provide a method to send a query and get a Stream of
hm do you have a link to the patch? I thought pipelined queries were not allowed over a replication connection |
I like it. Maybe it can even be refactored so that there's one generic entry point,
Is there a reason you thought pipelined queries would not be allowed in replication mode? And if so, how would you prevent them, since the whole protocol implementation is built around pipelining, without explicitly blocking in certain cases? |
You're 100% right. I confused pipelined queries with extended query mode which is the one that's not allowed in replication mode. So yeah, your original comment still applies in that users should pay attention to this bug. I added this note in the documentation of the |
tokio-postgres/src/copy_both.rs
Outdated
/// CopyOut-->ServerError<--CopyIn | ||
/// \ | / | ||
/// `---, | ,---' | ||
/// \ | / | ||
/// v v v | ||
/// CopyNone | ||
/// | | ||
/// v | ||
/// CopyComplete | ||
/// | | ||
/// v | ||
/// CommandComplete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the right place in the state machine for ServerError
. If a server error happens in CopyBoth
mode, that's the odd (and hard-to-test) case where an error is thrown by the server in the middle of streaming. From the docs, it looks like Postgres will just return the ErrorResponse
and then ReadyForQuery
, without either of the CommandComplete
messages.
You can test this by hacking up test_decoding
to throw an error randomly every 10 records or something.
Summary of the remaining issues as I see them:
After these are done, I'll be able to port my other replication code to a new crate, and if that works, then I think this PR is ready. |
50c81b8
to
77dff40
Compare
@sfackler any news on this ? I'm relying on this PR for a project and I think a lot of people would benefits from this. |
@Martichou, you're welcome to use our fork (https://github.com/materializeInc/rust-postgres) for the moment! It's got this PR and #774 together, and we integrate new changes from rust-postgres periodically. (We would, of course, love to get these PRs upstreamed, but @sfackler's time has seemed quite limited lately.) |
@benesch Thanks ! I was doing the same (maintaining a fork), but I'll use yours from now on ;) |
915ef69
to
2fed91d
Compare
@sfackler It'd be great if you can review this PR by @petrosagg and @jeff-davis. Merging this will be helpful for those who are currently maintaining or using the forks. |
@sfackler would you be interested in replication support? I know this PR has conflicts, but I can clean this one up. Or, if you are not happy with the current design, maybe open a fresh one? We are forced to use a fork from MaterializeInc (cc @benesch) in pg_replicate which is not ideal because it prevents us from publishing to crates.io. |
Hey @imor! We were excited to see you build |
I rebased this PR on top of current |
@benesch That's sad. Were there any concrete reasons given as to why this PR is not accepted? I don't see any comment from Steven in this PR. In the worst case if the status quo doesn't change, would it be too much to ask to publish your fork on crates.io. It will allow downstream users to publish their crates as well. Not ideal, I know, but looking for options here. |
Nope, we’ve never heard anything, I’m afraid. I think Steven maintains this project on a volunteer basis though so I certainly don’t blame him. :) Re a fork: I’d like to avoid having us (@MaterializeInc) on the critical path for new releases of |
Keep in mind that our fork contains more or less the code in this PR but also a lot more (the entire logical replication decoding logic). The idea was that once this PR was merged then the logical replication protocol would be handled by a separate crate layered on top of tokio-postgres, which would then expose the right functionality (i.e CopyBoth queries). I came up with this layering after we had our fork working to reduce the size of this PR and make more likely to be reviewed and merged. So what it might make sense to publish is a crate of this PR and then we can extract the logical decoding bits into another one. In the event that this PR eventually gets merged we can deprecate the published fork crate but keep the logical replication crate |
Oh there's no question of blaming Steven. All I have is gratitude for this great library. I understand how hard can it be to maintain a popular open source project. Regarding the way forward, I like @petrosagg's suggestion about publishing a separate crate. A very practical solution which avoid fragmentation by a fork. I'm willing to help with this effort so let me know how I can be of use. |
I sent @sfackler an email asking if he has bandwidth to review/merge this PR. If we don't hear back in a few days, then we can investigate the backup plan of publishing a crate which is just rust-postgres plus this one PR. |
6340072
to
63b8392
Compare
Heads up that I tidied up our fork and brought in all changes from the master branch of this repo plus this PR. The logical replication functionality has been extracted in a separate crate here https://github.com/MaterializeInc/rust-postgres/tree/master/postgres-replication and is the crate that we could in theory publish if this PR ever gets merged or gets published under a different name. |
Co-authored-by: Petros Angelatos <[email protected]>
Signed-off-by: Petros Angelatos <[email protected]>
Signed-off-by: Petros Angelatos <[email protected]>
Signed-off-by: Petros Angelatos <[email protected]>
This PR only adds support for setting the connection replication mode in the connection configuration and support for
CopyBoth
mode queries. This is the minimum support needed for downstream users to implement replication support on their own.@jeff-davis After studying the code I think we don't need to add
unpipelined_send
nor worry about protocol desyncs.The reason we don't need
unpipelined_send
is because just likeCopyIn
, when performing a CopyBoth operation the connection enters a special mode where it consumes FrontendMessages from a sub-receiver . These sub-receivers are theCopyInReceiver
andCopyBothReceiver
structs.The reason we don't need to worry about protocol desyncs is that once the connection enters a sub-mode (with
Copy{In,Both}Receiver
) all other sends to the main command queue are ignored while the subprotocol is running. Only after these are exhausted, which always happens with aCopyDone
/CopyFail
/Sync
message, the system resumes consuming the main queue.The PR includes two tests that verify the client can resume operation to normal query processing. One that gracefully shuts down the replication stream and one that simply drops the handles.