Skip to content

Change SQLite backup to use VACUUM INTO query#6989

Open
getaaron wants to merge 2 commits intodani-garcia:mainfrom
getaaron:fix-backup-memory
Open

Change SQLite backup to use VACUUM INTO query#6989
getaaron wants to merge 2 commits intodani-garcia:mainfrom
getaaron:fix-backup-memory

Conversation

@getaaron
Copy link

#6279 introduced several great improvements, but replaced VACUUM INTO with serialize_database_to_buffer() which is less efficient and loads the whole DB into RAM. This may be contributing to an increase in OOM errors which I have noticed in small vaultwarden deployments with 256MB RAM.

This PR reverts the serialize_database_to_buffer() change back into a VACUUM INTO query while keeping the other improvements from that PR. I also used .bind::<Text, _>(&backup_file) instead of the previous format!("VACUUM INTO '{}'", backup_file) to prevent theoretical SQL injection (even though path is trusted), let me know if you prefer the format! approach and I can switch it

Replaced manual file creation for SQLite backup with a VACUUM INTO query.
@getaaron
Copy link
Author

@BlackDex I would appreciate your review on this in cased I missed anything, I wasn't sure if there was a reason for the change

@BlackDex
Copy link
Collaborator

Thanks @getaaron, there was no specific reason here except for this comment: #6279 (comment)

The only thing which might be a thing in the future though, since that isn't a option right now, is that if we would allow changing the backup location, we might want to use the OpenDal feature, which would make it possible to upload backups to S3 for example. Using VACUUM INTO does not support this, and that would mean we would need to VACUUM, and read/upload the file afterwards.

Other than that, this PR looks ok code wise 😄

@getaaron
Copy link
Author

Thanks @getaaron, there was no specific reason here except for this comment: #6279 (comment)

The only thing which might be a thing in the future though, since that isn't a option right now, is that if we would allow changing the backup location, we might want to use the OpenDal feature, which would make it possible to upload backups to S3 for example. Using VACUUM INTO does not support this, and that would mean we would need to VACUUM, and read/upload the file afterwards.

Other than that, this PR looks ok code wise 😄

Agreed. When we add OpenDal we could VACUUM into a file and upload as you say. This could add some latency and an extra failure point but doesn't seem that bad to me. Other options:

  • Make it configurable
  • Try to make it intelligent (only VACUUM if it seems like realizing the whole DB may cause memory pressure)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants