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.

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

sqlite: set connection attributes on open #17867

Closed
wants to merge 1 commit into from

Conversation

vrothberg
Copy link
Member

The symptoms in #17859 indicate that setting the PRAGMAs in individual EXECs outside of a transaction can lead to concurrency issues and failures when the DB is locked. Hence set all PRAGMAs when opening the connection. Move them into individual constants to improve documentation and readability.

Further make transactions exclusive as #17859 also mentions an error that the DB is locked during a transaction.

Fixes: #17859

Does this PR introduce a user-facing change?

None

@mheon @baude @edsantiago PTAL

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 21, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 21, 2023
The symptoms in containers#17859 indicate that setting the PRAGMAs in individual
EXECs outside of a transaction can lead to concurrency issues and
failures when the DB is locked.  Hence set all PRAGMAs when opening
the connection.  Move them into individual constants to improve
documentation and readability.

Further make transactions exclusive as containers#17859 also mentions an error
that the DB is locked during a transaction.

[NO NEW TESTS NEEDED] - existing tests cover the code.

Fixes: containers#17859
Signed-off-by: Valentin Rothberg <[email protected]>
@baude
Copy link
Member

baude commented Mar 21, 2023

LGTM

@rhatdan
Copy link
Member

rhatdan commented Mar 21, 2023

@mheon PTAL

@edsantiago
Copy link
Member

The string "database is locked" does not appear in any of the CI logs for this PR, yay.

There is one instance of the "UNIQUE constraint failed" bug, in f37 root.

I rebased my #17831 on top of this, so we can see what system tests do.

@vrothberg
Copy link
Member Author

The string "database is locked" does not appear in any of the CI logs for this PR, yay.

There is one instance of the "UNIQUE constraint failed" bug, in f37 root.

Yes, I did not tackle #17858 in this PR.

I rebased my #17831 on top of this, so we can see what system tests do.

That is good news, thanks!

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@edsantiago
Copy link
Member

The string "database is locked" does not appear in any of the logs for the abovementioned CI run of #17831. (Two systest failures, both expected). (And, two "UNIQUE constraint" failures, and I know this PR did not set out to fix that, but I had this fantasy that the bugs were related and could be 2birds1stoned...)

cirrus-pr-timing results below (only the boltdb vs sqlite comparison). The new sqlite options seem to slow things down a little bit, I don't know how significantly.

type distro user DB local remote container
int fedora-37 root 29:49 32:55 28:10
int fedora-37 root sqlite 29:33 33:49
int fedora-37 rootless 28:15
int fedora-37 rootless sqlite 30:39

Anyhow, LGTM

@vrothberg
Copy link
Member Author

cirrus-pr-timing results below (only the boltdb vs sqlite comparison). The new sqlite options seem to slow things down a little bit, I don't know how significantly.

Those numbers are really great to have, thanks, Ed! @baude, we can leverage Ed's timing tracking for the upcoming performance massages.

@edsantiago edsantiago mentioned this pull request Mar 21, 2023
@mheon
Copy link
Member

mheon commented Mar 21, 2023

@vrothberg PTAL at #17874 - I think we can avoid transaction exclusivity

@vrothberg
Copy link
Member Author

@vrothberg PTAL at #17874 - I think we can avoid transaction exclusivity

Why open a new PR that conflicts with this one? Seems like a time waste.

@vrothberg
Copy link
Member Author

OK, we had a quick chat and greed to mix the two PRs together. This PR gets rid of the multiple EXECs setting the PRAGMAs which is flaking. An open question is what setting max connections does in comparison to making transactions exclusive. I assumed that transactions (i.e., writes) must be exclusive.

But I am buried in meeting til the end of my workday and let @mheon take over :)

@mheon
Copy link
Member

mheon commented Mar 21, 2023

Pulled it into my PR, closing this now

@mheon mheon closed this Mar 21, 2023
@vrothberg vrothberg deleted the sqlite-fix branch March 22, 2023 08:00
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 4, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sqlite: database is locked
6 participants