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

Check that auto_vacuum is disabled when porting a SQLite database to Postgres, as VACUUMs must not be performed between runs of the script. #13195

Merged
merged 3 commits into from
Jul 7, 2022

Conversation

reivilibre
Copy link
Contributor

@reivilibre reivilibre commented Jul 6, 2022

The portdb script relies on rowids being consistent.

VACUUM does not uphold that.

auto_vacuum is not enabled by default (unless SQLite was compiled with an option to make it default), but I'm aware that
some community members manually VACUUM and I wouldn't be surprised if some of them enabled auto_vacuum themselves.

Doing so is dangerous for the portdb script as it means that some rows may be omitted from the copy.

For safety's sake, let's prohibit auto_vacuum when porting a database and document that manual VACUUM are also not to be done.

@reivilibre reivilibre requested a review from a team as a code owner July 6, 2022 11:40
@reivilibre reivilibre marked this pull request as draft July 6, 2022 11:41
@reivilibre reivilibre removed the request for review from a team July 6, 2022 11:41
@reivilibre reivilibre force-pushed the rei/port_db_ensure_no_autovacuum branch from 6686191 to 2846b88 Compare July 6, 2022 11:49
@reivilibre reivilibre marked this pull request as ready for review July 6, 2022 11:50
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Happy with the changes, just a few questions.

The portdb script relies on rowids being consistent.

Can you point me to the relevant source code here?

" be used if the database could be vacuumed between re-runs.\n"
" To disable auto_vacuum, you need to stop Synapse and run the following SQL:\n"
" PRAGMA auto_vacuum=off;\n"
" VACUUM;"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to vacuum here after disabling autovacuum? To ensure there is no vacuum in progress when the script runs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't change the auto_vacuum setting without VACUUMing if there are existing tables

Copy link
Contributor

Choose a reason for hiding this comment

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

You can't change the auto_vacuum setting without VACUUMing if there are existing tables

Does that mean the vacuum should happen first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No; you set the PRAGMA then VACUUM to make it take effect.

@reivilibre
Copy link
Contributor Author

The portdb script relies on rowids being consistent.

Can you point me to the relevant source code here?

An example line is

forward_select = (

but feel free to look all over the script — the tables that are being migrated are paginated by rowid.

Notable is the port_from_sqlite3 table that gets created, which tracks the oldest and newest rowids that have been copied across.

@reivilibre reivilibre enabled auto-merge (squash) July 7, 2022 09:38
@reivilibre reivilibre merged commit fb7d24a into develop Jul 7, 2022
@reivilibre reivilibre deleted the rei/port_db_ensure_no_autovacuum branch July 7, 2022 10:08
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.

2 participants