-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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
sqlite: expose backup api #56253
sqlite: expose backup api #56253
Conversation
6c61b4c
to
9b80c2d
Compare
632edd3
to
174ace5
Compare
174ace5
to
2aee11d
Compare
This comment mentions that The backup can be performed synchronously but I don't think it should be like that. I wonder if indeed this PR would be more suitable for async API. Any thoughts? Thanks in advance |
69a3bf3
to
bd43083
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56253 +/- ##
==========================================
- Coverage 89.21% 89.18% -0.03%
==========================================
Files 665 665
Lines 192514 192755 +241
Branches 37040 37084 +44
==========================================
+ Hits 171744 171909 +165
- Misses 13613 13648 +35
- Partials 7157 7198 +41
|
Yeah, I have no idea what to try. I asked friends of mine to run this test on their machines with non-administrator permissions (Windows 11) and it worked. |
Maybe worth pinging @nodejs/platform-windows for help. |
9a6f158
to
3c49571
Compare
Looks like Windows passed 🎉 https://ci.nodejs.org/job/node-test-binary-windows-js-suites/32505/ (other failed but seems unrelated) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks for the contribution o/
Commit Queue failed- Loading data for nodejs/node/pull/56253 ✔ Done loading data for nodejs/node/pull/56253 ----------------------------------- PR info ------------------------------------ Title sqlite: expose backup api (#56253) Author Edy Silva <[email protected]> (@geeksilva97) Branch geeksilva97:sqlite-backup -> nodejs:main Labels c++, author ready, sqlite Commits 19 - sqlite, test: expose sqlite online backup api - sqlite: resolve backup promise with total transferred pages - sqlite,test: test DatabaseSync.prototype.backup interface - sqlite: pass object to progress function instead of two args - fixup: improve tests - sqlite: update interface - doc: add backup docs - test: use temp dir to test invalid paths - doc: fixup: fix typos - fixup: improve formatting - fixup: centralize backup error handling - fixup: fix new env properties order - fixup: await rejects assertion - sqlite,test,doc: move backup implementation to a separate function - sqlite,doc: add more details about concurrent edits - sqlite,doc: use async/await in backup code example - sqlite: use proper functions to open/close database - test: close databases after test - sqlite: fix backup finalization when promise resolves Committers 1 - Edy Silva <[email protected]> PR-URL: https://github.com/nodejs/node/pull/56253 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/56253 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 14 Dec 2024 01:36:33 GMT ✔ Approvals: 2 ✔ - Colin Ihrig (@cjihrig): https://github.com/nodejs/node/pull/56253#pullrequestreview-2596944274 ✔ - Vinícius Lourenço Claro Cardoso (@H4ad): https://github.com/nodejs/node/pull/56253#pullrequestreview-2596917775 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-02-05T20:48:52Z: https://ci.nodejs.org/job/node-test-pull-request/65013/ - Querying data for job/node-test-pull-request/65013/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 56253 From https://github.com/nodejs/node * branch refs/pull/56253/merge -> FETCH_HEAD ✔ Fetched commits as 436723282d64..4c4d73b810fe -------------------------------------------------------------------------------- Auto-merging src/node_sqlite.cc Auto-merging src/node_sqlite.h [main e6561e9bdd] sqlite, test: expose sqlite online backup api Author: Edy Silva <[email protected]> Date: Sun Jan 12 21:56:43 2025 -0300 4 files changed, 461 insertions(+) create mode 100644 test/parallel/test-sqlite-backup.mjs Auto-merging src/node_sqlite.cc [main de3853950a] sqlite: resolve backup promise with total transferred pages Author: Edy Silva <[email protected]> Date: Sun Jan 12 23:49:34 2025 -0300 1 file changed, 9 insertions(+), 7 deletions(-) [main 27c17ed52d] sqlite,test: test DatabaseSync.prototype.backup interface Author: Edy Silva <[email protected]> Date: Mon Jan 13 00:01:06 2025 -0300 1 file changed, 88 insertions(+), 10 deletions(-) Auto-merging src/node_sqlite.cc [main 5a6196bb7e] sqlite: pass object to progress function instead of two args Author: Edy Silva <[email protected]> Date: Tue Jan 14 12:32:56 2025 -0300 3 files changed, 43 insertions(+), 8 deletions(-) [main 83fa046d6b] fixup: improve tests Author: Edy Silva <[email protected]> Date: Tue Jan 14 14:05:16 2025 -0300 1 file changed, 46 insertions(+), 58 deletions(-) Auto-merging src/node_sqlite.cc [main 0a61c96ed9] sqlite: update interface Author: Edy Silva <[email protected]> Date: Tue Jan 14 14:20:12 2025 -0300 3 files changed, 15 insertions(+), 17 deletions(-) [main 3636bebe9b] doc: add backup docs Author: Edy Silva <[email protected]> Date: Tue Jan 14 15:19:57 2025 -0300 1 file changed, 23 insertions(+) [main d8f89eae40] test: use temp dir to test invalid paths Author: Edy Silva <[email protected]> Date: Tue Jan 14 15:24:39 2025 -0300 1 file changed, 1 insertion(+), 1 deletion(-) [main 7b218e7c10] doc: fixup: fix typos Author: Edy Silva <[email protected]> Date: Wed Jan 15 20:54:56 2025 -0300 1 file changed, 3 insertions(+), 3 deletions(-) Auto-merging src/node_sqlite.cc [main a7534a16f1] fixup: improve formatting Author: Edy Silva <[email protected]> Date: Wed Jan 15 21:25:59 2025 -0300 1 file changed, 44 insertions(+), 71 deletions(-) Auto-merging src/node_sqlite.cc [main b9f65e3a8f] fixup: centralize backup error handling Author: Edy Silva <[email protected]> Date: Wed Jan 15 21:59:47 2025 -0300 3 files changed, 34 insertions(+), 45 deletions(-) [main c4967daff8] fixup: fix new env properties order Author: Edy Silva <[email protected]> Date: Mon Jan 20 14:12:42 2025 -0300 1 file changed, 3 insertions(+), 3 deletions(-) [main 5560c83169] fixup: await rejects assertion Author: Edy Silva <[email protected]> Date: Mon Jan 20 15:02:09 2025 -0300 1 file changed, 5 insertions(+), 5 deletions(-) Auto-merging src/node_sqlite.cc Auto-merging src/node_sqlite.h [main 33727815e2] sqlite,test,doc: move backup implementation to a separate function Author: Edy Silva <[email protected]> Date: Fri Jan 24 12:20:35 2025 -0300 6 files changed, 177 insertions(+), 149 deletions(-) [main d1c3534575] sqlite,doc: add more details about concurrent edits Author: Edy Silva <[email protected]> Date: Fri Jan 31 13:58:30 2025 -0300 1 file changed, 40 insertions(+), 2 deletions(-) [main 5cecd76b35] sqlite,doc: use async/await in backup code example Author: Edy Silva <[email protected]> Date: Mon Feb 3 13:14:34 2025 -0300 1 file changed, 14 insertions(+), 16 deletions(-) Auto-merging src/node_sqlite.cc [main dd2b54d22d] sqlite: use proper functions to open/close database Author: Edy Silva <[email protected]> Date: Mon Feb 3 18:07:10 2025 -0300 1 file changed, 5 insertions(+), 2 deletions(-) [main c680940ca9] test: close databases after test Author: Edy Silva <[email protected]> Date: Tue Feb 4 14:39:55 2025 -0300 1 file changed, 11 insertions(+), 1 deletion(-) Auto-merging src/node_sqlite.cc [main dd8154e385] sqlite: fix backup finalization when promise resolves Author: Edy Silva <[email protected]> Date: Tue Feb 4 23:57:54 2025 -0300 1 file changed, 3 insertions(+), 1 deletion(-) ✔ Patches applied There are 19 commits in the PR. Attempting autorebase. Rebasing (2/38) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- sqlite, test: expose sqlite online backup apihttps://github.com/nodejs/node/actions/runs/13167245589 |
Landed in 16dc29d |
PR-URL: #56253 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Closes #55413
This PR exposes the SQLite Online Backup API, which allows database backup.
The API is inspired by better-sqlite3 https://github.com/WiseLibs/better-sqlite3/blob/master/docs/api.md#backupdestination-options---promise.
Multithreading caveats
As long as writes come from the same process and handle (
sqlite*
), the backup will continue progressing as expected. Other than that, it can cause the backup process to restart. From docs: