Skip to content
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

Option to run a few queries sequentially #3032

Closed
davidmartos96 opened this issue Jun 5, 2024 · 3 comments · Fixed by #3040
Closed

Option to run a few queries sequentially #3032

davidmartos96 opened this issue Jun 5, 2024 · 3 comments · Fixed by #3040
Labels
enhancement New feature or request

Comments

@davidmartos96
Copy link
Contributor

davidmartos96 commented Jun 5, 2024

Currently the DelegatedDatabase has the notion of isSequential in order to support parallel queries and transactions.
On ElectricSQL they recently introduced the option to disable the foreign key checks when replicating from remote, as it has better performance on web.
In order to do that it's necessary to do that in a safe manner, they do the following in a mutex. This is because foreign_keys cannot be called during a transaction.

PRAGMA foreign_keys = OFF;
SQLite transaction block {
	...
}
PRAGMA foreign_keys = ON;

Given that Drift can already work with sequential databases we were wondering if it would be feasible to add a function that runs a callback inside the same synchronized Lock. Something like runSequentially or runExclusively.

Context:
electric-sql/electric#1281
https://github.com/electric-sql/electric/blob/64a01949376536492fbc72fe6bf0d8539a3552b0/clients/typescript/src/util/transactions.ts#L23

@davidmartos96 davidmartos96 added the enhancement New feature or request label Jun 5, 2024
@simolus3
Copy link
Owner

simolus3 commented Jun 6, 2024

I think you'd still have the problem with sequential databases because they only serialize database access at a fairly low layer.

So if you do something like this Dart:

await customStatement('pragma foreign_keys = OFF;');
await transaction(() async {
  // ...
});
await customStatement('pragma foreign_keys = ON;');

If another async context is accessing the database and does its own thing, queries from there could be interleaved between the customStatement and transaction block, even with isSequential: true.

I agree that having a block that reliably locks the database is a good addition. It should ideally be implemented at a QueryExecutor level though (doing it in the database class is not reliable since this also needs to work across isolates, so there could be multiple database classes talking to the same logical database). Doing that there is a breaking change though...

@simolus3 simolus3 added the breaking A breaking change that would require a major version jump label Jun 6, 2024
@davidmartos96
Copy link
Contributor Author

@simolus3 I was thinking at the BaseExecutor level, which already manages its own lock. Is that the database low level you were referring to or the QueryExecutor level?

If the additional method is added to the QueryExecutor and to the DatabaseConnectionUser classes, what is the breaking change? Is it because subclasses need to implement it?

@simolus3 simolus3 removed the breaking A breaking change that would require a major version jump label Jun 8, 2024
@simolus3
Copy link
Owner

simolus3 commented Jun 8, 2024

No you're right! I've misremembered things and thought QueryExecutor was a public and stable interface. It's explicitly marked as internal in the documentation though, so I feel comfortable adding a beginExclusive method there and implementing it in the BaseExecutor (well, and in the remote isolate implementation and so on). I haven't completed this yet, but it doesn't look like a breaking change outside of minor concerns for web workers (beginExclusive will only work with newer workers due to a backwards-compatible change to the RPC protocol, that is not a problem though).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants