Skip to content
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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

geeksilva97
Copy link
Contributor

@geeksilva97 geeksilva97 commented Dec 14, 2024

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:

...If the source database is not an in-memory database, and the write is performed from within the same process as the backup operation and uses the same database handle (pDb), then the destination database (the one opened using connection pFile) is automatically updated along with the source.

Writes to an in-memory source database, or writes to a file-based source database by an external process or thread using a database connection other than pDb are significantly more expensive than writes made to a file-based source database using pDb (as the entire backup operation must be restarted...)

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Dec 14, 2024
@geeksilva97 geeksilva97 force-pushed the sqlite-backup branch 3 times, most recently from 6c61b4c to 9b80c2d Compare January 10, 2025 21:43
lib/sqlite.js Outdated Show resolved Hide resolved
@geeksilva97 geeksilva97 force-pushed the sqlite-backup branch 4 times, most recently from 632edd3 to 174ace5 Compare January 12, 2025 01:07
@geeksilva97 geeksilva97 changed the title [wip] sqlite: expose backup api sqlite: expose backup api Jan 12, 2025
@geeksilva97 geeksilva97 marked this pull request as ready for review January 12, 2025 01:29
@geeksilva97
Copy link
Contributor Author

geeksilva97 commented Jan 12, 2025

This comment mentions that DatabaseSync needs to have only sync operations (makes sense).

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

@geeksilva97 geeksilva97 force-pushed the sqlite-backup branch 3 times, most recently from 69a3bf3 to bd43083 Compare January 12, 2025 02:19
Copy link

codecov bot commented Jan 12, 2025

Codecov Report

Attention: Patch coverage is 81.59204% with 37 lines in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (808e6b3) to head (fa23c05).
Report is 41 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 81.59% 15 Missing and 22 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56253      +/-   ##
==========================================
+ Coverage   89.18%   89.19%   +0.01%     
==========================================
  Files         662      662              
  Lines      191759   192031     +272     
  Branches    36911    36968      +57     
==========================================
+ Hits       171017   171291     +274     
+ Misses      13613    13576      -37     
- Partials     7129     7164      +35     
Files with missing lines Coverage Δ
src/node_sqlite.h 70.00% <ø> (ø)
src/node_sqlite.cc 79.94% <81.59%> (+0.28%) ⬆️

... and 62 files with indirect coverage changes

@geeksilva97 geeksilva97 marked this pull request as draft January 12, 2025 13:43
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
if (try_catch.HasCaught()) {
Cleanup();

resolver->Reject(env()->context(), try_catch.Exception()).ToChecked();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably best to check try_catch.CanContinue() before calling Reject to check for fatal errors that are terminating the isolate.

Copy link
Contributor Author

@geeksilva97 geeksilva97 Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do that I think we need to let somehow the user know that the backup is happening even though the progress function is not called.

I added the CanContinue to check and ran the test below.

    await database.backup('database.bkp', {
      progress: () => {.throw 'something went wrong'; }
    });

For this progress function the CanContinue is true. The backup is finished but there's no progress information. I wonder if it might lead to confusion.

Maybe we can emit a warning or something?

src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
@geeksilva97
Copy link
Contributor Author

Thanks a lot, @jasnell

@geeksilva97 geeksilva97 marked this pull request as ready for review January 13, 2025 00:38
@geeksilva97
Copy link
Contributor Author

Hi hi @cjihrig . Could you share your thoughts?

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 14, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 14, 2025
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't made it through the changes in src/node_sqlite.cc yet, but left some comments. This also needs docs before landing.

test/parallel/test-sqlite-backup.mjs Outdated Show resolved Hide resolved
test/parallel/test-sqlite-backup.mjs Outdated Show resolved Hide resolved
src/env_properties.h Outdated Show resolved Hide resolved
test/parallel/test-sqlite-backup.mjs Outdated Show resolved Hide resolved
test/parallel/test-sqlite-backup.mjs Outdated Show resolved Hide resolved
test/parallel/test-sqlite-backup.mjs Outdated Show resolved Hide resolved
test/parallel/test-sqlite-backup.mjs Outdated Show resolved Hide resolved
test/parallel/test-sqlite-backup.mjs Outdated Show resolved Hide resolved
test/parallel/test-sqlite-backup.mjs Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
doc/api/sqlite.md Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
@geeksilva97 geeksilva97 force-pushed the sqlite-backup branch 2 times, most recently from 371f368 to a92e692 Compare January 16, 2025 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node:sqlite: support database.backup
5 participants