2025 10 18 make maybeint int conversion strict - #4
Open
bit-aloo wants to merge 467 commits into
Open
Conversation
closes tursodatabase#3672 Reviewed-by: Pere Diaz Bou <pere-altea@homail.com> Closes tursodatabase#3686
closes tursodatabase#3688 Reviewed-by: Pere Diaz Bou <pere-altea@homail.com> Closes tursodatabase#3692
…ntext' from Preston Thorpe This PR contains NO semantic changes at all, this simply refactors existing INSERT code to be easier to reason about. Very sorry I know I've been working on `INSERT OR IGNORE|REPLACE|etc..` for days now but the insert translation was literally unbearable and I got it working but I barely could wrap my head around that whole `translate_insert` function, so I spent a bunch of time refactoring the whole INSERT handling out into different "Plans"... which turned into a whole different clusterf***... So I just went back and made the existing insert emission more modular and created some context that can make it easier to reason about. This should be able to just be merged quickly Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com> Closes tursodatabase#3731
… Jussi Saurio Closes tursodatabase#2600 ## Problem Every btree has a key it is sorted by - this is the integer `rowid` for tables and an arbitrary-sized, potentially multi-column key for indexes. Executing an UPDATE in a loop is not safe if the update modifies any part of the key of the btree that is used for iterating the rows in said loop. For example: - Using the table itself to iterate rows is not safe if the UPDATE modifies the rowid (or rowid alias) of a row, because since it modifies the iteration order itself, it may cause rows to be skipped: ```sql CREATE TABLE t(x INTEGER PRIMARY KEY, y); INSERT <something> UPDATE t SET y = RANDOM() where x > 100; // safe to iterate 't', 'y' is not being modified UPDATE t SET x = RANDOM() where x > 100; // not safe to iterate 't', 'x' is being modified ``` - Using an index to iterate rows is not safe if the UPDATE modifies any of the columns in the index key ```sql CREATE TABLE t(x, y, z); CREATE INDEX txy ON t (x,y); INSERT <something> UPDATE t SET z = RANDOM() where x = 100 and y > 0; // safe to iterate txy, neither x or y is being modified UPDATE t SET x = RANDOM() where x = 100 and y > 0; // not safe to iterate txy, 'x' is being modified UPDATE t SET y = RANDOM() where x = 100 and y > 0; // not safe to iterate txy, 'y' is being modified ``` ## Current solution in tursodb Our current `main` code recognizes this issue and adopts this pseudocode algorithm from SQLite: - open a table or index for reading the rows of the source table, - for each row that matches the condition in the UPDATE statement, write the row into a temporary table - then use that temporary table for iteration in the UPDATE loop. This guarantees that the iteration order will not be affected by the UPDATEs because the ephemeral table is not under modification. ## Problem with current solution Our `main` code specialcases the ephemeral table solution to rowids / rowid aliases only. Using indexes for UPDATE iteration was disabled in an earlier PR (tursodatabase#2599) due to the safety issue mentioned above, which means that many UPDATE statements become full table scans: ```sql turso> create table t(x PRIMARY KEY); turso> insert into t select value from generate_series(1,10000); turso> explain update t set x = x + 100000 where x > 50 and x < 60; addr opcode p1 p2 p3 p4 p5 comment ---- ----------------- ---- ---- ---- ------------- -- ------- 0 Init 0 28 0 0 Start at 28 1 OpenWrite 0 2 0 0 root=2; iDb=0 2 OpenWrite 1 3 0 0 root=3; iDb=0 -- scan entire 't' despite very narrow update range! 3 Rewind 0 27 0 0 Rewind table t ... ``` ## Solution We move the ephemeral table logic to _after_ the optimizer has selected the best access path for the table, and then, if the UPDATE modifies the key of the chosen access path (table or index; whichever was selected by the optimizer), we change the plan to include the ephemeral table prepopulation. Hence, the same query from above becomes: ```sql turso> explain update t set x = x + 100000 where x > 50 and x < 60; addr opcode p1 p2 p3 p4 p5 comment ---- ----------------- ---- ---- ---- ------------- -- ------- 0 Init 0 35 0 0 Start at 35 1 OpenEphemeral 0 1 0 0 cursor=0 is_table=true 2 OpenRead 1 3 0 0 index=sqlite_autoindex_t_1, root=3, iDb=0 3 Integer 50 2 0 0 r[2]=50 -- index seek on PRIMARY KEY index 4 SeekGT 1 10 2 0 key=[2..2] 5 Integer 60 2 0 0 r[2]=60 6 IdxGE 1 10 2 0 key=[2..2] 7 IdxRowId 1 1 0 0 r[1]=cursor 1 for index sqlite_autoindex_t_1.rowid 8 Insert 0 3 1 ephemeral_scratch 2 intkey=r[1] data=r[3] 9 Next 1 6 0 0 10 OpenWrite 2 2 0 0 root=2; iDb=0 11 OpenWrite 3 3 0 0 root=3; iDb=0 -- only scan rows that were inserted to ephemeral index 12 Rewind 0 34 0 0 Rewind table ephemeral_scratch 13 RowId 0 5 0 0 r[5]=ephemeral_scratch.rowid ``` Note that an ephemeral index does not have to be used if the index is not affected: ```sql turso> create table t(x PRIMARY KEY, data); turso> explain update t set data = 'some_data' where x > 50 and x < 60; addr opcode p1 p2 p3 p4 p5 comment ---- ----------------- ---- ---- ---- ------------- -- ------- 0 Init 0 15 0 0 Start at 15 1 OpenWrite 0 2 0 0 root=2; iDb=0 2 OpenWrite 1 3 0 0 root=3; iDb=0 3 Integer 50 1 0 0 r[1]=50 -- direct index seek 4 SeekGT 1 14 1 0 key=[1..1] ``` Reviewed-by: Preston Thorpe <preston@turso.tech> Closes tursodatabase#3728
- fixes tursodatabase#3732 Reviewed-by: Pere Diaz Bou <pere-altea@homail.com> Closes tursodatabase#3733
We merged two concurrent fixes to `nchange` handling last night and AFAICT the fix in tursodatabase#3692 was incorrect because it doesn't count UPDATEs in cases where the original row was DELETEd as part of the UPDATE statement. The correct fix was in 87434b8
…om Jussi Saurio Closes tursodatabase#2158 Closes tursodatabase#3702
…Text types' from Jussi Saurio ## Background Simulator wants to create predicates that it knows will be Greater or Less than some known value. It uses `LTValue` and `GTValue` for generating these. ## Problem Current implementation simply decrements or increments a random char by 1, and can thus generate strings with control characters like null terminators that result in parse errors, as seen in e.g. this CI run htt ps://github.com/tursodatabase/turso/actions/runs/18459131141/job/5258630 5749?pr=3702 of PR tursodatabase#3702 EDIT: I realized the _actual_ problem is in `GTValue` when it decides to make the string longer, it uses a random char value from `0..255` which can include null terminators etc. Fixed that too. I think in general this PR's approach is a bit more predictable so let's keep it. ## Solution Restrict string mutations to ascii string characters so that the mutation always results in another ascii string character. Closes tursodatabase#3708
We merged two concurrent fixes to `nchange` handling last night and AFAICT the fix in tursodatabase#3692 was incorrect because it doesn't count UPDATEs in cases where the original row was DELETEd as part of the UPDATE statement. The correct fix was in 87434b8 EDIT: okay, it's not strictly _incorrect_ in tursodatabase#3692 I guess, I just think it's more intuitive to increment the change for UPDATE in the `insert` opcode because that's what performs the actual update. Closes tursodatabase#3735
I was sampling our performance and noticed that In the throughput test, we were only setting the `PRAGMA synchronous = full` in `setup_database` and not in the connection. Also we were not setting `PRAGMA fullfsync = true`. Now SQLite numbers and Turso numbers are much closer: `Turso,1,1000,0,215095.94` vs `SQLite,1,1000,0,219748.39` Specs: M2 Air 8gb related: tursodatabase#3027 Closes tursodatabase#3734
Closes tursodatabase#3687 . Previously, the `try_fold_expr_to_i64` function casted `NULL` as `0` when evaluating expressions in `LIMIT` or `OFFSET` clauses. I removed this function since evaluating the expression directly and relying on the MustBeInt operation for casting seems to handle everything. Closes tursodatabase#3695
The previous implementation of CompletionGroup::add() would filter out
successfully-finished completions:
if !completion.finished() || completion.failed() {
self.completions.push(completion.clone());
}
This caused a problem when combined with drain() in the calling code.
Completions that were already finished would be removed from the source
vector by drain() but not added to the group, effectively losing track
of them.
This breaks the invariant that all completions passed to a group must
be tracked, regardless of their state. The build() method already
handles finished completions correctly by not including them in the
outstanding count.
The fix is to always add all completions and let build() handle their
state appropriately, matching the behavior of the old io_yield_many!()
macro.
The previous implementation of CompletionGroup would call the group's
callback function directly when the last completion finished:
if prev == 1 {
let group_result = group.result.get().and_then(|e| *e);
(group.complete)(group_result.map_or(Ok(0), Err));
}
This broke nested completion groups because parent groups track their
children via the Completion::callback() method. By calling the function
pointer directly, we bypassed the completion chain and parent groups
never received notification that their child had completed.
The fix stores a reference to the group's own Completion object in
self_completion during build(). When the last child finishes, we call
group_completion.callback() instead of invoking the function directly.
This properly propagates through the completion hierarchy, ensuring
parent groups decrement their outstanding count and eventually complete.
This matches the behavior of individual completions and maintains the
invariant that all completions notify their parents through the unified
callback() mechanism.
- Add more statements per iteration - Allow interactive transaction to contain multiple statements - add VERBOSE flag to print all statements executed in a successful iteration
Test started executing another statement when previous statement returned IO the last time and didn't run to completion
MVCC does not support statement-level rollback.
We don't want something like `BEGIN IMMEDIATE` to start a subtransaction, so instead we will open it if: - Statement is write, AND a) Statement has >0 table_references, or b) The statement is an INSERT (INSERT doesn't track table_references in the same way as other program types)
Every transaction was reading page 1 from the WAL to check the schema cookie in op_transaction, causing unnecessary WAL lookups. This commit caches the schema_cookie in Pager as AtomicU64, similar to how page_size and reserved_space are already cached. The cache is updated when the header is read/modified and invalidated in begin_read_tx() when WAL changes are detected from other connections. This matches SQLite's approach of caching frequently accessed header fields to avoid repeated page 1 reads. Improves write throughput by 5% in our benchmarks. Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com> Closes tursodatabase#3727
…m Jussi Saurio ## Gist This PR implements _statement subtransactions_, which means that a single statement within an interactive transaction can individually be rolled back. ## Background The default constraint violation resolution strategy in SQLite is `ABORT`, which means to rollback the statement that caused the conflict. For example: ```sql CREATE TABLE t(x UNIQUE); INSERT INTO t VALUES (1); BEGIN; INSERT INTO t VALUES (2),(3); -- ok INSERT INTO t VALUES (4),(1); -- conflict on 1, this statement should rollback INSERT INTO t VALUES (5); -- ok COMMIT; -- ok SELECT * FROM t; 1 2 3 5 ``` So far we haven't been able to support this due to lack of support for subtransactions, and have used the `ROLLBACK` strategy, which means to rollback the entire transaction on any constraint error. ## Problem Although PRIMARY KEY and UNIQUE constraints allow defining the conflict resolution strategy (e.g. `id INTEGER PRIMARY KEY ON CONFLICT ROLLBACK`), FOREIGN KEY violations do not support this: they always use `ABORT` i.e. statement subtransaction rollback. For this reason alone it is important to implement this mechanism now rather than later, since we already have FOREIGN KEY support implemented. ## Details This PR implements statement subtransactions with _anonymous savepoints_. This means that whenever a statement begins, it will open a new savepoint which will write "page undo images" into a temporary file called a _subjournal_. Whenever the statement marks a page as dirty, it will write the before-image of the page into the subjournal so that its modifications can be undone in the event of an ABORT (statement rollback). - Right now, only anonymous savepoints are supported, so the explicit `SAVEPOINT` syntax is not. - Due to the above, there can be only one savepoint open per pager, and this is enforced with assertions. - The subjournal file is currently entirely in memory. If it were not, we would either have to block on IO or refactor many usages of code to account for potentially pending completions. - Constraint errors no longer cause transactions to abort nor do they cause the page cache to be cleared - instead, subjournaled pages will be brought back into the page cache which effectively handles the same behavior albeit more fine-grained. Reviewed-by: Preston Thorpe <preston@turso.tech> Closes tursodatabase#3792
This PR implements simple heap-sort approach for query plans like `SELECT ... FROM t WHERE ... ORDER BY ... LIMIT N` in order to maintain small set of top N elements in the ephemeral B-tree and avoid sort and materialization of whole dataset. I removed all optimizations not related to this particular change in order to make branch lightweight. Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com> Closes tursodatabase#3726
Open a connection per transaction in the rusqlite benchmark so that we're comparing the same workload with Turso.
If WAL is already enabled, let's just continue execution instead of erroring out.
Include database path name for debuggability.
…Preston Thorpe closes tursodatabase#3811 adds `text_cache` which owns the null terminated bytes, which get cached if a subsequent call to `sqlite3_column_text` is made. tursodatabase#3809 depends on this fix Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com> Closes tursodatabase#3817
If WAL is already enabled, let's just continue execution instead of erroring out. Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com> Reviewed-by: Pere Diaz Bou <pere-altea@homail.com> Closes tursodatabase#3819
…nchmark' from Pekka Enberg Open a connection per transaction in the rusqlite benchmark so that we're comparing the same workload with Turso. Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com> Closes tursodatabase#3816
…nberg Include database path name for debuggability. Closes tursodatabase#3818
… Enberg Reviewed-by: Nikita Sivukhin (@sivukhin) Closes tursodatabase#3824
bit-aloo
force-pushed
the
2025-10-18-make-maybeint-int-conversion-strict
branch
from
October 24, 2025 10:38
5ae224d to
b2769af
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.