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

sql: drop the attachdriver mechanism #97

Merged
merged 6 commits into from
Sep 10, 2024
Merged

Conversation

moio
Copy link
Contributor

@moio moio commented Aug 30, 2024

After security assessment it was determined storing filterable/sortable columns on disk is acceptable, as none of the information is expected to be sensible.

This removes the attachdriver mechanism that created one DB on the disk and another, optionally in memory, as attached DB. Only one DB is created now.

This change was driven by the need of dropping the shared cache SQLite mode:

https://sqlite.org/sharedcache.html

which is in turn needed in order to activate WAL mode, which allows for better performance and less locking (including at CREATE INDEX time).

best reviewed commit-by-commit

@moio moio requested a review from a team as a code owner August 30, 2024 14:31
@moio moio requested a review from pjbgf August 30, 2024 14:32
@moio moio changed the title Drop the attachdriver mechanism sql: drop the attachdriver mechanism Aug 30, 2024
@moio
Copy link
Contributor Author

moio commented Aug 30, 2024

@pjbgf as agreed this is described in rancher/steve#269

moio added 4 commits August 30, 2024 16:41
After security assessment it was determined storing filterable/sortable
columns on disk is acceptable, as none of the information is expected
to be sensible.

This removes the attachdriver mechanism that created one DB on the disk
and another, optionally in memory, as attached DB.

Only one DB is created now.

This change was driven by the need of dropping the shared cache SQLite
mode:

https://sqlite.org/sharedcache.html

which is in turn needed in order to activate WAL mode, which allows
for better performance and less locking (including at CREATE INDEX
time).

Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
@moio moio force-pushed the drop_attachdriver branch 2 times, most recently from ca518d1 to 9793898 Compare August 30, 2024 15:01
@moio moio requested a review from tomleb August 30, 2024 15:05
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

Minor nit, feel free to ignore. Otherwise, LGTM.

pkg/cache/sql/informer/factory/informer_factory.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tomleb tomleb left a comment

Choose a reason for hiding this comment

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

Just needs to update the sql cache README but otherwise looks good 👍

pkg/cache/sql/attachdriver/driver.go Outdated Show resolved Hide resolved
Signed-off-by: Silvio Moioli <[email protected]>
@moio moio requested a review from tomleb September 9, 2024 12:54
Copy link
Contributor

@MbolotSuse MbolotSuse left a comment

Choose a reason for hiding this comment

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

approving to allow merge since there's already the required number of reviewers.

@moio moio merged commit 591e606 into rancher:master Sep 10, 2024
1 check passed
@moio moio deleted the drop_attachdriver branch September 10, 2024 15:49
moio added a commit that referenced this pull request Sep 18, 2024
This 
 - sets PRAGMAs correctly, before they were ignored as they used a wrong syntax
 - removes the `shared=cache` option as #97 removed the need for it
 - removes the mechanism retrying on SQLITE_BUSY, which is then unnecessary
 - adds code to issue `BEGIN` on read-only transactions and `BEGIN IMMEDIATE` for transactions expected to write

The last point is necessary in order to avoid `SQLITE_BUSY` (5) errors in presence of multiple concurrent writing transactions. Without it `busy_timeout` would not be sufficient, see discussion in #98

Signed-off-by: Silvio Moioli <[email protected]>
moio added a commit to moio/rancher that referenced this pull request Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants