feat: 2pc logic for resharding#1088
Conversation
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Could you please uncomment the tests in the |
|
@meskill these tests pass for me even if two_pc flag is disabled |
yeah, they are just flaky sometimes on ci. don't worry about it, I'll be just checking ci status |
|
@meskill are we ok to merge or do we need anything else? |
| TwoPcPhase::Phase1 => format!("PREPARE TRANSACTION '{txn}_{shard}'"), | ||
| TwoPcPhase::Phase2 => format!("COMMIT PREPARED '{txn}_{shard}'"), |
There was a problem hiding this comment.
this should work rn since the same logic is used for tx name as in the binding
but it could diverge if something changed on one side. It's especially important since the rollback is used exactly from the binding.
Can we share some of the logic of the query or tx name creation between here and binding.rs? that'll prevent possible issues
| let query = match phase { | ||
| TwoPcPhase::Phase1 => format!("PREPARE TRANSACTION '{txn}_{shard}'"), | ||
| TwoPcPhase::Phase2 => format!("COMMIT PREPARED '{txn}_{shard}'"), | ||
| _ => unreachable!(), |
There was a problem hiding this comment.
There could be some issues due to the nature of cli calls i.e. when we run pgdog data-sync instead admin command on the running instance:
- the
Manager::monitorshould be running anyway, but we don't wait for it's shutdown in the cli path. That means if we fail the prepared transactions could be left on some shards and they won't be cleaned up. - wal is also is not enabled in cli path, but on running instance it'll be enabled. We can get different behavior depending how exactly the copy command was run.
We probably should unify this and make sure we wait for shutdown and enable wal for the cli path. But that also means we should think how the old transactions should be recovered..
There was a problem hiding this comment.
should i take up this as another PR, after thinking about crash recovery
There was a problem hiding this comment.
We can call this waiter on shutdown: https://github.com/pgdogdev/pgdog/blob/main/pgdog/src/frontend/client/query_engine/two_pc/manager.rs#L357
| } | ||
|
|
||
| for result in join_all(futures).await { | ||
| result?; |
There was a problem hiding this comment.
I'm not sure if it's really a problem, but the commit/rollback for prepared transactions happens inside Manager::monitor handler and it's async to this code meaning we may actually start new attempt before the manager logic is fired and the cleanup could happen in the middle of new copy attempt.
@levkk wdyt? should we care about it?
There was a problem hiding this comment.
Maybe we can add a waiter to make sure the transaction is rolled back / committed?
There was a problem hiding this comment.
That makes sense. Should we add some code to wait for the transaction to be rolled back?
There was a problem hiding this comment.
yeah, something like this, but this should be probably done not just after the error, but when starting the new attempt, so we won't wait twice until the transaction is rolled back and then the sleep timeout between retries. We could wait for manager's queue exhausting, before starting to push data.
4 participants
Use two-phase commit for
COPYcommands when resharding. This allows for a clean rollback on error.