WallHook v2 #517

MarinPostma · 2023-10-27T09:40:22Z

No description provided.

libsql-sqlite3/src/btree.h

psarna · 2023-10-30T07:37:10Z

libsql-sqlite3/src/loadext.c

@@ -517,12 +517,7 @@ static const sqlite3_api_routines sqlite3Apis = {
  sqlite3_stmt_explain
 };

-static const libsql_api_routines libsqlApis = {


that's a delicate change since libsqlApis is public. If we want to nuke them it's better to do it now rather than later, but we need to clearly document and announce that, in case we have external users of the virtual WAL api.

psarna · 2023-10-30T07:38:06Z

libsql-sqlite3/src/main.c

@@ -1238,6 +1238,9 @@ static int sqlite3Close(sqlite3 *db, int forceZombie){
  */
  sqlite3VtabRollback(db);

+  /* Destroy the create wal */
+  /* (db->create_wal.xDestroy)(db->create_wal.self); */


Is it commented out because it's a draft and it's going to be back later? Or are we changing the way wal is destroyed? In any case, we need a FIXME/TODO comment placed here

This is the quirk I wanted to talk about. With shared pagers, the ownership of the create_wal is shared as well, because pagers are lifted to the global scope. So the connection does not necessarily owns the create_wal. This is only ok for now because there is nothing to clean with the sqlite3_create_wal. I think we need to have that reference counted, but maybe this is done another way in C?

libsql-sqlite3/src/pager.h

psarna · 2023-10-30T07:43:48Z

libsql-sqlite3/src/wal.c

@@ -247,13 +247,28 @@
 ** that correspond to frames greater than the new K value are removed
 ** from the hash table at this point.
 */
-#include "sqliteInt.h"
+#include <sqlite3.h>


nope - now you risk it will be taken from one of the standard paths like /usr/include/, and it's not what you want here. Why is this header needed? If just for a few types => forward-declare them

libsql-sqlite3/src/wal.c

psarna · 2023-10-30T07:47:59Z

libsql-sqlite3/src/wal.c

+** already be opened on connection pDbFd. The buffer that zWalName points
+** to must remain valid for the lifetime of the returned Wal* handle.
+**
+** Custom virtual methods may be provided via the pMethods parameter.


this comment is out of date right?

yes yes, I haven't started looking at documentation, just on getting something in a working state

psarna · 2023-10-30T07:51:37Z

libsql-sqlite3/src/wal.c

+** an SQLite error code is returned and *ppWal is left unmodified.
+*/
+static int sqlite3WalOpen(
+  void *self,


I don't follow why this pointer is type-erased, but we must at least name it anyway. Similarly, sqlite3_file object is never really instantiated, but instead cast to a specific type before it's used, e.g. a unix file. I can't suggest a name for it before I understand what self is here, isn't it just an implementation of a specific WAL, so something like libsql_wal_impl *? This type can be public and still be opaque, same as sqlite3_file, you just declare it in one of the public headers as

typedef struct libsql_wal_impl libsql_wal_impl;

We can't afford void * here because this type name also serves as documentation

self is the method receiver. It's whatever state the WAL needs to function. In the case of sqlite3, this is a sqlite3_wal.

I don't get the analogy with sqlite3_file? the sqlite3_file has a pointer to the methods, but self can really be anything. whatever is returned by xOpen

ping: instead of void, it should be a named opaque type, something like libsql_wal_impl *, defined as typedef struct libsql_wal_impl *libsql_wal_impl.

psarna · 2023-10-30T07:53:25Z

libsql-sqlite3/src/wal.h

+
+struct libsql_wal {
+    libsql_wal_methods methods; /* virtual wal methods */
+    void* self; /* methods receiver */


right, so it's a pointer-to-implementation. In that case let's name it.

I need a translation here 😄

psarna · 2023-10-30T07:55:55Z

libsql-sqlite3/src/wal.h

+  ** the WAL file is open. It is a good place for initialization routines, as WAL
+  ** is otherwise open lazily.
+  */
+  int (*xPreMainDbOpen)(void* self, const char *main_db_path, int main_db_path_len);


Side note: since we overhaul the interface, we should consider removing this thing altogether, it makes very little sense. I thought I need it long time ago when bottomless was developed as a loadable module, but even then, it makes 10000x more sense to just expose an initialization method that users are supposed to call before a database is open, not a weird WAL methods hook. So if we decide on the shape of virtual WAL v2, it should also include nuking this xPreMainDbOpen hook.

Each request for new frames from a replica to a primary resets the idle timer but such request is a long poll. It won't return until it gets some new frame. This means that a replica won't send another request for new frames in the mean-time and won't restart the idle timer. This commit makes primary keep track of replicas waiting for new frames and prevents IdleShutdownLayer from stopping the primary if there is at least one replica still waiting for the new frames. When a replica is put into sleep, it will drop the connection and the connected_replicas counter will be decreased so only live replicas can prevent primary from going to sleep. Fixes tursodatabase#506 Signed-off-by: Piotr Jastrzebski <[email protected]>

psarna · 2023-11-02T09:52:06Z

libsql-sqlite3/src/pager.c

    }
-    memset((void*)&(pPager->wal), 0, sizeof(libsql_wal));
+    destroy_create_wal(&pPager->create_wal);


that one should only be called if pagerUseWal(pPager), right? So it should be moved one line up

The create wal is always there, but a wal is only created if the wal journaling mode is enabled. So we always need to destroy it.

libsql-sqlite3/src/wal.c

libsql-server/src/replication/primary/replication_logger_wal.rs

This WAL implementation dispatches between a Bottomless wal and a regular LoggerWal

MarinPostma changed the title ~~wip~~ WallHook v2 Oct 27, 2023

MarinPostma force-pushed the wal-hook-v2 branch 2 times, most recently from 5d21da9 to 3c75c70 Compare October 29, 2023 17:46

psarna reviewed Oct 30, 2023

View reviewed changes

MarinPostma force-pushed the wal-hook-v2 branch 3 times, most recently from dbf9519 to cf0d62a Compare October 30, 2023 17:29

psarna requested changes Nov 2, 2023

View reviewed changes

MarinPostma force-pushed the wal-hook-v2 branch 6 times, most recently from 7c7a3bf to 227ca89 Compare November 9, 2023 13:01

psarna reviewed Nov 10, 2023

View reviewed changes

libsql-server/src/replication/primary/replication_logger_wal.rs Outdated Show resolved Hide resolved

psarna reviewed Nov 10, 2023

View reviewed changes

libsql-server/src/replication/primary/replication_logger_wal.rs Outdated Show resolved Hide resolved

MarinPostma force-pushed the wal-hook-v2 branch 9 times, most recently from d58b5f6 to 977d9ea Compare November 15, 2023 09:58

MarinPostma mentioned this pull request Nov 15, 2023

Move ffi to libsql-ffi #624

Merged

MarinPostma force-pushed the wal-hook-v2 branch 3 times, most recently from b83f752 to 5ee3bcc Compare November 20, 2023 13:20

MarinPostma added 25 commits November 30, 2023 11:24

make libsql connection use create_wal

af812c9

use create wal connection in write proxy

65d8914

fix type aliases with create wal connections

4b4e505

Introduce ReplicationWal

30c149f

This WAL implementation dispatches between a Bottomless wal and a regular LoggerWal

Remove bottomless static initialization

a6d6855

use create wal when instanciating namespaces

0a1c5f9

fmt and clippy

60c4f96

remove libsql-sys walhook module

6523bf3

make make_create_wal public

d9c6c5d

re-export rusqlite from libsql-sys

82819aa

add wasmtime-bindings feature to libsql-sys

176d6c4

fix rust tests

100c52c

fix lockfile after rebase

31bf468

remove Deref impl for BottomlessWal

4c98234

fix lockfile

77ae43c

rename libsql_create_wal to libsql_wal_manager

ff53bcb

static sqlite3 wal

2125e63

restore deprecated libsql open methods, introduce v3

b426b23

post-rebase fixes

e1a912c

make pager's wal a pointer

05f6d47

static initialization of sqlite3wal rc

3d7c3c0

fix libsql_open usage in sys

49882a3

homogenise rust wal naming

49b11ab

fix review nits

4ae365b

fmt

fef6978

MarinPostma force-pushed the wal-hook-v2 branch from 4f6e81c to fef6978 Compare November 30, 2023 10:29

MarinPostma added this pull request to the merge queue Nov 30, 2023

Merged via the queue into main with commit 85279bd Nov 30, 2023
13 checks passed

MarinPostma deleted the wal-hook-v2 branch November 30, 2023 11:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WallHook v2 #517

WallHook v2 #517

MarinPostma commented Oct 27, 2023

psarna Oct 30, 2023

psarna Oct 30, 2023

MarinPostma Oct 30, 2023

psarna Oct 30, 2023

psarna Oct 30, 2023

MarinPostma Oct 30, 2023

psarna Oct 30, 2023

MarinPostma Oct 30, 2023

psarna Nov 27, 2023

psarna Oct 30, 2023

MarinPostma Oct 30, 2023

psarna Oct 30, 2023

psarna Nov 2, 2023

MarinPostma Nov 2, 2023

WallHook v2 #517

WallHook v2 #517

Conversation

MarinPostma commented Oct 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment