Repository navigation
[sqflite] Support query cursors and fix transaction state handling - #1209
seungsoo47 wants to merge 4 commits into
Conversation
Handle cursorPageSize in query and the queryCursorNext method so that queryCursor/rawQueryCursor read rows page by page instead of loading the whole result at once. A cursor keeps its own uncached statement until the last page is read or it is cancelled, and the statement is finalized when the database is closed. Upstream sqflite 2.2.0+3 added queryCursor()/rawQueryCursor() (https://github.com/tekartik/sqflite/blob/master/sqflite/CHANGELOG.md). This follows its Android and Darwin implementations (Database.java doQuery/doQueryCursorNext, SqfliteDatabase.m dbDoQuery/dbQueryCursorNext).
When a single instance database is re-opened, for example after a hot restart, the plugin always returned recoveredInTransaction as false. A transaction left open by the previous Dart isolate was therefore never rolled back by sqflite_common (rollbackActiveTransactionOnOpen), and its uncommitted changes stayed visible to the new isolate. Report it from sqlite3_get_autocommit(), as the upstream Android implementation does with its transaction depth. Upstream: sqflite_common 2.5.8 rollbackActiveTransactionOnOpen, sqflite_android SqflitePlugin.onOpenDatabaseCall (makeOpenResult(databaseId, true, database.isInTransaction())).
SQLite rolls back the whole transaction by itself on some errors (ConflictAlgorithm.rollback, RAISE(ROLLBACK)...). The plugin did not tell the client, so the next statements of the transaction ran and were committed outside of any transaction, and a continueOnError batch kept going. When a failed statement started inside a transaction and SQLite is back in autocommit mode, set transactionClosed in the error data so that sqflite 2.4.5 marks the transaction closed, and stop the batch. The execute error now carries the sql and arguments like the other methods. Same fix as sqflite_android 2.4.5 and sqflite 2.4.5: tekartik/sqflite@25c882c tekartik/sqflite@0b78153 The tests are ported from sqflite_common_test ('rolled back transactions', 'Batch rolled back continue on error').
A failed COMMIT (e.g. a deferred foreign key violation) leaves SQLite in the transaction, while the client considers it ended. The uncommitted rows stayed visible and the next transaction failed to begin. When an execute that ends a transaction (inTransaction: false) fails and SQLite is still in a transaction, roll it back. A failed ROLLBACK TO a savepoint keeps the transaction, as upstream does. Same fix as sqflite_android 2.4.5: tekartik/sqflite@76804b4 The test is ported from sqflite_common_test ('failed commit (deferred foreign key)'). The hang and worker pool parts of that commit do not apply, the Tizen plugin does not queue calls by transaction id.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e833a207c6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| flutter::EncodableValue(cursor_id); | ||
| } | ||
| } catch (const sqflite_errors::DatabaseError &exception) { | ||
| result->Error(sqflite_constants::kErrorDatabase, exception.what()); |
There was a problem hiding this comment.
If sqlite3_step fails on a later page while an explicit transaction is active and SQLite auto-rolls that transaction back (for example on an I/O failure), QueryStmt marks the rethrown DatabaseError.transaction_closed, but this handler discards all error data. When the caller catches the page error and continues its transaction callback, sqflite never marks the transaction closed and later statements can execute and commit in autocommit mode; route this through an error response that includes transactionClosed, as the normal query handler does.
Useful? React with 👍 / 👎.
| auto response = | ||
| MakeOpenResult(*found_database_id, true, in_transaction); |
There was a problem hiding this comment.
If a hot restart occurs after QueryCursor has stored a statement but before Dart sends queryCursorNext(cancel: true), the old Dart cursor is gone while the static database_map_ reuses this DatabaseManager. This recovery path returns it without finalizing cursors_, so an orphaned active SELECT can make the new isolate's schema migration fail with database table is locked/SQL statements in progress (and can block writers on another connection) until the whole database is closed; clear outstanding native cursors as part of recovered-open handling.
Useful? React with 👍 / 👎.
Supports paged queries and aligns transaction handling with sqflite 2.4.5.
queryCursorandrawQueryCursor(cursorPageSize,queryCursorNext).ConflictAlgorithm.rollback,RAISE(ROLLBACK)...) as closed, and stop acontinueOnErrorbatch.COMMITfails (e.g. a deferred foreign key violation).Updates sqflite to 2.4.5 and bumps the version to 0.2.0.