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

Remove strict_sql_mode which is covered by sql_mode #4494

Merged
merged 4 commits into from Apr 12, 2019

Conversation

Projects
None yet
3 participants
@breeswish
Copy link
Member

breeswish commented Apr 9, 2019

Signed-off-by: Breezewish breezewish@pingcap.com

What have you changed? (mandatory)

In latest tipb we removed strict_sql_mode because it is already covered by sql_mode. The removal totally avoids the following unsound scenarios, which is very hard to handle:

  • Received a request whose sql_mode contains strict mode but is_strict_mode is false
  • Received a request whose sql_mode does not contain strict mode but is_strict_mode is true

Accordingly, we now remove all usages of strict_sql_mode in TiKV, changing to use sql_mode instead.

What are the type of the changes? (mandatory)

  • Bug fix: Previously we use a wrong bit flag for these SQL modes so that they actually won't take effect.
  • Engineering

How has this PR been tested? (mandatory)

Existing unit tests.

@breeswish breeswish force-pushed the breeswish:update_tipb branch from cb8f235 to 0593810 Apr 9, 2019

Remove strict_sql_mode which is covered by sql_mode
Signed-off-by: Breezewish <breezewish@pingcap.com>

@breeswish breeswish force-pushed the breeswish:update_tipb branch from 0593810 to 7710733 Apr 9, 2019

Fix an incorrect test case
Signed-off-by: Breezewish <breezewish@pingcap.com>
@breeswish

This comment has been minimized.

Copy link
Member Author

breeswish commented Apr 9, 2019

/run-integration-tests

@breeswish breeswish requested review from AndreMouche, nrc and rleungx Apr 9, 2019

@@ -145,16 +158,11 @@ impl EvalConfig {
self
}

pub fn set_sql_mode(&mut self, new_value: u64) -> &mut Self {
pub fn set_sql_mode(&mut self, new_value: SqlMode) -> &mut Self {

This comment has been minimized.

Copy link
@breeswish

breeswish Apr 9, 2019

Author Member

Maybe we need a better name. For bit flags, set usually means "mask" instead of "replace" (consider set_xxx_flag and unset_xxx_flag, we are not expecting to unset all other flags when calling set_xxx_flag). However the semantic of this function here is replace.

This comment has been minimized.

Copy link
@nrc

nrc Apr 11, 2019

Contributor

I think it is OK since we are setting the whole value and we don't have methods to set individual bits (i.e., if this were a method on SqlMode it should be replace_ but since it is a method on EvalConfig and the field being set is SqlMode, I think set_ is correct).

@nrc

nrc approved these changes Apr 11, 2019

Copy link
Contributor

nrc left a comment

LGTM, some minor comments inline, none are blockers

}

impl SqlMode {
/// Returns if 'STRICT_TRANS_TABLES' or 'STRICT_ALL_TABLES' mode is set.

This comment has been minimized.

Copy link
@nrc

nrc Apr 11, 2019

Contributor

Nit: this comment is not useful. It would be better to describe what 'strict' mode means, the effect it will have on execution, etc. (or you could just remove it)

This comment has been minimized.

Copy link
@breeswish

breeswish Apr 12, 2019

Author Member

Good idea, thanks!

@@ -145,16 +158,11 @@ impl EvalConfig {
self
}

pub fn set_sql_mode(&mut self, new_value: u64) -> &mut Self {
pub fn set_sql_mode(&mut self, new_value: SqlMode) -> &mut Self {

This comment has been minimized.

Copy link
@nrc

nrc Apr 11, 2019

Contributor

I think it is OK since we are setting the whole value and we don't have methods to set individual bits (i.e., if this were a method on SqlMode it should be replace_ but since it is a method on EvalConfig and the field being set is SqlMode, I think set_ is correct).

pub fn mode_no_zero_date_mode(&self) -> bool {
self.sql_mode & MODE_NO_ZERO_DATE_MODE == MODE_NO_ZERO_DATE_MODE
}

This comment has been minimized.

Copy link
@nrc

nrc Apr 11, 2019

Contributor

I would keep these methods, it means users of EvalConfig don't have to know how sql mode is implemented internally

This comment has been minimized.

Copy link
@breeswish

breeswish Apr 12, 2019

Author Member

Thanks! I removed these functions because they simply check individual flags. It might be not DRY if we provide a function for each flag 🤨

@breeswish

This comment has been minimized.

Copy link
Member Author

breeswish commented Apr 12, 2019

/run-integration-tests

@@ -673,8 +674,7 @@ mod tests {
// test zero case
let mut cfg = EvalConfig::new();
cfg.set_by_flags(FLAG_IN_UPDATE_OR_DELETE_STMT)
.set_sql_mode(MODE_ERROR_FOR_DIVISION_BY_ZERO)
.set_strict_sql_mode(true);
.set_sql_mode(SqlMode::NO_ZERO_DATE | SqlMode::STRICT_ALL_TABLES);

This comment has been minimized.

Copy link
@rleungx

rleungx Apr 12, 2019

Member

Why NO_ZERO_DATE here?

This comment has been minimized.

Copy link
@breeswish

breeswish Apr 12, 2019

Author Member

NO ZERO DATE is the correct flag to make this test working. This test succeeded before only because of our flags were wrong and did not have effects. Now this PR makes these flags correct so the mistake in this test must be fixed.

const STRICT_ALL_TABLES = 1 << 23;
const NO_ZERO_IN_DATE = 1 << 24;
const NO_ZERO_DATE = 1 << 25;
const INVALID_DATES = 1 << 26;

This comment has been minimized.

Copy link
@rleungx

rleungx Apr 12, 2019

Member

Is this necessary for this PR?

This comment has been minimized.

Copy link
@breeswish

breeswish Apr 12, 2019

Author Member

yes, since we are now using values from sql_mode (but which is wrong)

@breeswish breeswish merged commit 7865df1 into tikv:master Apr 12, 2019

5 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

@breeswish breeswish deleted the breeswish:update_tipb branch Apr 12, 2019

@b41sh b41sh referenced this pull request Apr 16, 2019

Merged

use bitflag to replace bool #4536

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.