Skip to content

Fixes and Improvements to the Backup/Restore SQLite Feature#818

Merged
gotson merged 4 commits intoxerial:masterfrom
pyckle:backup_restore_fix
Jan 5, 2023
Merged

Fixes and Improvements to the Backup/Restore SQLite Feature#818
gotson merged 4 commits intoxerial:masterfrom
pyckle:backup_restore_fix

Conversation

@pyckle
Copy link
Copy Markdown
Contributor

@pyckle pyckle commented Dec 1, 2022

BugFixes:
backup/restore sql commands run in jdbc fail silently if the backup or restore fails. The bugfix is that we check the return code status in JNI and throw an exception on failure. There is a unit test written to validate this
backup/restore progress observers were never updated. They are now updated if passed in via JNI. This is now unit tested.

Feature:
Various parameters to backup/restore can now be provided via jni rather than being hard coded within C.

JNI libs were rebuilt. I am happy to remove this commit if you'd prefer to build them.

Andrew Pikler and others added 2 commits December 1, 2022 03:51
fix(jdbc): SQLException is thrown on backup/restore failure
feat(native): SQLite backup/restore is more configurable from java
@gotson
Copy link
Copy Markdown
Collaborator

gotson commented Dec 2, 2022

Thanks, will have a look when i have some time. I was wondering why we do have that feature in the driver at all to be honest 🤔

@gotson gotson added enhancement:SQLite Enhancement about sqlite features impacts:JNI Has impact on JNI C code labels Dec 2, 2022
@pyckle
Copy link
Copy Markdown
Contributor Author

pyckle commented Dec 2, 2022

Thanks, will have a look when i have some time. I was wondering why we do have that feature in the driver at all to be honest thinking

Sure, no hurry.

I tend to agree that having regex in the driver that preprocesses a query and runs it through a completely different flow isn't great. It's also not at all portable - backup/restore aren't available in other jdbc drivers. At a minimum, if the backup/restore fails, the driver should throw a SQLException. I had a failing restore in my codebase and it took me a while to understand this is what was happening.

With that said, it makes sense to expose these APIs as it's a useful feature to be able to backup/restore SQLite databases. As it is SQLite specific, I think exposing it as a separate API makes more sense.

Also, while I parameterized some constants, I think that more customization may be necessary for many backup use cases. See https://www.sqlite.org/backup.html

Specifically, see example 2. The way that our backup/restore code is written, concurrent reads to the db would be handled fine, but concurrent writes would fail until the backup finishes. This may not always be desired.

@gotson gotson self-assigned this Dec 12, 2022
@gotson
Copy link
Copy Markdown
Collaborator

gotson commented Jan 5, 2023

/native

@gotson
Copy link
Copy Markdown
Collaborator

gotson commented Jan 5, 2023

JNI libs were rebuilt. I am happy to remove this commit if you'd prefer to build them.

It seems i am having issues with my workflow to rebuild native libs in PRs. Could you drop that commit ? I will rebuild the native libs after merging this PR to master. Thanks.

@gotson
Copy link
Copy Markdown
Collaborator

gotson commented Jan 5, 2023

/native

2 similar comments
@gotson
Copy link
Copy Markdown
Collaborator

gotson commented Jan 5, 2023

/native

@gotson
Copy link
Copy Markdown
Collaborator

gotson commented Jan 5, 2023

/native

@gotson
Copy link
Copy Markdown
Collaborator

gotson commented Jan 5, 2023

JNI libs were rebuilt. I am happy to remove this commit if you'd prefer to build them.

It seems i am having issues with my workflow to rebuild native libs in PRs. Could you drop that commit ? I will rebuild the native libs after merging this PR to master. Thanks.

Don't mind me. I managed to fix the Build Native workflow, all good now. Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement:SQLite Enhancement about sqlite features impacts:JNI Has impact on JNI C code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants