Virtualize WAL methods #53

psarna · 2022-10-21T15:10:44Z

This preparatory commit moves all WAL routines to a virtual table.
Also, a helper libsql_open() function is provided. It allows passing
a new parameter - name of the custom WAL methods implementation.

psarna · 2022-10-30T17:16:40Z

v2:

with wasm: drop SQLITE_OMIT_WAL flag #60 in mind, virtual WAL implementations now also declare if they rely on shared memory

src/main.c

haaawk · 2022-11-11T14:39:19Z

src/main.c

 rc = SQLITE_ERROR;
 }
+ *ppWal = libsql_wal_methods_find(zWal);
+ if (*ppWal == NULL) {


NIT: Do we want to keep the formatting consistent?

not a fan of this style, but it's trivial to fix here, so let's :)

haaawk · 2022-11-11T14:57:09Z

src/sqlite.h.in

+** ^Names are case sensitive.
+** ^Names are zero-terminated UTF-8 strings.
+** ^If there is no match, a NULL pointer is returned.
+** ^If zVfsName is NULL then the default implementation is returned.


Nit: s/zVfsName/zName/

src/pager.c

haaawk · 2022-11-11T15:13:39Z

src/pager.c

 ** various SQLITE_IO_XXX errors.
 */
 int sqlite3PagerOpen(
 sqlite3_vfs *pVfs, /* The virtual file system to use */


Ultra nit: formatting of comments got destroyed.

src/pager.c

haaawk · 2022-11-11T15:36:13Z

src/pager.c

 if( iLimit>=-1 ){
 pPager->journalSizeLimit = iLimit;
- sqlite3WalLimit(pPager->pWal, iLimit);
+ if (pagerUseWal(pPager)) {


Why do we have to check whether pWal is NULL after virtualising the WAL interface?

Previously, sqlite3WalLimit simply checked if pPager->pWal is not null and that was sufficient. Now it is not, because WAL support can even be compiled out by defining SQLITE_OMIT_WAL, and then pWalMethods is null and we can't call the virtual interface, because it does not exist. An alternative would be to spray #ifdefs on each call site, but I find if + pagerUseWal more readable.

ah wait, it's not enough, with SQLITE_OMIT_WAL this code won't compile, because the pWalMethods field is not even there. I'll recompile with SQLITE_OMIT_WAL and double check all the missing places, and fix accordingly

src/wal.c

haaawk · 2022-11-11T15:52:10Z

src/wal.h

+** An open write-ahead log file is represented by an instance of the
+** following object.
+*/
+struct Wal {


Would it be possible to make this opaque for upper layers (Pager)? It seems that a lot of details are being exposed. Can this become a problem for some very exotic WAL implementations?

Pager doesn't assume anything about struct Wal, it used to be opaque before this patch. To be honest, I don't remember why this struct is itself exposed, as we should only care about exposing libsql_wal_methods to the user. Perhaps it's an artifact from previous iterations that should just be removed

Ok, I refreshed my knowledge a little bit - wal.h is still a header private to libSQL files and no module assumes anything about it. In particular, pager.c does not include it. It is however needed for people who implement their own WAL routines, because their own implementations need to know the WAL structure, that's why this definition is exposed in a header.

haaawk · 2022-11-11T15:55:00Z

src/pager.c

- Wal *pWal; /* Write-ahead log used by "journal_mode=wal" */
- char *zWal; /* File name for write-ahead log */
+ Wal *pWal; /* Write-ahead log used by "journal_mode=wal" */
+ libsql_wal_methods *pWalMethods; /* Virtual methods for interacting with WAL */


Why do we need to store pWalMethods here when pWal already has them in pMethods field ?

Related to the comment above - Wal used to (and I'm pretty sure still should be) opaque to upper layers, pager included. And once struct Wal is opaque, we need to somehow propagate WAL methods from libsql_open down to WAL, and pager is the only way through.

as above - struct Wal is still opaque to its upper layers (pager), but it's not opaque for the authors of custom virtual WAL implementations

haaawk

LGTM
I left some nits and questions just to prove I've actually read all that code :)

psarna · 2022-11-11T19:20:15Z

Thanks for reviewing, I need to take a fresh look at this PR next week and validate - e.g. I'm pretty sure moving struct Wal definition from .c file to a public .h header is unnecessary and we should avoid it indeed

This preparatory commit moves all WAL routines to a virtual table. Also, a helper libsql_open() function is provided. It allows passing a new parameter - name of the custom WAL methods implementation.

Before enabling WAL journaling, libSQL detects whether WAL is fully supported. Historically it meant either operating in exclusive mode or supporting shared memory locks in the underlying VFS, but it may not be the case with custom WAL implementations. Thus, custom WAL methods can declare whether they rely on shared memory - if not, it will also not get verified when checking for WAL support.

It makes little sense to store the WAL file name if the virtual WAL implementation is not based on a single file. Therefore, WAL pathname handling is also virtualized, with the original implementation still producing a <dbname>-wal file.

This commit adds a stub implementation of custom WAL methods in ext/vwal subdirectory. It can be used as a starting point for implementing custom WAL routines.

The test case registers a new naive virtual WAL coded on top of Rust's std::collections::HashMap. The WAL implementation only covers reading and writing pages, without checkpointing or savepoints, but it's enough to validate that the virtual method table works. After registering custom WAL, a few simple operations are performed on the database to validate that pages were indeed stored and retrieved properly. For extra logs, run the test with -- --nocapture.

The command is heavily inspired by .vfslist and lists all the registered custom WAL methods.

These are customarily run early, before a call to libsql_open, so it makes sense to auto-initialize.

With this patch applied, WAL methods can be registered from a loadable extension module dynamically.

Not critical, as WAL methods would customarily live as long as the program that runs it, but it's good practice to be able to unregister during a cleanup.

WAL is open lazily on first access, while it is useful to be able to set up ground for it even before the main db file is open. This optional hook allows it.

Similarly to how other interfaces work, the version number in WAL methods lets the user know which functions are supported, and which aren't yet.

The test now properly sets the iVersion and pre-main-db-open hook.

ABI should be consistent regardless of the compilation options, so the optional functions are in there anyway - they can be simply set to nulls if the user did not compile support for them in libSQL.

It will be useful for any state that custom WAL methods could potentially have.

penberg · 2022-12-20T10:27:25Z

@psarna I think this is good to go. Should we document this in doc/libsql_extensions.md?

psarna · 2022-12-20T15:16:55Z

@penberg yes, a docs entry is definitely needed. I'll provide one soon, and then we can proceed with this PR

In amalgamation mode it compiled just fine, but otherwise it relies on the wal.h header now.

The paragraph briefly explains the newly introduced mechanism.

psarna · 2022-12-20T21:12:07Z

@penberg the checks are still ongoing, but I wrote the doc entry, green light from my end

53: Rename crate folders to match crate names r=psarna a=MarinPostma It is good practice to have the crate name match that of its folder, this is what people expect, and is less surprising when working with cargo commands, such as the `-p` flag to specify a target package Co-authored-by: ad hoc <postma.marin@protonmail.com>

psarna requested a review from penberg October 21, 2022 15:10

psarna force-pushed the virtual_wal branch 10 times, most recently from 7d9b46e to 14b7ed6 Compare October 25, 2022 12:04

psarna mentioned this pull request Oct 30, 2022

wasm: drop SQLITE_OMIT_WAL flag #60

Merged

psarna force-pushed the virtual_wal branch from 998ed1a to 621dcae Compare October 30, 2022 17:15