Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sql: enable auto_vacuum on all connections #2955

Merged
merged 2 commits into from
Feb 15, 2023
Merged

sql: enable auto_vacuum on all connections #2955

merged 2 commits into from
Feb 15, 2023

Conversation

link2xt
Copy link
Collaborator

@link2xt link2xt commented Jan 6, 2022 •
edited
Loading

Trying to enable auto_vacuum on all connections

@link2xt
Copy link
Collaborator Author

link2xt commented Jan 6, 2022

This is flaky, I originally pushed it to master as ce0984f after testing, but reverted because of "database is busy" errors on CI: 652d67a

This is probably working correctly, but is slower because of this.

r10s
r10s approved these changes Jan 6, 2022
View reviewed changes
Copy link
Member

@r10s r10s left a comment •
edited
Loading

Choose a reason for hiding this comment

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

thanks a lot for taking care!

not totally sure i understand all side-effects totally, however, some things turn out to be visible during testing only, so it probably makes sense to get things in and move forward. we can revert and improve any time.

@link2xt
Copy link
Collaborator Author

link2xt commented Jan 6, 2022

The point of having this pragma enabled on all connections is to ensure that when we execute VACUUM, no matter on which connection it happens, we enable auto_vacuum.

This can't be merged as is though, it fails random tests with "database is locked" error on CI currently.

@r10s
Copy link
Member

r10s commented Jan 6, 2022

This can't be merged as is though, it fails random tests with "database is locked" error on CI currently.

wondering what is happening here. databases used for ci are only a few kb large and even a full VACUUM should take only a ms or so.

@link2xt link2xt marked this pull request as draft January 7, 2022 00:51
@link2xt
Copy link
Collaborator Author

link2xt commented Jan 7, 2022

Auto vacuum is not really important right now, it is enabled for new databases (with a test for it) and for existing databases if it's not enabled worst case is that the database never shrinks in size except on VACUUM, but the space is still reused by the database itself.

@link2xt link2xt force-pushed the auto_vacuum-all branch 3 times, most recently from 38dcd0e to 737d0d1 Compare April 16, 2022 21:46
@link2xt
Copy link
Collaborator Author

link2xt commented Apr 16, 2022 •
edited
Loading

Dropping sqlite connection pool does not result in actually closing connections immediately. On windows this is a problem, because files remain locked for some time after drop. As a workaround, migration test and account removal now has a 5 second sleep to ensure all connections are actually closed :/

There is an open bugreport about this in r2d2: sfackler/r2d2#99

@r10s r10s mentioned this pull request Apr 16, 2022
Closed
@link2xt
Copy link
Collaborator Author

link2xt commented Apr 16, 2022

I guess as a workaround when we try to rename files or delete files, we should sleep 1 second and retry up to 5 times or something like this.

@link2xt link2xt force-pushed the auto_vacuum-all branch 6 times, most recently from 2669b53 to f3e360e Compare April 17, 2022 22:31
@link2xt link2xt mentioned this pull request Apr 17, 2022
Merged
@link2xt
Copy link
Collaborator Author

link2xt commented Apr 17, 2022

Waiting for #3229 to be merged and a similar fix for migrate_account is needed.

@hpk42 hpk42 force-pushed the auto_vacuum-all branch 2 times, most recently from 3d9c619 to 2dd6bd4 Compare April 26, 2022 13:28
@link2xt link2xt force-pushed the auto_vacuum-all branch from 2dd6bd4 to ebcfe62 Compare July 6, 2022 05:59
@hpk42
Copy link
Contributor

hpk42 commented Dec 4, 2022 •
edited
Loading

Auto vacuum is not really important right now, it is enabled for new databases (with a test for it) and for existing databases if it's not enabled worst case is that the database never shrinks in size except on VACUUM, but the space is still reused by the database itself.

If this is still true and the PR triggers issues in tests the i think this can be put to project resurrection. It would become more important if we put blob files into the same database used for messages, networking etc. And then only for older databases. New databases are created with auto-vaccum as noted in the quote.

@link2xt
Copy link
Collaborator Author

link2xt commented Feb 15, 2023

Waiting for #3229 to be merged and a similar fix for migrate_account is needed.

I implemented retries for account migration in #4043.

@link2xt link2xt force-pushed the auto_vacuum-all branch 2 times, most recently from 4a09e01 to f7eeb15 Compare February 15, 2023 16:37
@link2xt
Copy link
Collaborator Author

link2xt commented Feb 15, 2023

Now with #4043 tests pass.

@link2xt link2xt marked this pull request as ready for review February 15, 2023 16:54
@link2xt
sql: update version first in migration transactions
c52b48b 
In execute_migration transaction first update the version, and only
then execute the rest of the migration. This ensures that transaction
is immediately recognized as write transaction and there is no need
to promote it from read transaction to write transaction later, e.g.
in the case of "DROP TABLE IF EXISTS" that is a read only operation if
the table does not exist. Promoting a read transaction to write
transaction may result in an error if database is locked.
@link2xt link2xt merged commit 7d2cca8 into master Feb 15, 2023
@link2xt link2xt deleted the auto_vacuum-all branch February 15, 2023 20:41
@link2xt link2xt mentioned this pull request Feb 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r10s r10s r10s approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@link2xt @r10s @hpk42