-
Notifications
You must be signed in to change notification settings - Fork 95
LogicalTransactions in frontend Client #317
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
base: main
Are you sure you want to change the base?
Conversation
export PGPASSWORD=postgres | ||
|
||
docker-compose up -d | ||
docker compose up -d |
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.
docker-compose
is deprecated.
docker compose
has replaced it.
docker-compose
does not work on my recent version of docker
desktop.
docker compose
works on the version of docker in our CI.
win-win?
I suppose it will work for a every engineer's machine too. Can rollback this change if that is not the case
prepared_statements: &mut PreparedStatements, | ||
params: &Parameters, | ||
in_transaction: bool, | ||
logical_transaction: &LogicalTransaction, |
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.
this is the root change, followed the compiler from here basically.
pgdog/src/frontend/client/mod.rs
Outdated
let in_transaction = self.in_transaction(); | ||
|
||
// 3) Reconcile against our LogicalTransaction | ||
if should_be_tx && !in_transaction { |
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'd probably clarify that the query parser isn't always active and therefore we don't catch BEGIN
statements always. So sometimes, the only way to know we're inside a transaction is because the server told us.
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 think you're right, I also think it might be cheap to check twice. I'll check 👀
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.
Cool!
Some(Command::StartTransaction(query)) => { | ||
if let BufferedQuery::Query(_) = query { | ||
self.start_transaction().await?; | ||
inner.start_transaction = Some(query.clone()); |
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.
This is important to record. BEGIN
can have multiple arguments that affect what kind of transaction we're starting: https://www.postgresql.org/docs/current/sql-begin.html
/// Execute a BEGIN on all servers | ||
/// TODO: Block mutli-shard BEGINs as transaction should not occur on multiple shards | ||
pub async fn begin(&mut self) -> Result<(), Error> { | ||
let query = Query::new("BEGIN"); |
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.
See other comment. Client can pass options to BEGIN
, changing transaction type.
Replace the dead-simple in_transaction flag with a real LogicalTransaction state machine
Why
LogicalTransaction
struct carries that context.What changed
Refresher on logical transactions
Merge as standalone file in PR #313: a logical transaction makes a sharded cluster look like a single Postgres instance.
Basically, single-node postgres have Transactions.
PgDog can only have LogicalTransacion, and must orchestrate true Transactions across shards.
It tracks shards, phases, and errors early so we can later block illegal cross-shard writes and bolt on 2PC enventually.
TODO
LogicalTransaction::touch() is wired to throw if the client tries to touch multiple shards in the same txn (might need to track backend_ids instead—TBD). It’s not used yet; hook it up next.