netatalk
netatalk copied to clipboard
Netatalk
SQLite CNID backend
Port the sqlite CNID backend from netatalk-classic (netatalk 2.x based) to netatalk 4.x.
@NJRoadfan This is close to working now, FYI.
Two outstanding issues:
The directory that houses the sqlite cnid databases (e.g. /var/lib/netatalk/CNID needs to have 777 permissions since the database client is embedded in afpd which runs as a normal user (obviously) when a user authenticates and attempts to initialize the volume. We may need a helper daemon akin to cnid_dbd to run the database client as root.
But more importantly, there's a crash in dircache_add() when it tries to enumerate the volume, so something isn't lining up quite yet.
Nonetheless, the backend builds and starts up now, and can generate a little cnid sqlite database file that you can query, which is neat.
Here is what comes up when trying to rebuild an existing volume with dbd -f:
Nov 14 18:53:18.614566 dbd[5231] {cnid_sqlite.c:100} (error:Default): sqlite3_bind_text(db->cnid_add_stmt, 1, stmt_param_name, strlen(stmt_param_name), SQLITE_STATIC) failed: No such file or directory
Here is a dump of the database, it doesn't get very far:
PRAGMA foreign_keys=OFF;
BEGIN TRANSACTION;
CREATE TABLE volumes ( VolUUID CHAR(32) PRIMARY KEY,VolPath TEXT(4096),Stamp BINARY(8),Depleted INT);
INSERT INTO volumes VALUES('830A28BBB084683384019DD280C51FA8','/srv/A2SERVER/A2FILES','n�6g',0);
CREATE TABLE `830A28BBB084683384019DD280C51FA8`(Id INTEGER PRIMARY KEY AUTOINCREMENT,Name VARCHAR(255) NOT NULL,Did INTEGER NOT NULL,DevNo INTEGER NOT NULL,InodeNo INTEGER NOT NULL,UNIQUE (Did, Name), UNIQUE (DevNo, InodeNo));
DELETE FROM sqlite_sequence;
INSERT INTO sqlite_sequence VALUES('`830A28BBB084683384019DD280C51FA8`',16);
CREATE INDEX idx_volpath ON volumes(VolPath);
COMMIT;
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
~~This is on hold while I clarify the license grant for the mysql CNID module that this is based on.~~
Resolved.
For the record, the Arch failure is due to their dependency tree being broken at the moment.
https://bbs.archlinux.org/viewtopic.php?id=306080
The cnid_sqlite_add operation works now. But there are still plenty of bugs to work through.
PRAGMA foreign_keys=OFF;
BEGIN TRANSACTION;
CREATE TABLE volumes ( VolUUID CHAR(32) PRIMARY KEY,VolPath TEXT(4096),Stamp BINARY(8),Depleted INT);
INSERT INTO volumes VALUES('56027931E1405C87F7C97E56D21DBA8F','/opt/afplite','*ICh',0);
CREATE TABLE IF NOT EXISTS "56027931E1405C87F7C97E56D21DBA8F"(Id INTEGER PRIMARY KEY AUTOINCREMENT,Name VARCHAR(255) NOT NULL,Did INTEGER NOT NULL,DevNo INTEGER NOT NULL,InodeNo INTEGER NOT NULL,UNIQUE (Did, Name), UNIQUE (DevNo, InodeNo));
INSERT INTO "56027931E1405C87F7C97E56D21DBA8F" VALUES(17,'TheVolumeSettingsFolder',2,16777241,70839489);
INSERT INTO "56027931E1405C87F7C97E56D21DBA8F" VALUES(18,'Network Trash Folder',2,16777241,70839491);
INSERT INTO "56027931E1405C87F7C97E56D21DBA8F" VALUES(19,'Trash Can Usage Map',18,16777241,70839493);
INSERT INTO "56027931E1405C87F7C97E56D21DBA8F" VALUES(20,'Trash Can #2',18,16777241,70839495);
INSERT INTO "56027931E1405C87F7C97E56D21DBA8F" VALUES(21,'AppleShare 3.sit.1',2,16777241,70839552);
DELETE FROM sqlite_sequence;
INSERT INTO sqlite_sequence VALUES('56027931E1405C87F7C97E56D21DBA8F',21);
CREATE INDEX idx_volpath ON volumes(VolPath);
COMMIT;
@NJRoadfan @cheesestraws This is ready for initial review. Basic file operations work for me, but there are several failing spectests that need to be worked through.
The sqlite backend is built by default when the sqlite3 dependency is satisfied. To enable the backend for a volume, set cnid scheme = sqlite.
A known limitation is that the dir that houses the sqlite db needs to be writable by all users. The meson install script sets this up for you.
I'm looking forward to you pointing out all of the obvious mistakes I've made. :-D
@NJRoadfan dbd -f does indeed not work still, but now with errors like
Jun 07 19:01:15.813359 dbd[61676] {cnid_sqlite.c:616} (error:CNID): cnid_sqlite_add: sqlite query error: datatype mismatch
Agreed, a migration path would be crucial for deprecating BDB.
BTW, I don't think that the mysql backend supports a migration path either, does it?
Anyways, I consider the migration path a fast-follow feature. I want to stabilize the base functionality and get this very long-running PR merged sooner rather than later!
What dbd -f does is calls cnid_wipe() then scans/rebuilds the CNID from scratch.
I touched up cnid_sqlite_wipe() a bit and can confirm that the db gets wiped when invoked through dbd, but there obviously some piece missing to restore the db to a good state afterwards. I'll keep working on it later tonight.
@NJRoadfan the dbd -f db wipe should be working now, although I have only tested a few trivial cases so far.
There’s a tricky bug right now where when you copy multiple files in one batch into a subdirectory, only the first file gets written into the database. This does not happen when copying multiple files into the root directory.
~~I suspect some kind of concurrency situation, so I will attempt to refactor the code to use manually controlled SQLite transactions rather than autocommit that I’m relying on at the moment.~~
The actual bug was that the cnid_sqlite_resolve() function used a transient statement that caused some kind of conflict or race condition with the prepared statements used by other functions. The solution was to consistently use prepared statements for all CNID API functions, save for the initial setup of the sqlite database.
This practically works now.
The big question to figure out right now is how to make the database writable by any AFP user while avoiding 666 permissions on the sqlite db file. The one idea I have right now is to call become_root() before every SQL write query but this feels like a crude solution too.
Pulling this back into draft again. The current solution works great for a single threaded scenario, when a single afpd process accesses the sqlite database.
However, when multiple afpd forks opens the database, one of them will put a lock on it and prevent the others from writing to it.
The SQLite docs has this to say
... SQLite implements serializable transactions by actually serializing the writes. There can only be a single writer at a time to an SQLite database. There can be multiple database connections open at the same time, and all of those database connections can write to the database file, but they have to take turns. SQLite uses locks to serialize the writes automatically; this is not something that the applications using SQLite need to worry about.
But somehow the automatic serialization is not taking effect in our CNID implementation, and we encounter fatal cnid_sqlite_add: sqlite query error: database is locked errors.
More research will be required.
The joys of concurrency. What command is actually failing? Multiple SELECTs should work as long as its coming from separate database connections. Users shouldn't be calling CREATE or DROP tables either. I haven't taking an in-depth peek, but is any code using the COMMIT command?
Ref: https://www2.sqlite.org/cvstrac/wiki?p=DatabaseIsLocked
Maybe give WAL mode a try? https://til.simonwillison.net/sqlite/enabling-wal-mode Also: https://www.reddit.com/r/golang/comments/1988ch8/database_is_locked_when_writing_sqlite3_interface/mvxgmmy/
The failing command is an INSERT (the cnid_put_stmt prepared statement). This is a sample log if you're curious. It is curious to note that the logs suggest that .DS_Store doesn't exist in the database at the moment of error, which it actually seems to do when you dump the database.
Jun 16 21:57:35.774255 afpd[25653] {cnid_sqlite.c:756} (maxdebug:CNID): cnid_sqlite_add(id: 34, did: 2, name: "manual.pdf", hint: 0): END
Jun 16 21:57:35.781940 afpd[25653] {cnid_sqlite.c:609} (maxdebug:CNID): cnid_sqlite_add(did: 2, name: "manual.pdf", hint: 34): BEGIN
Jun 16 21:57:35.782715 afpd[25653] {cnid_sqlite.c:415} (maxdebug:CNID): cnid_sqlite_lookup(did: 2, name: "manual.pdf"): BEGIN
Jun 16 21:57:35.783324 afpd[25653] {cnid_sqlite.c:476} (maxdebug:CNID): cnid_sqlite_lookup: id=34, did=2, name=manual.pdf, dev=16777239, ino=72929441
Jun 16 21:57:35.783884 afpd[25653] {cnid_sqlite.c:580} (maxdebug:CNID): cnid_sqlite_lookup(id: 34, did: 2, name: "manual.pdf"): END
Jun 16 21:57:35.784459 afpd[25653] {cnid_sqlite.c:632} (debug:CNID): cnid_sqlite_add: lookup returned id=34, errno=93
Jun 16 21:57:35.785035 afpd[25653] {cnid_sqlite.c:756} (maxdebug:CNID): cnid_sqlite_add(id: 34, did: 2, name: "manual.pdf", hint: 34): END
Jun 16 21:57:36.384288 afpd[25653] {cnid_sqlite.c:609} (maxdebug:CNID): cnid_sqlite_add(did: 2, name: "manual.pdf", hint: 34): BEGIN
Jun 16 21:57:36.385143 afpd[25653] {cnid_sqlite.c:415} (maxdebug:CNID): cnid_sqlite_lookup(did: 2, name: "manual.pdf"): BEGIN
Jun 16 21:57:36.386338 afpd[25653] {cnid_sqlite.c:476} (maxdebug:CNID): cnid_sqlite_lookup: id=34, did=2, name=manual.pdf, dev=16777239, ino=72929441
Jun 16 21:57:36.386896 afpd[25653] {cnid_sqlite.c:580} (maxdebug:CNID): cnid_sqlite_lookup(id: 34, did: 2, name: "manual.pdf"): END
Jun 16 21:57:36.387818 afpd[25653] {cnid_sqlite.c:632} (debug:CNID): cnid_sqlite_add: lookup returned id=34, errno=93
Jun 16 21:57:36.388336 afpd[25653] {cnid_sqlite.c:756} (maxdebug:CNID): cnid_sqlite_add(id: 34, did: 2, name: "manual.pdf", hint: 34): END
Jun 16 21:57:36.973773 afpd[25081] {cnid_sqlite.c:609} (maxdebug:CNID): cnid_sqlite_add(did: 2, name: ".DS_Store", hint: 33): BEGIN
Jun 16 21:57:36.975327 afpd[25081] {cnid_sqlite.c:415} (maxdebug:CNID): cnid_sqlite_lookup(did: 2, name: ".DS_Store"): BEGIN
Jun 16 21:57:36.975928 afpd[25081] {cnid_sqlite.c:449} (debug:CNID): cnid_sqlite_lookup: name: '.DS_Store', did: 2 is not in the CNID database
Jun 16 21:57:36.976524 afpd[25081] {cnid_sqlite.c:580} (maxdebug:CNID): cnid_sqlite_lookup(id: 0, did: 2, name: ".DS_Store"): END
Jun 16 21:57:36.976962 afpd[25081] {cnid_sqlite.c:632} (debug:CNID): cnid_sqlite_add: lookup returned id=0, errno=0
Jun 16 21:57:36.977211 afpd[25081] {cnid_sqlite.c:648} (debug:CNID): cnid_sqlite_add: binding name='.DS_Store', did=2, dev=16777239, ino=72929430
Jun 16 21:57:36.977786 afpd[25081] {cnid_sqlite.c:705} (error:CNID): cnid_sqlite_add: sqlite query error: database is locked
Jun 16 21:57:36.978256 afpd[25081] {cnid_sqlite.c:756} (maxdebug:CNID): cnid_sqlite_add(id: 0, did: 2, name: ".DS_Store", hint: 33): END
Jun 16 21:57:36.978731 afpd[25081] {dircache.c:442} (error:AFPDaemon): dircache_add(): did:0 is less than the allowed 17. Try rebuilding the CNID database for: ".DS_Store"
And no, I hardly use the COMMIT command at all, but rather rely on SQLite's autocommit.
And yes, WAL journaling mode is activated. See the cnid_sqlite_open() function right after opening the database.
As a side note, modifying the cnid_dbd daemon to support SQLite might be the safest choice. It was designed literally to solve this concurrency problem.
@NJRoadfan I've refactored the code to use BEGIN + COMMIT / ROLLBACK consistently for destructive transactions, rather than relying on SQLite's automatic transaction handling. This doesn't resolve the database lock issue unfortunately, but it's probably an improvement nonetheless.
I also added some timeout and retry logic for good measure.
@NJRoadfan Read/write access by concurrent users works now. The key change was to explicitly call sqlite3_reset() on the prepared statement in the cleanup step of every CNID function call that uses a prepared statement. Not entirely intuitively, but it turns out that bookending a query with BEGIN / COMMIT (or ROLLBACK) doesn't necessarily end the transaction.
This documentation page was helpful in understanding the behavior: https://www.sqlite.org/lang_transaction.html
~~There's a bug where moving a file from one folder to another folder leads to an undefined error in the logs. This doesn't have any visible impact on the AFP client however. The CNID record gets updated correctly.~~
It was an easy fix. I was using the wrong prepared statement in cnid_sqlite_get().
Jun 21 19:01:03.756417 afpd[45368] {cnid_sqlite.c:877} (maxdebug:CNID): cnid_sqlite_get(did: 21, name: "mar-0.2.0-tc6.68k"): BEGIN
Jun 21 19:01:03.757512 afpd[45368] {cnid_sqlite.c:897} (error:Default): sqlite3_bind_int64(db->cnid_resolve_stmt, 2, ntohl(did)) failed: Undefined error: 0
Jun 21 19:01:03.758526 afpd[45368] {cnid_sqlite.c:911} (maxdebug:CNID): cnid_sqlite_get(id: 0, did: 21, name: "mar-0.2.0-tc6.68k"): END
Jun 21 19:01:03.767506 afpd[45368] {cnid_sqlite.c:926} (maxdebug:CNID): cnid_sqlite_resolve(id: 29): BEGIN
Jun 21 19:01:03.768353 afpd[45368] {cnid_sqlite.c:953} (debug:CNID): cnid_sqlite_resolve: resolved did: 2, name: "asdf"
Jun 21 19:01:03.769116 afpd[45368] {cnid_sqlite.c:955} (maxdebug:CNID): cnid_sqlite_resolve(): END
Jun 21 19:01:03.773238 afpd[45368] {cnid_sqlite.c:926} (maxdebug:CNID): cnid_sqlite_resolve(id: 21): BEGIN
Jun 21 19:01:03.774480 afpd[45368] {cnid_sqlite.c:953} (debug:CNID): cnid_sqlite_resolve: resolved did: 2, name: "mar-0.2.0 Folder"
Jun 21 19:01:03.775249 afpd[45368] {cnid_sqlite.c:955} (maxdebug:CNID): cnid_sqlite_resolve(): END
~~The spectest suite runs immensely better now, but we still have 9 failing tests out of the set that passes with the mysql backend.~~
These were fixed by improving error handling, notably using the CNID_DBD_RES_NOTFOUND error code in the correct spots.
Failed tests: FPGetFileDirParms:test58: folder 1 (DIRDID_ROOT_PARENT) FPGetFileDirParms:test132: GetFilDirParams errors FPGetSrvrParms:test316: GetSrvrParms for a volume with option nostat set FPOpenDir:test57: OpenDir call FPSetDirParms:test82: test set dir parameters FPSetDirParms:test351: change root folder unix perm FPSetDirParms:test356: change root folder unix perm FPSetFileDirParms:test350: change root folder perm
Not tested tests (setup step failed): FPGetFileDirParms:test396: dir root attribute
The full spectest is now running against the sqlite backend in CI jobs, and all tests are passing. Even those that are consistently failing with the mysql backend.
~~The final thing I want to test before considering this shippable, is that the CNID depletion logic works correctly. Namely that when hitting CNID=0xffffffff (32 bit integer) then we reset the CNID back to 17 (0-16 are reserved) and start incrementing from there again. I can't think of a way to comfortably copy over 4 billion files over AFP, so temporarily using a smaller depletion trigger is probably good enough. :)~~
Ran tests with the threshold of CNID > 128 and it works as expected, with no disruption to the AFP clients. It functions akin to a wipe, but unlike "dbd -f" the new CNIDs are assigned lazily, and the CNID_SQLITE_FLAG_DEPLETED set so that existing appledouble CNID hints are ignored. It's probably advisable to run a wipe with "dbd -f" as soon as possible after a depletion to get a complete set of CNID assignments plus restore appledouble hints.
I have been tied up with some other matters the past few weeks. I'm going to try this out when I have a chance, namely migrating an existing share from BDB to SQLite along with exercising the ad tool suite. I know some of the tools have checks to only run with bdb shares, but I have been successful in using the tools with those checks disabled and the MySQL backend.
@NJRoadfan Would it make sense to merge this now, and roll forward with any fixes for ad compatibility that we discover later?
I wouldn't worry about the ad tool suite right now. There is a check in the volume open function that checks for BDB only. The priority is making sure volume migration from one of the other CNID back ends to SQLite works properly and safely. I would strongly suggest that the SQLite backend be labelled experimental at first given its importance to afpd working correctly.
I was thinking along the same lines. But leaning towards a "beta" label with a notice about taking backups of data before using it. And perhaps also throw a Warning level log message when initializing the backend.
@NJRoadfan Pushed changes to libatalk to print a big warning message when loading the sqlite module, plus notes on the experimental state in the docs.
@NJRoadfan I'm aiming to merge this before the end of the week, unless you see any major blocker.
My plan is to get this experimental backend into the wild with v4.3 and get some data and feedback from users how it performs. Then in the meantime in the main branch start rejigging the bdb backend to swap out BerkeleyDB for SQLite. Once we feel comfortable about the reliability of the rejigged bdb backend, put out v5.0 that removes the BerkeleyDB dependency.
Thoughts?
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
6.5% Duplication on New Code
See analysis details on SonarQube Cloud