planetscale/database-js

Inconsistent transactions with `Promise.all`

tomqchristensen opened this issue · 10 comments

When using the transaction api, attempts to parallelize parts of the transaction using Promise.all can lead to corrupt data with no indication of failure.

I've created a detailed repro here (note: the example is contrived on purpose and could be done without a transaction). The summary is that using a pattern like await Promise.all([tx.execute(...), ..., tx.execute(...)]) inside a transaction can lead to only one of those statements being executed and the rest being ignored, resulting in corrupt data. Since all promises resolve, this happens without any indication of a failure.

The same pattern works as expected for the mysql2 driver (example provided in the repro).

Hi there. I'm not sure if there's a way we can strictly prevent this, but doing this would definitely fall under "undefined behavior". Parallelism within a transaction without any coordination is going to do what you describe.

Within a single transaction, queries need to be executed sequentially. You can leverage multiple transactions and operate over all transactions in parallel just fine.

We would need to introduce a lock or something, I'm not sure what's available to use easily within JavaScript, to prevent this. Even so, it'd still fall under undefined behavior since the order wouldn't be guaranteed, which is a fundamental property of transactions. Transactions are intended to be sequential.

If the mysql2 library allows this, it must use a separate transaction, or in its case, a discrete connection entirely to accomplish it. The MySQL protocol wouldn't allow the behavior you're describing.

Yeah, I figured it might not be proper use of transactions. I'll close this issue. Given it's valid JS syntax and the mysql2 library allows it, perhaps it's worth adding a cautionary note in the docs to discourage this kind of use?

For sure, I agree. I'm genuinely curious what mysql2 does here. I wonder if they use a mutex to cause them to execute serially? Or if they execute truly in parallel? Do you happen to know offhand?

I'm going to look into this later for my curiosity since I wouldn't have expected that to work.

I don't know, I'm afraid. What I find strange is that when using the Planetscale driver, I can see in the Insights tab that all of the statements are hitting the db, but somehow only one ends up getting executed (I believe it's the first statement to hit the db that actually gets executed). Makes me think that it has something to do with how the Planetscale HTTP API handles execution compared to TCP. Could it have something to do with the session being updated on each response here? If there's some server logic to discard requests with stale sessions, perhaps only the first request that reaches the db is considered fresh, and the other parallel requests are just ignored.

I guess to your point, we document how to correctly do a transaction: https://github.com/planetscale/database-js#transactions

But we don't explicitly say why it's done that way or anything about avoiding parallelism within a transaction. Our example explicitly awaits each individual query sequentially.

I'll see what we can do here.

Ok, so I just quickly looked into what mysql2 does for this. And as predicted, it doesn't actually execute in parallel over a single connection.

Internally, mysql2 uses a Queue to force serial execution over each statement. So the promises are effectively all waiting on a lock, and running in a sorta unpredictable order. But they are executing sequentially, which is at minimum, what's required here, since fundamentally parallel execution is not possible.

So while I think promises are a weird API here given there can't be any parallelism, I think the best we can do here as well is also to leverage a Queue or a Mutex to prevent it, that at least gets us out of undefined behavior territory. We can ensure no statements truly execute in parallel.

Out of curiosity, what is your goal with trying to do parallel queries within a single transaction? Are you trying to just optimize round-trip times? If our API had a method such as executeBatch([..]) that took an array of statements you could send in one request, would that be sufficient here? Then the array can be ordered how you like, and it'd be 1 round trip.

I'm asking since I'm not personally sure any other reason you'd want to do parallelism within a transaction without breaking the database.

Forcing sequential execution sounds like a sensible solution for sure.

Yep, I was naively attempting to avoid an http "waterfall" when updating many records. The specific use case here is that I have a foo table where each foo belongs to some foo_group, and I need to update all foo rows in a specific foo_group. Each foo row update can be run independently of the others, but all of the updates must either complete or fail together to avoid data corruption - I'm not sure if an executeBatch API would offer this kind of guarantee?

At the end of the day, running the updates sequentially is not a big issue but I was curious as to why it worked with mysql2 but not this driver.

Yeah, that makes sense. So yeah, the executeBatch would give you 1 round trip cost, and we'd perform the execution sequentially on the server. So effectively the same result as doing them in parallel, but still within transactional guarantees. They'd give you an all or nothing outcome, but only obviously if they are wrapped in a transaction. Otherwise, they'd use auto-commit.

So ultimately something like:

const updateTxParallel = async (n: number) =>
  await conn.transaction(async (tx) => {
    const pipeline = tx.pipeline()
    fooIds.map((id) =>
      pipeline.push('update `foo` set `n` = ? where `foo`.`id` = ?', [n, id])
    )
    await pipeline.run()
  })

TBD on the API, but might surface it as a "pipeline" kind of interface for simplicity. So the only blocking operation here is pipeline.run() which sends them all to use in a single round trip.

At the end of the day, running the updates sequentially is not a big issue but I was curious as to why it worked with mysql2 but not this driver.

And I guess to be clear, mysql2 isn't actually doing any parallel execution, it just appears to be because they abstract this away. You gain no benefit of this pattern in mysql2 either in practice. At least you shouldn't, since internally it's just translating to a for loop over each query. It's all a facade. :)

Would love a pipeline.run(). I am using replicache with transaction. Would love a way to pull all the tables I need with a single round trip to the db.