-
Notifications
You must be signed in to change notification settings - Fork 18
feature: getNextCrudTransactionBatch #222
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
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.
Thank you for your contribution!
We still think this is a feature we want to have, but after some discussion we also want to get the parameters for limits right and want to ensure this method performs well even with a large amount of local crud items. Another thing we're looking at is possibly adding an index on ps_crud.tx_id
to make queries faster (but we might be able to rely on the fact that the rowid order matches the transaction order - something to take a look at depending on what query we use for this in the end).
* Default is 5. | ||
*/ | ||
@Throws(PowerSyncException::class, CancellationException::class) | ||
public suspend fun getNextCrudTransactionBatch(transactionLimit: Int = 10): CrudBatch? |
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.
Another thing we would likely want here is to have an optional limit on the amount of crud items - here it can essentially be unbounded if you have multiple huge transactions.
I'm not sure yet if we'll need both limits or if a limit on items is enough. Basically the behavior would be to return the maximum amount of transactions so that the total items in those transactions doesn't exceed the limit, but also return at least one transaction.
return internalDb.readTransaction { transaction -> | ||
// Since tx_id can be null, we can't use a WHERE tx_id < ? with transactionLimit + first crud entry tx_id | ||
// So we get all operations and group them by transaction or fall back to an individual transaction if tx_id is null | ||
val allOperations = |
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.
We would likely need an optimized query here that does the counting in SQL - fetching all items first could be too inefficient.
I know this isn't easy to get right in SQL. Some windowing magic might help, if you need suggestions we're also happy to help.
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'll look into it and get back to you - do we have a number of items and a measure of "good" performance ?
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 don't have specific times in mind. We're just concerned about the case where we're requesting say batches with a limit of 100 total entries in a state where we have a crud backlog of 100,000 items, that still shouldn't take long.
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 added a benchmark test
…of transactionLimit, improve performance by using CTEs & window function
No description provided.