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 order of SQLite PRAGMAS to allow configurable page_size #5140

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

vvysokikh1
Copy link
Collaborator

High Level Overview of Change

Fixed the order of application of pragmas when opening SQLite connection. Due to specific pragmas set with commonPragmas, it looks like a page gets allocated in the DB file BEFORE page_size is set, thus locking the page_size to default value 4096.

Context of Change

This issue has no effect before #5135 is merged.

Type of Change

  • [ x] Bug fix (non-breaking change which fixes an issue)

Test Plan

  1. Run rippled creating brand new transaction.db file with SQLite: Allow configurable sqlite database pragma variables. #5135, setting rippled.cfg to have the following:
...
[sqlite]
page_size = 8192
...
  1. After a couple of seconds you should be able to run sqlite command to check the page_size of new transaction.db file:
sqlite3 <path>/transaction.db 'PRAGMA page_size;'

@vvysokikh1 vvysokikh1 changed the title Fix order of PRAGMAS to allow configurable page_size Fix order of SQLite PRAGMAS to allow configurable page_size Sep 17, 2024
@vvysokikh1 vvysokikh1 added the Bug label Sep 17, 2024
Copy link

codecov bot commented Sep 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.2%. Comparing base (b6391fe) to head (915c71f).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5140   +/-   ##
=======================================
  Coverage     76.2%   76.2%           
=======================================
  Files          760     760           
  Lines        61550   61550           
  Branches      8123    8122    -1     
=======================================
+ Hits         46884   46888    +4     
+ Misses       14666   14662    -4     
Files with missing lines Coverage Δ
src/xrpld/core/DatabaseCon.h 100.0% <100.0%> (ø)

... and 6 files with indirect coverage changes

Impacted file tree graph

@Bronek Bronek self-requested a review September 18, 2024 09:55
Copy link
Collaborator

@Bronek Bronek left a comment

Choose a reason for hiding this comment

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

LGTM

@Bronek Bronek added the Trivial label Sep 18, 2024
@intelliot intelliot mentioned this pull request Sep 19, 2024
1 task
@Bronek
Copy link
Collaborator

Bronek commented Sep 19, 2024

For readers of this PR:

  • this is about swapping the order of execution of SQLite PRAGMA statements in commonPragma parameter and pragma parameter
  • commonPragma is either nullptr or globalPragma initialised in xrpld/core/detail/DatabaseCon.cpp (look for result->emplace_back)
  • the actual PRAGMA statements are constructed from the three strings CommonDBPragma... in xrpld/app/main/DBInit.h
  • the problem this PR solves is caused by PRAGMA journal_mode which creates journal file - hence committing page_size to the database, whatever it is at that point
  • so if we want a non-default page_size, it needs to be set before PRAGMA journal_mode
  • we set page_size in pragma parameter , hence it needs to be processed before commonPragma parameter
  • we have not noticed the problem until today because the hardcoded page_size happens to be 4KB, which is identical as the SQLite default
  • however there's a PR in flight SQLite: Allow configurable sqlite database pragma variables. #5135 to make page_size configurable and that will, obviously, do not work

FWIW globalPragma is static which is a bad code smell, but changing it belongs to a different PR (refactoring only)

@intelliot intelliot mentioned this pull request Sep 20, 2024
1 task
Copy link
Collaborator

@intelliot intelliot left a comment

Choose a reason for hiding this comment

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

Important to move these pragmas first so that PRAGMA page_size is executed before PRAGMA journal_mode (which accesses the database and commits the page_size)

@intelliot intelliot merged commit 0ece395 into XRPLF:develop Sep 20, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants