From 44fdf80c2859cf45862a52080e62bb5abf19ed35 Mon Sep 17 00:00:00 2001 From: stephan Date: Sat, 22 Nov 2025 19:21:25 +0000 Subject: [PATCH] This combination of kvvfs callbacks seems to work okay for both persistent and transient storage, but it's unclear why/how, as xAccess() does not appear to be doing anything useful (and misbehaves when told to report that the storage object in question exists). These tests fail in 64-bit builds for as-yet-unknown reasons (other tests continue to pass in 64-bit). FossilOrigin-Name: 69e591d0054218ead789cee199e5258f1c378a89e4b7b0e38fe74e834e23156b --- ext/wasm/api/sqlite3-vfs-kvvfs.c-pp.js | 122 ++++++++++++++----------- ext/wasm/tester1.c-pp.js | 19 +++- manifest | 16 ++-- manifest.uuid | 2 +- src/os_kv.c | 26 +++--- 5 files changed, 105 insertions(+), 80 deletions(-) diff --git a/ext/wasm/api/sqlite3-vfs-kvvfs.c-pp.js b/ext/wasm/api/sqlite3-vfs-kvvfs.c-pp.js index d3aae3b384..e1ed474ff4 100644 --- a/ext/wasm/api/sqlite3-vfs-kvvfs.c-pp.js +++ b/ext/wasm/api/sqlite3-vfs-kvvfs.c-pp.js @@ -36,7 +36,7 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ hop = (o,k)=>Object.prototype.hasOwnProperty.call(o,k); const cache = Object.assign(Object.create(null),{ - rxJournalSuffix: /^-journal$/, // TOOD: lazily init once we figure out where + rxJournalSuffix: /-journal$/, // TOOD: lazily init once we figure out where zKeyJrnl: wasm.allocCString("jrnl"), zKeySz: wasm.allocCString("sz") }); @@ -334,23 +334,30 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ astack = wasm.scopedAllocPush(); try{ const store = storageForZClass(zClass); - if( !store ) return -1; if( 0 ){ - debug("xRcrdRead", nBuf, zClass, wasm.cstrToJs(zClass), - wasm.cstrToJs(zKey), store); + debug("xRcrdRead", wasm.cstrToJs(zClass), + wasm.cstrToJs(zKey), zBuf, nBuf, store ); } + if( !store ) return -1; const zXKey = keyForStorage(store, zClass, zKey); if(!zXKey) return -3/*OOM*/; const jV = store.s.getItem(wasm.cstrToJs(zXKey)); - if(!jV) return -1; + if(null===jV) return -1; const nV = jV.length /* We are relying 100% on v being ** ASCII so that jV.length is equal ** to the C-string's byte length. */; + if( 0 ){ + debug("xRcrdRead", wasm.cstrToJs(zXKey), store, jV); + } if(nBuf<=0) return nV; else if(1===nBuf){ wasm.poke(zBuf, 0); return nV; } + if( 0 ){ + debug("xRcrdRead", nBuf, zClass, wasm.cstrToJs(zClass), + wasm.cstrToJs(zKey), nV, jV, store); + } const zV = wasm.scopedAllocCString(jV) /* TODO: allocate a single 128kb buffer (largest page size) for reuse here, or maybe even preallocate @@ -360,7 +367,7 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ Number(zBuf), Number(zV), wasm.ptr.addn(zV, nBuf,- 1) ); wasm.poke(wasm.ptr.add(zBuf, nBuf), 0); - return nBuf - 1; + return nBuf; }catch(e){ error("kvrecordRead()",e); return -2; @@ -420,16 +427,17 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ if( n > kvvfsMethods.$nKeySize - 8 /*"-journal"*/ - 1 ){ warn("file name is too long:", wasm.cstrToJs(zName)); return capi.SQLITE_RANGE; + }else{ + wasm.poke32(pOutFlags, flags | sqlite3.SQLITE_OPEN_CREATE); } const rc = originalMethods.vfs.xOpen(pProtoVfs, zName, pProtoFile, flags, pOutFlags); if( 0==rc ){ const f = new KVVfsFile(pProtoFile); util.assert(f.$zClass, "Missing f.$zClass"); - const jzName = wasm.cstrToJs(zName); - const jzClass = wasm.cstrToJs(f.$zClass); + const jzClass = wasm.cstrToJs(zName);//f.$zClass); let s = cache.jzClassToStorage[jzClass]; - //debug("xOpen", jzName, jzClass, s); + //debug("xOpen", jzClass, s); if( s ){ ++s.refc; }else{ @@ -437,10 +445,10 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ around forever so that future xOpen()s get the same Storage-ish objects. We can accomplish that by simply increasing the refcount once more. */ - util.assert( !f.$isJournal, "Opening a journal before its db? "+jzName ); + util.assert( !f.$isJournal, "Opening a journal before its db? "+jzClass ); const other = f.$isJournal - ? jzName.replace(cache.rxJournalSuffix,'') - : jzName + '-journal'; + ? jzClass.replace(cache.rxJournalSuffix,'') + : jzClass + '-journal'; s = cache.jzClassToStorage[jzClass] = cache.jzClassToStorage[other] = Object.assign(Object.create(null),{ @@ -452,9 +460,9 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ s: new TransientStorage }); debug("xOpen installed storage handles [", - jzName, other,"]", s); + jzClass, other,"]", s); } - pFileHandles.set(pProtoFile, {s,f,n:jzName}); + pFileHandles.set(pProtoFile, {s,f,n:jzClass}); } return rc; }catch(e){ @@ -484,46 +492,49 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ xAccess:function(pProtoVfs, zPath, flags, pResOut){ try{ - if( 0 ){ + /** + In every test run to date, if this function sets + *pResOut to anything other than 0, the VFS fails to + function. Why that that is is a mystery. How it seems + to work despite never reporting "file found" is a + mystery. + */ + wasm.poke32(pResOut, 0); + return 0; + if( 1 ){ + /* This is fundamentally exactly what the native impl does. */ const jzName = wasm.cstrToJs(zPath); - const s = cache.jzClassToStorage[jzName]; - debug("xAccess", jzName, s); - let drc = 1; - wasm.poke32(pResOut, 0); - if( s ){ - /* This block is _somehow_ triggering an - abort() via xClose(). */ - if(0) debug("cache.zKeyJrnl...",wasm.cstrToJs(cache.zKeyJrnl), - wasm.cstrToJs(cache.zKeySz)); - drc = recordHandler.xRcrdRead( - zPath, - jzName.endsWith("-journal") - ? cache.zKeyJrnl - : cache.zKeySz, - wasm.ptr.null, 0 + const drc = recordHandler.xRcrdRead( + zPath, cache.rxJournalSuffix.test(jzName) + ? cache.zKeyJrnl + : cache.zKeySz, + wasm.ptr.null, 0 + ); + if( drc>0 ){ + wasm.poke32( + pResOut, 1 + /* This poke is triggering an abort via xClose(). + If we poke 0 then no problem... except that + xAccess() doesn't report the truth. Same effect + if we move that to the native impl + os_kv.c:kvvfsAccess(). */ ); - debug("xAccess", jzName, drc, pResOut); - if( drc>0 ){ - wasm.poke32( - pResOut, 1 - /* This poke is triggering an abort via xClose(). - If we poke 0 then no problem... except that - xAccess() doesn't report the truth. Same effect - if we move that to the native impl - os_kv.c:kvvfsAccess(). */ - ); - } } + debug("xAccess", jzName, drc, pResOut, wasm.peek32(pResOut)); return 0; + }else{ + const rc = originalMethods.vfs.xAccess( + pProtoVfs, zPath, flags, pResOut + /* This one is only valid for local/session storage. */ + ); + if( 0 && !!wasm.peek32(pResOut) ){ + /* The native impl, despite apparently hitting the + right data on this side of the call, never sets + pResOut to anything other than 0. */ + debug("xAccess *pResOut", wasm.cstrToJs(zPath), wasm.peek32(pResOut)); + } + return rc; } - const rc = originalMethods.vfs.xAccess( - pProtoVfs, zPath, flags, pResOut - /* This one is only valid for local/session storage */ - ); - if( 0 && 0===rc ){ - debug("xAccess pResOut", wasm.peek32(pResOut)); - } - return rc; }catch(e){ error('xAccess',e); return capi.SQLITE_ERROR; @@ -532,8 +543,6 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ //#if nope xFullPathname: function(pVfs, zPath, nOut, zOut){}, - xDlOpen: function(pVfs, zFilename){}, - xSleep: function(pVfs,usec){}, xCurrentTime: function(pVfs,pOutDbl){}, xCurrentTimeInt64: function(pVfs,pOutI64){}, //#endif @@ -579,7 +588,7 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ } return 0; }catch(e){ - warn("xClose",e); + error("xClose",e); return capi.SQLITE_ERROR; } }, @@ -717,15 +726,18 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ }; if( sqlite3.__isUnderTest ){ - jdb.prototype.testDbToObject = function(){ + jdb.prototype.testDbToObject = function(includeJournal=false){ const store = cache.jzClassToStorage[this.affirmOpen().filename]; + const rx = includeJournal ? undefined : /^kvvfs(-(local|session))?-jrnl$/; if( store ){ const s = store.s; const rc = Object.create(null); let i = 0, n = s.length; for( ; i < n; ++i ){ - const k = s.key(i), v = s.getItem(k); - rc[k] = v; + const k = s.key(i); + if( !rx || !rx.test(k) ){ + rc[k] = s.getItem(k); + } } return rc; } diff --git a/ext/wasm/tester1.c-pp.js b/ext/wasm/tester1.c-pp.js index 17a905e11b..956690e716 100644 --- a/ext/wasm/tester1.c-pp.js +++ b/ext/wasm/tester1.c-pp.js @@ -2913,6 +2913,7 @@ globalThis.sqlite3InitModule = sqlite3InitModule; }/*kvvfs sanity checks*/) .t({ name: 'transient kvvfs', + //predicate: ()=>false, test: function(sqlite3){ const filename = 'localThread' /* preinstalled instance */; const JDb = sqlite3.oo1.JsStorageDb; @@ -2935,18 +2936,26 @@ globalThis.sqlite3InitModule = sqlite3InitModule; db = new JDb(filename); db.exec('insert into kvvfs(a) values(4),(5),(6)'); T.assert(6 === db.selectValue('select count(*) from kvvfs')); - console.debug("kvvfs to Object:",db.testDbToObject()); + console.debug("kvvfs to Object:",db.testDbToObject(true)); close(); db = new JDb('new-storage'); db.exec(sqlSetup); T.assert(3 === db.selectValue('select count(*) from kvvfs')); - console.debug("kvvfs to Object:",db.testDbToObject()); + console.debug("kvvfs to Object:",db.testDbToObject(true)); const n = db.storageSize(); T.assert( n>0, "Db size count failed" ); - close(); - T.mustThrow(function(){ + if( 1 ){ + // Concurrent open of that same name uses the same storage + const x = new JDb(db.filename); + T.assert(3 === db.selectValue('select count(*) from kvvfs')); + x.close(); + } + close(); + // When the final instance of a name is closed, the storage + // disappears... + T.mustThrowMatching(function(){ /* Ensure that 'new-storage' was deleted when its refcount went to 0. TODO is a way to tell these instances to hang around after that, such that 'new-instance' could @@ -2958,7 +2967,7 @@ globalThis.sqlite3InitModule = sqlite3InitModule; }finally{ ddb.close(); } - }, "Expecting new-storage to be empty."); + }, /.*no such table: kvvfs.*/); }finally{ if( db ) db.close(); } diff --git a/manifest b/manifest index 4eb4fa510f..80cae3eacd 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C Fix\sa\smisinteraction\sof\sCREATE/READONLY\sflags\sin\sthe\sOPFS\sVFS. -D 2025-11-22T17:27:16.675 +C This\scombination\sof\skvvfs\scallbacks\sseems\sto\swork\sokay\sfor\sboth\spersistent\sand\stransient\sstorage,\sbut\sit's\sunclear\swhy/how,\sas\sxAccess()\sdoes\snot\sappear\sto\sbe\sdoing\sanything\suseful\s(and\smisbehaves\swhen\stold\sto\sreport\sthat\sthe\sstorage\sobject\sin\squestion\sexists).\sThese\stests\sfail\sin\s64-bit\sbuilds\sfor\sas-yet-unknown\sreasons\s(other\stests\scontinue\sto\spass\sin\s64-bit). +D 2025-11-22T19:21:25.670 F .fossil-settings/binary-glob 61195414528fb3ea9693577e1980230d78a1f8b0a54c78cf1b9b24d0a409ed6a x F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea @@ -600,7 +600,7 @@ F ext/wasm/api/sqlite3-api-worker1.c-pp.js 1041dd645e8e821c082b628cd8d9acf70c667 F ext/wasm/api/sqlite3-license-version-header.js 0c807a421f0187e778dc1078f10d2994b915123c1223fe752b60afdcd1263f89 F ext/wasm/api/sqlite3-opfs-async-proxy.js 9654b565b346dc609b75d15337f20acfa7af7d9d558da1afeb9b6d8eaa404966 F ext/wasm/api/sqlite3-vfs-helper.c-pp.js 3f828cc66758acb40e9c5b4dcfd87fd478a14c8fb7f0630264e6c7fa0e57515d -F ext/wasm/api/sqlite3-vfs-kvvfs.c-pp.js 31f6241415322d5e52bca4137a63f1f942f61a301041ac99209cc1ae7be0e780 +F ext/wasm/api/sqlite3-vfs-kvvfs.c-pp.js bd461cf2e1c35aab71ce43906b514d9f5bb348cbbcadf0bac46d53a3ab84af66 F ext/wasm/api/sqlite3-vfs-opfs-sahpool.c-pp.js 26cb41d5a62f46a106b6371eb00fef02de3cdbfaa51338ba087a45f53028e0d0 F ext/wasm/api/sqlite3-vfs-opfs.c-pp.js 88ce2078267a2d1af57525a32d896295f4a8db7664de0e17e82dc9ff006ed8d3 F ext/wasm/api/sqlite3-vtab-helper.c-pp.js 9097074724172e31e56ce20ccd7482259cf72a76124213cbc9469d757676da86 @@ -647,7 +647,7 @@ F ext/wasm/test-opfs-vfs.html 1f2d672f3f3fce810dfd48a8d56914aba22e45c6834e262555 F ext/wasm/test-opfs-vfs.js 1618670e466f424aa289859fe0ec8ded223e42e9e69b5c851f809baaaca1a00c F ext/wasm/tester1-worker.c-pp.html 0e432ec2c0d99cd470484337066e8d27e7aee4641d97115338f7d962bf7b081a F ext/wasm/tester1.c-pp.html 52d88fe2c6f21a046030a36410b4839b632f4424028197a45a3d5669ea724ddb -F ext/wasm/tester1.c-pp.js 9b353e5fa54452f5b08d07176bf62240d4d2237bc8c71cfa259b1a7ba1081424 +F ext/wasm/tester1.c-pp.js ff028364da670936db6b54d393d2c3672341c9285e210677a75f078c3bf0b5aa F ext/wasm/tests/opfs/concurrency/index.html 657578a6e9ce1e9b8be951549ed93a6a471f4520a99e5b545928668f4285fb5e F ext/wasm/tests/opfs/concurrency/test.js d08889a5bb6e61937d0b8cbb78c9efbefbf65ad09f510589c779b7cc6a803a88 F ext/wasm/tests/opfs/concurrency/worker.js 0a8c1a3e6ebb38aabbee24f122693f1fb29d599948915c76906681bb7da1d3d2 @@ -717,7 +717,7 @@ F src/notify.c 57c2d1a2805d6dee32acd5d250d928ab94e02d76369ae057dee7d445fd64e878 F src/os.c 509452169d5ea739723e213b8e2481cf0e587f0e88579a912d200db5269f5f6d F src/os.h 1ff5ae51d339d0e30d8a9d814f4b8f8e448169304d83a7ed9db66a65732f3e63 F src/os_common.h 6c0eb8dd40ef3e12fe585a13e709710267a258e2c8dd1c40b1948a1d14582e06 -F src/os_kv.c f11812682d83d125fb25c34db44d3091e4171662e087080ad2067203cc0606aa +F src/os_kv.c 653b3825dc63dd6a83b04814df4bda28effde43d5f94f3611648701d04749fc3 F src/os_setup.h 8efc64eda6a6c2f221387eefc2e7e45fd5a3d5c8337a7a83519ba4fbd2957ae2 F src/os_unix.c 7945ede1e85b2d1b910e1b4af9ba342e964b1e30e79f4176480a60736445cb36 F src/os_win.c a89b501fc195085c7d6c9eec7f5bd782625e94bb2a96b000f4d009703df1083f @@ -2178,8 +2178,8 @@ F tool/version-info.c 33d0390ef484b3b1cb685d59362be891ea162123cea181cb8e6d2cf6dd F tool/warnings-clang.sh bbf6a1e685e534c92ec2bfba5b1745f34fb6f0bc2a362850723a9ee87c1b31a7 F tool/warnings.sh d924598cf2f55a4ecbc2aeb055c10bd5f48114793e7ba25f9585435da29e7e98 F tool/win/sqlite.vsix deb315d026cc8400325c5863eef847784a219a2f -P 61606be2ae2b0d73cdcd7947a77c7ad87cdf850bba90b0c3e3cdf8c02177db73 -R 54ce506656ff5a47f677e534b68299f2 +P 0b53be562f1e1a5b20ffe8d72df64e753a8d759b580d949a0f32409144769bb0 +R 18ff16459471e0fa48753f413430772f U stephan -Z 7d3474159eeb14b7002c4398431607cf +Z 100d31fa602c6a076823477a2cb4e392 # Remove this line to create a well-formed Fossil manifest. diff --git a/manifest.uuid b/manifest.uuid index 34dda8185e..0b625bdcad 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -0b53be562f1e1a5b20ffe8d72df64e753a8d759b580d949a0f32409144769bb0 +69e591d0054218ead789cee199e5258f1c378a89e4b7b0e38fe74e834e23156b diff --git a/src/os_kv.c b/src/os_kv.c index feb3536f4c..305962dd09 100644 --- a/src/os_kv.c +++ b/src/os_kv.c @@ -21,7 +21,7 @@ ** Debugging logic */ -/* SQLITE_KV_TRACE() is used for tracing calls to kvstorage routines. */ +/* SQLITE_KV_TRACE() is used for tracing calls to kvrecord routines. */ #if 0 #define SQLITE_KV_TRACE(X) printf X #else @@ -535,6 +535,9 @@ static int kvvfsClose(sqlite3_file *pProtoFile){ pFile->isJournal ? "journal" : "db")); sqlite3_free(pFile->aJrnl); sqlite3_free(pFile->aData); +#ifdef SQLITE_WASM + memset(pFile, 0, sizeof(*pFile)); +#endif return SQLITE_OK; } @@ -920,17 +923,18 @@ static int kvvfsAccess( SQLITE_KV_LOG(("xAccess(\"%s\")\n", zPath)); #if 0 && defined(SQLITE_WASM) /* - ** Bug somewhere: if this method sets *pResOut to non-0 then - ** xClose() is abort()ing via free(). The symptoms are the same - ** whether we set *pResOut from here or from JS. + ** This is not having the desired effect in the JS bindings. + ** It's ostensibly the same logic as the #else block, but + ** it's not behaving that way. + ** + ** In JS we map all zPaths to Storage objects, and -journal files + ** are mapped to the storage for the main db (which is is exactly + ** what the mapping of "local-journal" -> "local" is doing). */ - if( 0==sqlite3_strglob("*-journal", zPath) ){ - *pResOut = - sqlite3KvvfsMethods.xRcrdRead(zPath, "jrnl", 0, 0)>0; - }else{ - *pResOut = - sqlite3KvvfsMethods.xRcrdRead(zPath, "sz", 0, 0)>0; - } + const char *zKey = (0==sqlite3_strglob("*-journal", zPath)) + ? "jrnl" : "sz"; + *pResOut = + sqlite3KvvfsMethods.xRcrdRead(zPath, zKey, 0, 0)>0; #else if( strcmp(zPath, "local-journal")==0 ){ *pResOut = -- 2.47.3