-
Notifications
You must be signed in to change notification settings - Fork 142
Add MySQL, MariaDB, PostgreSQL, SQLite support #24
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
base: main
Are you sure you want to change the base?
Conversation
nicotsx
left a comment
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.
Thank you @altendorfme this is a good first stab! I left you a few comments to review
| if ( | ||
| formValues.backend === "nfs" || | ||
| formValues.backend === "smb" || | ||
| formValues.backend === "webdav" || | ||
| formValues.backend === "mariadb" || | ||
| formValues.backend === "mysql" || | ||
| formValues.backend === "postgres" | ||
| ) { |
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.
Maybe we could make this a constant ["nfs", "smb", ...] and here refactor to if SUPPORTS_CONNECTION.includes(formValues.backend) {} as this condition is becoming to big
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.
As SQLite is just a file, I'm wondering if it makes any sense to add it as a "volume". Users can simply use the local directory feature. I would remove it
| const volumePath = getVolumePath(volume); | ||
| let backupPath: string; | ||
| let dumpFilePath: string | null = null; | ||
| const isDatabase = isDatabaseVolume(volume); | ||
|
|
||
| if (isDatabase) { | ||
| logger.info(`Creating database dump for volume ${volume.name}`); | ||
|
|
||
| const timestamp = Date.now(); | ||
| dumpFilePath = getDumpFilePath(volume, timestamp); | ||
|
|
||
| try { | ||
| await executeDatabaseDump(volume.config as DatabaseConfig, dumpFilePath); | ||
| logger.info(`Database dump created at: ${dumpFilePath}`); | ||
| } catch (error) { | ||
| logger.error(`Failed to create database dump: ${toMessage(error)}`); | ||
| throw error; | ||
| } | ||
|
|
||
| backupPath = dumpFilePath; | ||
| } else { | ||
| backupPath = getVolumePath(volume); | ||
| } |
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.
I see this becoming hard to maintain if we ever add more types. Instead, I suggest you add a new abstract method in the xxx-backend.ts files for getVolumePath() and then here you. can simply call it the same way for all possible backends.
By doing this, you can get rid of all the 4 functions you have added in helpers.ts
app/server/utils/spawn.ts
Outdated
| resolve({ | ||
| exitCode: -1, | ||
| stdout: stdoutData, | ||
| stderr: stderrData, | ||
| }); | ||
| reject(error); |
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.
Why changing this? The whole purpose of the safeSpawn is to not throw and avoid needless try catches when using it
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.
Sorry, this is the kind of change we make when some things break and we don't know why, haha!
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.
This should probably live in each respective backend file instead of a big everything helper
app/server/utils/spawn.ts
Outdated
| // Write stdin if provided | ||
| if (stdin && child.stdin) { | ||
| child.stdin.write(stdin); | ||
| child.stdin.end(); | ||
| } |
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.
You don't seem to be using this at the moment. Could be useful but no need to clutter the code with it if we don't need it now
Dockerfile
Outdated
| mariadb-client \ | ||
| mysql-client \ | ||
| postgresql-client \ |
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.
Bun ships with all 3 of these clients https://bun.com/docs/runtime/sql could you explore if it is possible to replace the usage of those clients with a direct connection from javascript and running a raw query for dumping?
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.
Hum! I'll try it, I didn't know about this route!
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.
I was reading the comparisons, and the native clients have full support for tables, views, triggers, procedures, and functions, and the backup process is safer and more stable for long processing times.
|
I removed SQLite support. |
|
|
|
Will merge this asap! Looks great |
|
Hello @altendorfme The more I tested the implementation the more I started thinking that maybe we are not the right tool to integrate Database backups. For a few reasons mainly:
Overall I think this complexifies the code too much while the user could simply run dump executions from a script on the host and backup that folder with Zerobyte. What do you think? I'm sorry you've probably spent some time here |
|
To avoid this conflict with volumes, could I handle and separate them in an environment called Source or Database? |
|
I think we should maybe start a discussion and check with the community how this feature would be used. This way we make sure that we get the implementation right from the get go I'll start a thread tomorrow (it's late here) and ping you |
Hi @nicotsx, excuse me for jumping in. Speaking from a PostgreSQL perspective at least, your instincts are correct. Furthermore, it's common to restore databases to an exact point in time, which means you should make sure to back up WAL files as well in a consistent way, otherwise you're risking data loss or corruption. If you'd like to cover database backups with Zerobyte, I'd always suggest going native. I haven't been using SQLite much, but before backing it up, I'd make sure there are no writers to the file. |
I added MySQL, MariaDB, PostgreSQL, and SQLite as volume options, removed the files tab, and adjusted the listings.
I also had to add the client to the Dockefile to support dumping
I think this structure can still be improved; I don't know if volumes are a better place or if I should create a new Sources environment