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

session, exectutor: Guarantee external consistency by default; Add an explicit begin statement to disable it #22597

Merged
merged 20 commits into from
Feb 4, 2021

Conversation

ekexium
Copy link
Contributor

@ekexium ekexium commented Jan 28, 2021

What problem does this PR solve?

Problem Summary:

External consistency is not a widely known concept for many users. So we guarantee it by default, and only allow disabling it by an explicit start transaction with causal consistency only statement.

What is changed and how it works?

What's Changed:

  • Hide the tidb_guarantee_external_consistency variable from show variables
  • Guarantee external consistency by default
  • Add a statement start transaction with causal consistency only to start a transaction that does not require external consistency
  • Stop using the ambiguous term "external consistency". Use "linearizability" instead

How it Works:

Related changes

Check List

Tests

  • Integration test (UTF)

Side effects

Release note

  • Adds a statement start transaction with causal consistency only to start a transaction that does not require external consistency.
  • Rename the sysvar tidb_guarantee_external_consistency to tidb_guarantee_linearizability

@ekexium ekexium requested a review from a team as a code owner January 28, 2021 08:40
@ekexium ekexium requested review from XuHuaiyu and removed request for a team January 28, 2021 08:40
@github-actions github-actions bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels Jan 28, 2021
@ekexium
Copy link
Contributor Author

ekexium commented Jan 28, 2021

/run-all-tests

@ekexium
Copy link
Contributor Author

ekexium commented Jan 28, 2021

@sticnarf @cfzjywxk PTAL

@sticnarf
Copy link
Contributor

Currently, there are some controversies over how to disable external consistency:

  1. The way in this PR is more conservative. The business code programmer fully controls the behavior. It's the safest way because it's so explicit that the programmer must understand external consistency before disabling it.

  2. Some others hope to keep the system variables. Because it is quite rare when external consistency is needed, if the business code is known, it will give an easy way to get some performance boost. And external consistency is guaranteed by default, the user disables it at their own risk.

cc @scsldb @jackysp @cfzjywxk @youjiali1995

@youjiali1995
Copy link
Contributor

It's ok to guarantee external consistency by default, but there must be a system variable to control it. We need an easy way to disable it, otherwise, it's difficult to get the best performance in POC or in users' business if they know what external consistency it is.

I'm fine with the syntax and users use it to remove the guarantee by themself, but I think no one will use it...

@sticnarf
Copy link
Contributor

sticnarf commented Feb 3, 2021

I suggest the async commit related settings changed by the HTTP APIs can be persistent and take effect globally. Then, we needn't set the property again after restarting or adding a new instance.

@ekexium ekexium force-pushed the begin-external-consistency branch from be225f3 to b639100 Compare February 3, 2021 07:11
@ekexium
Copy link
Contributor Author

ekexium commented Feb 3, 2021

/run-all-tests

Copy link
Contributor

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

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

Let's change the ambiguous word "external consistency" in the TiDB repo.

session/session.go Outdated Show resolved Hide resolved
sessionctx/variable/tidb_vars.go Outdated Show resolved Hide resolved
@ekexium ekexium force-pushed the begin-external-consistency branch from c1ed099 to d6991df Compare February 3, 2021 08:32
kv/kv.go Outdated Show resolved Hide resolved
@sticnarf
Copy link
Contributor

sticnarf commented Feb 3, 2021

@youjiali1995 PTAL

@ekexium ekexium force-pushed the begin-external-consistency branch from f6c9eb5 to 044d27a Compare February 3, 2021 08:36
Signed-off-by: ekexium <[email protected]>
@ekexium ekexium force-pushed the begin-external-consistency branch from c32fe7a to de28dd7 Compare February 3, 2021 08:39
Copy link
Contributor

@youjiali1995 youjiali1995 left a comment

Choose a reason for hiding this comment

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

Rest LGTM.

store/tikv/2pc.go Outdated Show resolved Hide resolved
session/session.go Outdated Show resolved Hide resolved
@youjiali1995
Copy link
Contributor

I suggest the async commit related settings changed by the HTTP APIs can be persistent and take effect globally. Then, we needn't set the property again after restarting or adding a new instance.

So we still use the global sysvar?😃 I prefer it to HTTP API.

@sticnarf
Copy link
Contributor

sticnarf commented Feb 3, 2021

I suggest the async commit related settings changed by the HTTP APIs can be persistent and take effect globally. Then, we needn't set the property again after restarting or adding a new instance.

So we still use the global sysvar?smiley I prefer it to HTTP API.

I wouldn't prefer either. If we don't use sysvar, where do you suggest to put the configurations?

I discussed this with @ekexium about some alternatives like watching a key in the PD's etcd. Personally, I think this solution is simpler if a hidden sysvar is acceptable because we don't need to invent a new mechanism to synchronize configs across TiDB instances.

@youjiali1995
Copy link
Contributor

Personally, I think this solution is simpler if a hidden sysvar is acceptable because we don't need to invent a new mechanism to synchronize configs across TiDB instances.

I mean I agree with it...

@sticnarf
Copy link
Contributor

sticnarf commented Feb 3, 2021

I mean I agree with it...

sorry I misread the sentence "I prefer it to HTTP API"...

@ekexium
Copy link
Contributor Author

ekexium commented Feb 4, 2021

/run-all-tests

@ekexium ekexium force-pushed the begin-external-consistency branch from 030ea9f to 5d08138 Compare February 4, 2021 03:06
@ekexium ekexium force-pushed the begin-external-consistency branch from 5d08138 to cdd3d51 Compare February 4, 2021 03:12
@ekexium
Copy link
Contributor Author

ekexium commented Feb 4, 2021

/run-all-tests

@sticnarf
Copy link
Contributor

sticnarf commented Feb 4, 2021

LGTM. And please resolve conflicts.

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 4, 2021
@jackysp jackysp added the sig/transaction SIG:Transaction label Feb 4, 2021
Copy link
Contributor

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Feb 4, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 4, 2021
@sticnarf
Copy link
Contributor

sticnarf commented Feb 4, 2021

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 4, 2021
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 2fc1703 into pingcap:master Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/sql-infra SIG: SQL Infra sig/transaction SIG:Transaction status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants