-
Notifications
You must be signed in to change notification settings - Fork 597
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
feat: deprecate current epoch in pinned snapshot #18230
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
statement ok | ||
SET VISIBILITY_MODE TO checkpoint; | ||
|
||
connection txn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this PR, we won't provide consistency support for barrier read, and the CI with visibility_mode set to all will fail in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that read-only transaction does not guarantee any isolation when visibility_mode
is all
? If so, shall we ban user from doing this to avoid confusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For simplicity, I change to send a notice to user when the user explicitly start read only transaction with visibility_mode is set to all.
🥵 After this PR, the e2e
|
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Motivation is described in #18214.
As we are deprecating MVCC support on a single hummock version, barrier read with consistency will greatly increase the overall complexity. As barrier read is not with high priority, we may give up the consistency in barrier read. Implementation-wise, instead of using a consistent uncommitted epoch for queries in all tables, we will change to use
u64::MAX
as the epoch to query data from all tables, which means we will read the latest data in all tables. In this way, while deprecating MVCC support on a single hummock version, we can still preserve the functionality of barrier read for query with higher freshness.After deprecating the consistent barrier read, we can further deprecate the current epoch in pinned snapshot, and then we can deprecate the usage of
HummockReadEpoch::Current(epoch)
, and thevalidate_read_epoch
andseal_epoch
ofStateStore
trait can be further deprecated.Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
Previously, when the session variable
visibility_mode
is set toall
, while we are able to query uncommitted data, we still ensure consistency between the tables involved in the query. After this PR, whenvisibility_mode
is set toall
, we will change to query the latest uncommitted data, and there is no consistency between the involved tables, even if a read only transaction is begun.