1913241 - Detect and rename corrupt Skv databases

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, enhancement, P3)

Description

[Lina Butler (ex-Mozilla)]

Let's add a periodic background maintenance task to Skv. This task will be responsible for detecting and fixing up database coherence issues, and vacuuming the database. Some examples of things we'll want to run: PRAGMA integrity_check, PRAGMA foreign_key_check, REINDEX, and VACUUM.

For inspiration, Desktop's PlacesDBUtils.sys.mjs does this for Places, Fenix's PlacesHistoryStorageWorker calls into the Rust Places maintenance routine in Application Services, IndexedDB's DatabaseMaintenance class runs PRAGMA integrity_check and VACUUM.

Comment 1

[Marco Bonardo [:mak]]

if you set incremental vacuum, you can run that instead of a full vacuum.
You may also want to run pragma optimize before closing the connection.
REINDEX should not be normally necessary, it's only a way to try to fix a database if integrity_check fails due to indexing problems, it may or may not solve the problem, though now there is also a recovery api that we didn't use yet.

Comment 2

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1913241 - Detect and rename corrupt Skv databases. r?mak,asuth,janv,nanj,nika

Depends on D220045

Comment 3

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1913241 - Release the state lock during Skv store maintenance. r?mak,asuth,janv,nanj

Depends on D220760

Comment 4

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1913241 - Add tests for the Skv store state machine. r?mak,asuth,janv,nanj,nika

Depends on D220786

Comment 5

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1913241 - Fix race conditions in `skv::Store`'s guard objects. r?mak,asuth,janv,nanj,nika

Previously:

• Store::open() returned an OpenStore that held two
  Arc<Connection>s: one writer; one reader.
• Store::{reader, writer}() returned a ConnectionGuard that
  incremented the store's pending operation count in its constructor,
  and decremented the count in its destructor.

This design had two issues:

• Forgetting to wrap one of the Arc<Connection>s in a
  ConnectionGuard could cause the pending operation count to
  fall out of sync with the reference counts of both connections.
• ConnectionGuard could have decremented the pending operation count
  too early. According to Rust's rules [1], this logic would have run
  before the guard's Arc<Connection> field was dropped. If this
  happened while Store::close() was waiting on pending operations to
  finish, OperationWaiter::wait() could have unblocked the calling
  thread, only for the Arc::try_unwrap() call to fail.

This commit makes the store's use of guard objects more robust:

• ConnectionGuard is now split into OpenStoreGuard and
  OperationGuard. An OpenStoreGuard holds an Arc<OpenStore>
  and an OperationGuard in fields that are dropped in order.
• OpenStore is now stored in an Arc, ensuring that the lifecycles
  and reference counts of the reader and writer are the same.

As a consequence, Reader and Writer are the new guard types for the
read-only and read-write connections.

Depends on D220045

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5c866e72f91
Fix race conditions in `skv::Store`'s guard objects. r=nika
https://hg.mozilla.org/integration/autoland/rev/3b23f0165872
Detect and rename corrupt Skv databases. r=nanj
https://hg.mozilla.org/integration/autoland/rev/29c91f045ba8
Add tests for the Skv store state machine. r=nanj

Comment 7

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/e5c866e72f91
https://hg.mozilla.org/mozilla-central/rev/3b23f0165872
https://hg.mozilla.org/mozilla-central/rev/29c91f045ba8