vtorc: enable read_uncommitted when in sqlite shared-cache mode

[timvaillancourt]

Description

This PR causes VTOrc to use a READ-COMMITTED transaction isolation when running in shared-cache mode, which is what VTOrc does by default

Why? The default isolation level of sqlite3 is SERIALIZABLE, which has a major impact on concurrency of both reads and writes. VTOrc reads and writes data from the same sqlite3 tables concurrently. After the VTOrc performance tunings for concurrency (many-1000s of tablets per instance), the main bottleneck showing up in profiles is the sqlite3 golang library itself

A change to READ-COMMITTED should reduce some overhead in the sqlite3 library and allow reads to occur without blocking on writes. Aside from CI passing, I believe this change is safe because the only explicit BEGIN/COMMIT transaction in VTOrc happens during init time, before the isolation level is set and any data is read or written. There are cases where many instances are written at once, however this is always done in a single mulit-INSERT/REPLACE INTO, never explicit transactions that could introduce consistency risks when paired with READ-COMMITTED

Related Issue(s)

Checklist

• "Backport to:" labels have been added if this change should be back-ported to release branches
• If this change is to be back-ported to previous releases, a justification is included in the PR description
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on CI?
• Documentation was added or is not required

Deployment Notes

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[codecov]

Codecov Report

❌ Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.53%. Comparing base (7910e1d) to head (d810ccd).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vtorc/db/db.go	85.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #18658      +/-   ##
==========================================
+ Coverage   67.50%   67.53%   +0.02%     
==========================================
  Files        1607     1613       +6     
  Lines      263567   263796     +229     
==========================================
+ Hits       177931   178163     +232     
+ Misses      85636    85633       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[shlomi-noach]

I find it difficult to gauge the risk of this change. We'd need a thorough review of our queries/statements and see whether we at all use multi-statement transactions. If not, then any query is a transaction, and read_uncommitted is safe.

[timvaillancourt]

I find it difficult to gauge the risk of this change. We'd need a thorough review of our queries/statements and see whether we at all use multi-statement transactions. If not, then any query is a transaction, and read_uncommitted is safe.

@shlomi-noach agreed, this is a hard one to review

I've done some git greping around and I'm confident the only place we're making an explicit transaction is here, but this happens only during the init of the backend, before table data is actually written by VTOrc

tim@mac vitess % git grep Begin go/vt/vtorc
go/vt/vtorc/db/db.go:   tx, err := db.Begin()

There are cases where many instances are written at the same time, but this is done using a multi-insert that read_uncommitted should not affect

[mattlord]

Why would we use read_uncommitted rather than read_committed?

[timvaillancourt]

Why would we use read_uncommitted rather than read_committed?

@mattlord sqlite3 can only support SERIALIZABLE or READ-UNCOMMITTED (in shared-cache mode only)

[mattlord]

1. I don't believe there's any doubt that this introduces some future risk — even if no danger is present today. We should understand the actual performance benefit this offers in practice so that we are able to do a proper cost/benefit analysis. Do you have any details regarding the performance benefits a user could expect?
2. The shared-cache mode appears to be marked as obsolete/deprecated and its use is NOT recommended: https://www.sqlite.org/sharedcache.html
   • Can you elaborate on why we're ignoring the SQLite authors' recommendations here?
   • They instead recommend WAL mode. On a quick read, that seems to offer the benefits of the shared-cache mode and more AND without the potential danger of using read-uncommitted (you should update the PR description as it says read-committed). Why would we not move to WAL mode by default instead (it's "server mode", which is perfect for vtorc), if we're not already? Why would we want to support shared-cache mode at all given all of this? What am I missing 🙂