Same-thread frame injection #695
Conversation
MarinPostma
force-pushed
the
same-thread-frame-injector
branch
8 times, most recently
from
1c52981 to
30ec997
Compare
psarna
left a comment
psarna
left a comment
There was a problem hiding this comment.
This PR is too vast for my tiny brain to review in one go, so I'll do it in stages. First batch of comments:
|
|
||
| Ok(()) | ||
| } else { | ||
| anyhow::bail!("failed to apply pages"); |
There was a problem hiding this comment.
do we bother updating is_txn here, should we?
There was a problem hiding this comment.
we don't bother, because this is caught as a fatal replication error anyway, and everything blows up
| OpenFlags::SQLITE_OPEN_READ_WRITE | ||
| | OpenFlags::SQLITE_OPEN_CREATE | ||
| | OpenFlags::SQLITE_OPEN_URI | ||
| | OpenFlags::SQLITE_OPEN_NO_MUTEX, |
There was a problem hiding this comment.
note: once we migrate to libsql crate, we need to audit that NO_MUTEX is still ok, because there's been discussions around which threading model we should use.
| impl Frame { | ||
| /// size of a single frame | ||
| pub const SIZE: usize = size_of::<FrameHeader>() + WAL_PAGE_SIZE as usize; | ||
| /// Owned version of a frame, on the heap |
There was a problem hiding this comment.
I'm missing the purpose of the new FrameMut and FrameBorrowed. Why do we need it? Feel free to also post the answer in the commit message
There was a problem hiding this comment.
sorry, only got back to this PR now. So basically, FrameBorrowed is a stack data structure that contains the frame's raw data. But since it's quite big, you don't really ever get an owned version. Instead, you get a Frame and a FrameMut that are the stack-allocated equivalents. Both deref to a FrameBorrowed, with the difference between FrameMut and Frame being that Frame is a shared pointer that is cheaply clonable, and FrameMut is an exclusive reference to the Frame.
MarinPostma
force-pushed
the
same-thread-frame-injector
branch
2 times, most recently
from
43c0012 to
2d645cd
Compare
| &TRANSPARENT_METHODS, | ||
| // safety: hook is dropped after connection | ||
| (), | ||
| DEFAULT_AUTO_CHECKPOINT, |
There was a problem hiding this comment.
Autocheckpoint should be preventively disabled in this case. If we somehow end up having 999 pages in WAL while recreating this table (e.g. because the user mistakenly dropped it), this call will trigger an autocheckpoint outside of our virtual WAL. And that we don't want, especially if we later introduce checkpoint logic that only allows checkpointing frames that were backed up in S3.
| let connection = self.connection.lock(); | ||
| connection.execute("INSERT INTO libsql_temp_injection VALUES (42)", ())?; | ||
| // force call to xframe | ||
| match connection.cache_flush() { |
There was a problem hiding this comment.
@MarinPostma did you observe xFrames not being called without it? I found it weird, the docs https://www.sqlite.org/c3ref/db_cacheflush.html suggest that it's for flushing pages mid-transaction, and a single insert is a whole transaction on its own. It should be flushed to disk the moment you finish calling execute. And rusqlite's cache_flush is just a wrapper over sqlite3_db_cacheflush.
There was a problem hiding this comment.
we are in the context of a transaction here, so the call to INSERT never causes xFrame to get called
There was a problem hiding this comment.
we are in the context of a transaction here
Ok, that's new, and proves I didn't follow the idea closely enough.. I see the begin_txn now. And I suppose we take it so that we can inject frames in multiple xFrames calls, but we need it to happen atomically anyway?
There was a problem hiding this comment.
Yea sir, this way we can apply very large transactions, such as snapshots
psarna
left a comment
psarna
left a comment
There was a problem hiding this comment.
Looks ok, although I'm still anxious if we never leave this new transaction hanging forever, preventing further injections and checkpoints. I suppose the idea is that either we apply a finite number of frames, or exit with a fatal error, right? Is there are non-fatal error that would leave a transaction hanging, or do we ever wait for the network in transaction context?
MarinPostma
force-pushed
the
same-thread-frame-injector
branch
2 times, most recently
from
e581066 to
742fe9d
Compare
MarinPostma
force-pushed
the
same-thread-frame-injector
branch
from
742fe9d to
f749b1d
Compare
This reverts commit ebb7ae9.
This reverts commit ebb7ae9.
A series of improvements to the frame injection process:
FrameAPI refactor to allow in-place frame header mutation (this one might need a bit of rework later on. We force the alignment of the whole frame when only the header needs to be aligned, meaning we have to perform a full copy on the frame each time. Thinking about it now, we can do better, work with unaligned frames, and mutate the header by writing it back to the frame data)