-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add updateHook #604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add updateHook #604
Conversation
the code looks good ! Isn't there any way to unregister the hook without registering a new one ? I suppose calling a js function on each row update has a significant performance cost, even if the function is empty.
the code looks good !
Thx
Isn't there any way to unregister the hook without registering a new one ?
Not according to the sqlite documentation. For the quite similar sqlite3_preupdate_hook the docs (first paragraph) say that you can disable that by passing NULL as a callback.
I suppose calling a js function on each row update has a significant performance cost, even if the function is empty.
Your're probably right.
But a typical use-case for the update hook is to enable it at the start of your application, maybe after loading your base data.
Then you keep that setup to track all changes to the db until your app is closed.
You'll very likely not ever enable the update hook if you have a workload that is affected by the performance cost of empty function calls. An alternative is also to close and open the database again to get rid of the update hook.
So, I'd like to stick to the official docs and not invent my own - but it's your call, I can also document that passing null will (most likely) disable it.
I also noticed that I need to somehow clean up the function pointer when the database connection is closed to not add a mem leak.
Also I should probably keep the original signature of callback(update, database_name, table_name, rowid) instead of omitting the database_name.
Will update the PR in the evening.
Also the CI job is failing weirdly (Puppeteer fails to start because of missing entryies in /sys), need to check why.
I opened a topic on the official sqlite forums: https://sqlite.org/forum/forumpost/652aef4747
Oh cool, I am curious as well. I also took a look at the c code for update_hook and preupdate_hook and with my limited knowledge of C did they look pretty similar. I also assume the pointer for the update_hook is null initially.
sgbeal
commented
Mar 12, 2025
I opened a topic on the official sqlite forums: https://sqlite.org/forum/forumpost/652aef4747
That's now addressed https://sqlite.org/forum/forumpost/5ccd29dc29: NULL is the correct way to unset those, it just wasn't documented for update hooks. The website's docs won't reflect this change until 3.50 approaches, but the behavior has been unchanged since that API was introduced, so can be relied upon in pre-3.50 versions.
Soo i investigated the CI action failure:
Looks like for some reason starting chrome in sandbox mode for the worker test fails.
Running the tests with RUN_WORKER_TEST_WITHOUT_PUPPETEER_SANDBOX=1 works. A sandbox is not required anyways as only code from this repository is tested and not arbitrary (well except all the npm deps 😄) code from the interwebs.
No idea why it fails with this PR but the last run of that action was 6months ago so maybe due to OS updates in Github runners.
@lovasoa any opinions, comments?
Can you rebase ? I removed the dependency to puppeteer altogether, and updated other dependencies
bd3f4e6 to
f3cff2b
Compare
removed the now obsolete env var from CI.yml and rebased to the latest master
A wrapper around sqlite3_update_hook. For now only as a low-level operation to Database. To be useful in projects it will probably need some wrapping in the worker but right now I have no idea yet how that should look.
Also release the callback function when the callback is removed or the database is closed. Include the previously omitted database name in the callback args as the sqlite callback does.
* Add updateHook A wrapper around sqlite3_update_hook. For now only as a low-level operation to Database. To be useful in projects it will probably need some wrapping in the worker but right now I have no idea yet how that should look. * Allow removing the updateHook callback Also release the callback function when the callback is removed or the database is closed. Include the previously omitted database name in the callback args as the sqlite callback does. --------- Co-authored-by: Erik Soehnel <erik.soehnel@gmail.com>
A wrapper around sqlite3_update_hook.
For now only as a low-level operation (method on
Database).To be useful in projects it will probably need some wrapping in the worker but right now I have no idea yet how that should look.
BTW great project. The code, while being older JS (haven't written any non ES6 code for years 😅) looks great. I had a blast extending it. Also the test suite is simple but super-fast 😮 👍
Also as I already wrote in #599, I'd like to add the
sqlite_column_table_namesfunction too (will create another issue).That should enable me to create an automatic change detection on any select query (knowing which tables where queried and thus when to re-run the query).
Please review and maybe merge if it fits in.