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

node:sqlite: support database.backup #55413

Open
topvis opened this issue Oct 16, 2024 · 8 comments · May be fixed by #56253
Open

node:sqlite: support database.backup #55413

topvis opened this issue Oct 16, 2024 · 8 comments · May be fixed by #56253
Labels
feature request Issues that request new features to be added to Node.js. sqlite Issues and PRs related to the SQLite subsystem.

Comments

@topvis
Copy link

topvis commented Oct 16, 2024

What is the problem this feature will solve?

no simple function to make backup of the sqlite database.

What is the feature you are proposing to solve the problem?

make a backup of sqlite database with a simple function like database.backup(filename);
refer to sqlite backup API: https://www.sqlite.org/backup.html

What alternatives have you considered?

No response

@topvis topvis added the feature request Issues that request new features to be added to Node.js. label Oct 16, 2024
@github-project-automation github-project-automation bot moved this to Awaiting Triage in Node.js feature requests Oct 16, 2024
@OnlyAviv OnlyAviv added the sqlite Issues and PRs related to the SQLite subsystem. label Oct 16, 2024
@OnlyAviv OnlyAviv moved this from Awaiting Triage to Triaged in Node.js feature requests Oct 16, 2024
@geeksilva97
Copy link
Contributor

geeksilva97 commented Oct 29, 2024

Hello @cjihrig . Would you have an opinion about this? This is something I'd like to try. Having some advice would be good.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 29, 2024

Seems like a useful feature to support.

One design issue I see is whether we would expose the individual functions that make up the online backup API, or provide a single backup() function. A single function would be simpler for most cases, but 1. I could imagine people requesting access to the individual functions as well, and 2. we almost certainly would not want to block the event loop for the entire duration of the backup.

@geeksilva97
Copy link
Contributor

@geeksilva97
Copy link
Contributor

geeksilva97 commented Dec 2, 2024

Seems like a useful feature to support.

One design issue I see is whether we would expose the individual functions that make up the online backup API, or provide a single backup() function. A single function would be simpler for most cases, but 1. I could imagine people requesting access to the individual functions as well, and 2. we almost certainly would not want to block the event loop for the entire duration of the backup.

I was wondering if we could go with the simpler approach and, if needed, expose individual functions.

Even though I'm a newbie I got a minimal implementation in C and I am about to turn it into C++. For the interface, I was thinking of something like

const backup = database.backup('backup-filename', {
  sourceDbName: 'sourceDb', // defaults to main
  targetDbName: 'targetDb', // defaults to main
});

In the beginning, I thought this could return a Promise. So when the backup finishes the Promise fulfills. If it fails the Promise rejects.

However, the progress of the backup might be something users want to know. So I wonder if it would be better to have Events like progress, finish, and error.


Another question: should this operation be cancellable?

@billywhizz
Copy link
Contributor

@topvis you should be able to use VACUUM INTO to do this as things stand? might not be quite right for what you need.

https://www.sqlite.org/lang_vacuum.html#vacuuminto

not really sure how you could do this on a separate thread. there is also serialize/deserialize as an alternative to VACUUM INTO or Backup API. they should be pretty easy to expose in node i think?

@topvis
Copy link
Author

topvis commented Dec 4, 2024

@topvis you should be able to use VACUUM INTO to do this as things stand? might not be quite right for what you need.

https://www.sqlite.org/lang_vacuum.html#vacuuminto

not really sure how you could do this on a separate thread. there is also serialize/deserialize as an alternative to VACUUM INTO or Backup API. they should be pretty easy to expose in node i think?

Thank you @billywhizz. I didn't know VACUUM INTO before. It is perfect for what I need.

@geeksilva97 geeksilva97 linked a pull request Dec 14, 2024 that will close this issue
@geeksilva97
Copy link
Contributor

Seems like a useful feature to support.
One design issue I see is whether we would expose the individual functions that make up the online backup API, or provide a single backup() function. A single function would be simpler for most cases, but 1. I could imagine people requesting access to the individual functions as well, and 2. we almost certainly would not want to block the event loop for the entire duration of the backup.

I was wondering if we could go with the simpler approach and, if needed, expose individual functions.

Even though I'm a newbie I got a minimal implementation in C and I am about to turn it into C++. For the interface, I was thinking of something like

const backup = database.backup('backup-filename', {
  sourceDbName: 'sourceDb', // defaults to main
  targetDbName: 'targetDb', // defaults to main
});

In the beginning, I thought this could return a Promise. So when the backup finishes the Promise fulfills. If it fails the Promise rejects.

However, the progress of the backup might be something users want to know. So I wonder if it would be better to have Events like progress, finish, and error.

Another question: should this operation be cancellable?

Since some methods look like those in better-sqlite3 I will keep this as close as possible.

@cjihrig, I'm struggling with knowing how to make it non-blocking. Would you happen to have any references I can look into? I think I will have to use that HandleWrap but I'm not sure.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 14, 2024

I'm struggling with knowing how to make it non-blocking.

SQLite itself is synchronous. To make it async, you'll have to use the thread pool - there should be a number of examples of this in the src/ directory for other async APIs. However, there are some other things to consider:

  • The DatabaseSync class is named that way to match things like the fs API where everything should be synchronous.
  • The bigger issue though is that to do things async, due to the way SQLite works you'll need to synchronize every access to the database, or possibly use a separate database connection.

Given all of that, plus the fact that VACUUM INTO exists (which I was not aware of, thanks @billywhizz), I think it will be significantly simpler to expose the individual backup APIs from SQLite (at least as a starting point).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. sqlite Issues and PRs related to the SQLite subsystem.
Projects
Development

Successfully merging a pull request may close this issue.

5 participants