Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

rgw/sfs: use sqlite3 backup API to create temp db #208

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

tserong
Copy link
Member

@tserong tserong commented Sep 12, 2023

It's not safe for DBConn::check_metadata_is_compatible() to make a copy of the database file while it's already open via SQLite, because the close() that happens after the file is copied cancels all POSIX advisory locks, which can later lead to database corruption. For details, see:

https://www.sqlite.org/howtocorrupt.html#_posix_advisory_locks_canceled_by_a_separate_thread_doing_close_

This change replaces fs::copy() with use of SQLite's Backup API (https://www.sqlite.org/backup.html) which is safe to use with open databases.

Fixes: https://github.com/aquarist-labs/s3gw/issues/702

It's not safe for DBConn::check_metadata_is_compatible()
to make a copy of the database file while it's already open
via SQLite, because the close() that happens after the file
is copied cancels all POSIX advisory locks, which can later
lead to database corruption.  For details, see:

https://www.sqlite.org/howtocorrupt.html#_posix_advisory_locks_canceled_by_a_separate_thread_doing_close_

This change replaces fs::copy() with use of SQLite's Backup API
(https://www.sqlite.org/backup.html) which is safe to use with
open databases.

Fixes: https://github.com/aquarist-labs/s3gw/issues/702
Signed-off-by: Tim Serong <[email protected]>
@tserong tserong requested review from jecluis, irq0 and 0xavi0 September 12, 2023 06:00
Copy link
Member

@irq0 irq0 left a comment

Choose a reason for hiding this comment

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

An alternative might be sqlite_orm's make_backup_to. But that would have go through a copy of Storage via get_storage instead of first_db_conn. Benefit would be simpler error handling - sqlite_orm throws on non SQLITE_OK return codes.

Is it safe to ignore the returncodes of sqlite3_backup_step and sqlite3_backup_finish?

@tserong
Copy link
Member Author

tserong commented Sep 12, 2023

An alternative might be sqlite_orm's make_backup_to. But that would have go through a copy of Storage via get_storage instead of first_db_conn. Benefit would be simpler error handling - sqlite_orm throws on non SQLITE_OK return codes.

I didn't know about that :-) I don't have a strong preference for any particular implementation, and I don't think it makes much practical difference whether it uses get_storage or first_db_conn (this thing runs once at startup).

Is it safe to ignore the returncodes of sqlite3_backup_step and sqlite3_backup_finish?

Yeah, according to https://www.sqlite.org/backup.html and https://www.sqlite.org/c3ref/backup_finish.html if these fail an error code will be attached to the destination database, which will then be picked up when we call sqlite3_errcode(temporary_db);

@tserong tserong merged commit 6002dde into aquarist-labs:s3gw Sep 12, 2023
@tserong tserong deleted the wip-fix-702 branch September 12, 2023 22:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] sqlite3 db corruption if db read by external process while s3gw active
2 participants