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

fix(sqlite): support all sqlite pragma statement #682

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vbehar
Copy link

@vbehar vbehar commented Sep 27, 2024

What was changed

  • I removed the extra underscore in the key
  • I removed the pragma key check

Why?

  • the extra underscore was breaking the pragma support, because sqlite couldn't recognise the pragma key
  • the pragma key check was too restricting, users should be able to configure any pragma statement from https://www.sqlite.org/pragma.html - removing the check all to use any pragma, without adding more work to maintain an hardcoded list

Checklist

  1. Closes [Bug] Sqlite pragmas are not applied correctly #681

  2. How was this tested:

manually:

  • by running ./temporal server start-dev --db-filename temporal.sqlite --sqlite-pragma journal_mode=WAL --sqlite-pragma synchronous=NORMAL --sqlite-pragma busy_timeout=5000
  • and ensuring that 3 files have been created: temporal.sqlite, temporal.sqlite-shm and temporal.sqlite-wal
  1. Any docs updates needed?

no, because it's just a bug fix. the docs are already there.

@CLAassistant
Copy link

CLAassistant commented Sep 27, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Thanks! Will confer with team just to make sure there wasn't a reason we were limiting the pragmas. I don't see anything obvious at temporalio/temporalite-archived#30 at first glance.

Comment on lines -330 to +324
conf.ConnectAttributes["_"+k] = v
conf.ConnectAttributes[k] = v
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I suspect we just didn't have test cases. Would you be willing to add a test (or adjust an existing one) that just adds any innocuous pragma just to ensure it works?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll add one

fixes temporalio#681

- remove the extra underscore in the key
- remove the pragma key check - let's support any pragma
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.

[Bug] Sqlite pragmas are not applied correctly
3 participants