Closes#2241
## What
When an index interior cell is deleted, it steals the leaf cell with the
largest key in its left subtree, deletes the old interior cell and then
replaces it with the stolen cell. This ensures the binary-search-tree
aspect of the btree remains correct. However, this can cause a situation
where both are true:
1. The leaf page is now UNDERFULL and must be rebalanced
2. The leaf's IMMEDIATE parent page is now OVERFULL and must be
rebalanced
## Why is this a problem
We simply didn't support the case where:
- Leaf page P is unbalanced and rebalancing starts on it
- Its immediate parent is ALSO unbalanced and _overflows_.
We had an assertion against this happening (see #2241)
## The fix
Allow exactly 1 overflow cell in the parent under very particular
conditions:
1. The parent page must be an index interior page
2. The parent must be positioned exactly at the divider cell whose left
child page underflows
This is the _only_ case where the immediate parent of a page about to
undergo rebalancing can have overflow cells.
## Implementation details
The parent overflow cell is folded into `cell_array` fairly early on and
`parent.overflow_cells` is cleared. However we need to be careful with
`cell_idx` for dividers other than the overflow cell because they get
shifted left on the page in `drop_cell()`. I've added a long comment
about this.
## Testing
Adds fuzz test that does inserts and deletes on an index btree and
asserts that all the expected keys are found at the end in the right
order. This test runs into this case quite frequently so I was able to
verify it.
Reviewed-by: Pere Diaz Bou <pere-altea@homail.com>
Closes#2243
Let's make sure we don't end up in a weird situation by appending frames
one by one and we can later think of optimizations.
Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com>
Closes#2034
SQLite behavior is: if another connection has modified the DB when a
read tx starts, the connection beginning the transaction must clear its
page cache due to the potentiality of there being stale versions of
pages in it. Evidence of this here:
https://github.com/sqlite/sqlite/blob/ded1959/src/pager.c#L3258-L3260
and here:
https://github.com/sqlite/sqlite/blob/master/src/wal.c#L3368-L3370
In the future, we may want to do either:
1. a more granular invalidation logic for per-conn cache, or
2. a shared versioned page cache
But right now we must follow SQLite to make our current behavior not
corrupt data
Closes#2248Closes#2257
SQLite behavior is: if another connection has modified the DB when a
read tx starts, it must clear its page cache due to the potentiality
of there being stale versions of pages in it.
In the future, we may want to do either:
1. a more granular invalidation logic for per-conn cache, or
2. a shared versioned page cache
But right now we must follow SQLite to make our current behavior not
corrupt data
we run the simulator on aws every night for 4 hours, but for the past
few days it's been exiting with 137 and the parent sim-loop process
hasn't handled this case and also exited. i suspect the child process is
being killed due to memory consumption or something, but hard to be
sure. either way, let's not end the sim loop when this happens
Closes#2255
i suspect the child process is being killed due to memory consumption
or something, but hard to be sure. either way, let's not stop the sim-loop
parent process when this happens.
This PR addresses two related issues in the SQLite3 C API
implementation:
1. Properly sets error codes on the database object when handling SQL
preparation failures
2. Fixes C string handling in sqlite3_errstr_impl to ensure proper null-
termination
Error codes should be correctly set on the database object, making them
accessible to API consumers. Also fixes and issue where C strings
weren't properly null-terminated in error reporting functions, which
causes the issue in #2197 where messages all get pushed onto a single
line. Its possible that this also fixes a buffer overflow, but I'm not
actually sure what the behavior was.
Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com>
Closes#2212
This PR partially fixes issue when schema changes were invisible after
WAL sync calls. Now, `wal_insert_end` always read fresh schema cookie
and re-parse schema from scratch if cookie changed.
Generally, the problem of "silent" schema update can be more generic
if(when?) `turso-db` will support multi-process setup. But for now only
single-process can work with `turso-db`, so I decided to inject re-parse
logic explicitly in WAL raw API in order to not introduce any
unnecessary overhead in the ordinary execution path.
This fix is not complete, as if we will have already prepared statements
- they should be re-prepared too in case of schema changes. But this
problem already tracked in the PR
https://github.com/tursodatabase/turso/pull/2214
Reviewed-by: Pedro Muniz (@pedrocarlo)
Closes#2246
WAL insert API introduced in the #2231 works incorrectly as it never
mark inserted pages as dirty.
This PR fixes this issue and also add simple fuzz test which fails
without fixes.
Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com>
Closes#2245
Make `add_dirty` helper to set flag and add page to the dirt list. This
makes API safer as now its harder to do one thing and forget about
another (which can lead to DB corruption).
Reviewed-by: Pere Diaz Bou <pere-altea@homail.com>
Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com>
Closes#2244
The SetCookie opcode is used, among other things, to notify the
transaction of schema changes. We are not issuing it on DropTable.
Without it, the transaction thinks the schema hasn't changed, and does
not update the schema of the connection back to the database.
SQLite will, of course, issue it:
35 DropTable 0 0 0 foo 0
36 SetCookie 0 1 2 0
Unfortunately I don't have a unit test that breaks with this, because
the one that is supposed to break is having, let's put it this way,
bigger problems.
Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com>
Closes#2249
This PR adds a const associated value on the VTabModule trait,
`READONLY` defaulted to `true`, so we can bail early when a write
operation is done on an invalid vtable.
This prevents extensions from having to implement `insert`,`update`,
`delete` just to return `Error::ReadOnly`, and prevents us from having
to step through `VUpdate` just to error out.
Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com>
Closes#2247
The current code includes creating the database object, which is slow.
Unfortunately the same cannot be done on the standard SQLite.
Reviewed-by: Preston Thorpe (@PThorpe92)
Closes#2242
This PR adds support for `INSERT` queries with explicit value for
`rowid` column (not thought rowid alias):
```
turso> create table t(x, y, z);
turso> insert into t(rowid, x, y, z) values (10, 1, 2, 3);
turso> select rowid, * from t;
┌───────┬───┬───┬───┐
│ rowid │ x │ y │ z │
├───────┼───┼───┼───┤
│ 10 │ 1 │ 2 │ 3 │
└───────┴───┴───┴───┘
```
Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com>
Closes#2239
Before this update, the entire immutable record was **fully**
deserialized **every** time it was compared in the sorter.
This PR extends the sorter with incremental deserialization of record
keys, only when needed and only if they weren’t already deserialized in
a previous iteration.
I hate that we panic on failed deserialization in `cmp`, but
unfortunately, I can’t return `Result` as part of this interface.
Looking for feedback around a better way to handle this.
Alternatively, I could store the deserialization error as part of
`SortableImmutableRecord` and check it before returning the record in
`next`, thereby deferring the error handling. The downside of this
approach is that it complicates debugging, since the error will be
completely decoupled from the place where it occurs.
Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com>
Closes#2207
# Fix SUM aggregate function for mixed types
Fixes#2133
The SUM aggregate function was returning incorrect results when
processing tables with mixed numeric and non-numeric values. According
to SQLite documentation:
> "If any input to sum() is neither an integer nor a NULL, then sum()
returns a floating point value"
[*](https://sqlite.org/lang_aggfunc.html)
Now both SQLite and Turso yield the same output of 44.0.
--
I modified `Sum` to increment only for numeric values, skipping non-
numeric values. However, if we have mixed numeric values or non-numeric
values, we return a float output. Added a flag to keep track of it.
as pointed out by @FHaggs , If there are no non-NULL input rows then
sum() returns NULL but total() returns 0.0. I decided to include it in
this PR as well. Empty was such a natural test case.
Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com>
Closes#2182
The SetCookie opcode is used, among other things, to notify the
transaction of schema changes. We are not issuing it on DropTable.
Without it, the transaction thinks the schema hasn't changed, and does
not update the schema of the connection back to the database.
SQLite will, of course, issue it:
35 DropTable 0 0 0 foo 0
36 SetCookie 0 1 2 0
Unfortunately I don't have a unit test that breaks with this, because
the one that is supposed to break is having, let's put it this way,
bigger problems.
