Skip to content

feat: faster replication#1166

Open
ygxio wants to merge 7 commits into
pgdogdev:mainfrom
ygxio:faster-replication
Open

feat: faster replication#1166
ygxio wants to merge 7 commits into
pgdogdev:mainfrom
ygxio:faster-replication

Conversation

@ygxio

@ygxio ygxio commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

fixes #1036

@ygxio

ygxio commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

I am still working on the design for it, just a peak if the direction seems well @levkk

@ygxio ygxio changed the title faster replication v1 faster replication Jul 8, 2026
@levkk

levkk commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Cool! Would be good to see some benchmark numbers, just to get a sense of how much faster this is.

@ygxio

ygxio commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Do you have any such setup so that i can benchmark it?

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.28446% with 57 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...backend/replication/logical/subscriber/pipeline.rs 84.06% 47 Missing ⚠️
...c/backend/replication/logical/subscriber/stream.rs 80.00% 9 Missing ⚠️
pgdog/src/backend/replication/logical/error.rs 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@levkk

levkk commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Do you have any such setup so that i can benchmark it?

This might work:

benches/bench.sh

Specifically, see resharding folder.

@ygxio

ygxio commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author
image

PGDOG_BIN=/tmp/pgdog-new bash benches/resharding/replication/run.sh --baseline main

@ygxio

ygxio commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

For BENCH_SCALE = 3000, DEST_LATENCY_MS=10 we have more performance boosts

image

@ygxio

ygxio commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

RUNS = 2
WARMUP = 1
BENCH_SCALE = 5000
DEST_LATENCY_MS = 10
USE_TOXI = 1

image

@levkk results seems good but also too good at a point

@levkk

levkk commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Async should be a big improvement, so not that surprising. This is very cool. Let me know when you're ready for a review, I'll tag Kiryl.

@ygxio

ygxio commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

@levkk ready for review

@ygxio ygxio changed the title faster replication feat: faster replication Jul 13, 2026
@levkk
levkk requested a review from meskill July 13, 2026 17:42
}

#[cfg(test)]
mod test {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should add more unit tests here? I think we should have some for testing the whole flow - prepare -> execute -> await. And also some tests for possible errors

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure @meskill would add more tests

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still miss some important tests:

  • error path - something that will trigger an error response, validate that we get the error properly and we won't block the execution for future calls
  • missed rows calculation - just some calls to generate missed rows and make sure we get proper stats for it

@ygxio

ygxio commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

@meskill i have added a few more could you take a look?

@meskill

meskill commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Thanks @ygxio I'm still checking the code more deeply, but anyway looking great so far.

Putting my bench results to confirm the improvements:

Against main

With defaults:
image

With toxi and latency for source/destination for 1ms:
image

Separately

And the next without comparison with main (compare against prev run) since since it is executing too long to get the result.

With latency 10ms:
image

With latency 100ms:
image

@ygxio

ygxio commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

i see from 1 to 10 to 100 ms of latency the jump has been pretty high, need to see the cause maybe

Comment on lines +207 to +216
// One entry per outstanding DML op, in send order: `is_direct`. Popped on
// each CommandComplete.
direct: VecDeque<bool>,
// In-transaction prepare sync points: (remaining ParseComplete acks, waiter).
parse_ack_sync_points: VecDeque<(usize, oneshot::Sender<()>)>,
// Commit / out-of-transaction prepare sync points, resolved on ReadyForQuery.
ready_for_query_sync_points: VecDeque<oneshot::Sender<()>>,
// Waiter that resolves once every DML ack has been read. At most one is
// outstanding: `commit()` issues drain_acks sequentially, one per connection.
drain_waiter: Option<oneshot::Sender<()>>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The multiple separate VecDeque like this works, but I see this more error-prone and harder to maintain. The issue is this multiple queues partially maps to the underlaying FIFO queue executed by the backend. Due to current design of the replication it works just fine, but in case we add something new to a processing or try to extend async handling that's could become a break point.

I'd suggest to use a single queue of enums to control the state of the processing and the expected responses, to make sure we get everything we expect and we don't process entries in the wrong order for some reason.
I think the ProtocolState from pgdog/src/backend/protocol/state.rs does use the single queue for something like this.

impl Listener {
async fn run(mut self) {
loop {
select! {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't prove it but I believe here could be a theoretical issue due to disproportion of read/writes events - the usual execute command from recv expected to generate 2 server.read events. Since the select! is fair by default the buffer for server.read could grow fast that could lead to tcp stream blocking.

I'm thinking if we need to add biased to select! and move the read first, so we'll read the messages first if any to avoid this.

// latched on any connection means whole transaction is aborted.
for conn in &self.connections {
if let Some(err) = conn.take_error() {
return Err(err);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please check how does effect on one connection affect other connections and streams - are they cancelled in any way?
I think they stop after dropping the channel, but I want to be sure we are not doing useless work here.

// Rows a direct-to-shard DML expected to touch but didn't (0 rows affected).
missed: MissedRows,
// Test-only: total ParseComplete acks consumed by parse-ack sync points.
#[cfg(test)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we try to avoid this is the main code?

let's better test that everything works as expected after some commands than to rely on some implementation details. If we have prepare statements - we can validate that this statement work after parsing and that listener doesn't block for example.

}

#[cfg(test)]
mod test {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still miss some important tests:

  • error path - something that will trigger an error response, validate that we get the error properly and we won't block the execution for future calls
  • missed rows calculation - just some calls to generate missed rows and make sure we get proper stats for it

// If in transaction we send flush and wait for the acknowledgements
// since we donot want to commit the open transaction.
messages.push(Flush.into());
SyncPointKind::ParseAcks(parses.len())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add some validation that this should not be 0, since we may overflow on this later

c => return Err(Error::RelationOutOfSync(c)),
}
}
server.prepare(parses, in_txn).await?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also can speed up this a very little if we use try_join to run prepare in parallel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add async streaming for replication to improve replication speed

3 participants