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

executor: 'select * from information_schema.tables' fail after setting @@tidb_snapshot (#18676) #18692

Merged
merged 2 commits into from
Jul 28, 2020

Conversation

ti-srebot
Copy link
Contributor

cherry-pick #18676 to release-4.0


What problem does this PR solve?

Issue Number: close #18347

Problem Summary:

The reproduce steps:

	create database gzj_test_db
	create table gzj_test_db.ad_account_record_log (id int auto_increment primary key)
	select now()
	drop database gzj_test_db
        set @@tidb_snapshot = '2020-07-18 23:56:05'   // now() 
	select * from information_schema.tables

'Table gzj_test_db.ad_account_record_log' doesn't exist

What is changed and how it works?

What's Changed:

getAutoIncrementID should consider the @@tidb_snapshot variable, use infoschema.GetInfoSchema(ctx)

How it Works:

func GetInfoSchemaBySessionVars(sessVar *variable.SessionVars) InfoSchema {
	var is InfoSchema
	if snap := sessVar.SnapshotInfoschema; snap != nil {
		is = snap.(InfoSchema)
		logutil.BgLogger().Info("use snapshot schema", zap.Uint64("conn", sessVar.ConnectionID), zap.Int64("schemaVersion", is.SchemaMetaVersion()))
	} else {
		is = sessVar.TxnCtx.InfoSchema.(InfoSchema)
	}
	return is
}

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Manual test (add detailed scripts or steps below)

It's not written as an unit test, because the test case depends on TiKV.

func (s *testInfoschemaTableSuite) TestWrongSnapshotSchema(c *C) {
	// Test case for https://github.com/pingcap/tidb/issues/18347
	// getAutoIncrementID does not use the snapshot infoschema and cause this bug.
	tk := testkit.NewTestKit(c, s.store)
	tk.MustExec("create database gzj_test_db")
	tk.MustExec("create table gzj_test_db.ad_account_record_log (id int auto_increment primary key)")
	rs := tk.MustQuery("select now()").Rows()
	tk.MustExec("drop database gzj_test_db")

	tk.MustExec("set @@tidb_snapshot = ?", rs[0][0])
	// Before the bug fix, this query get "'Table gzj_test_db.ad_account_record_log' doesn't exist"
	tk.MustQuery("select * from information_schema.tables;")
}

Release note

  • Fix a bug `getAutoIncrementID()` function does not consider the `tidb_snapshot` session variable, this bug may cause dumper tool fail with 'table not exist' error.

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor Author

/run-all-tests

@tiancaiamao
Copy link
Contributor

PTAL @bb7133 @lzmhhh123

Copy link
Contributor

@lzmhhh123 lzmhhh123 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 Jul 21, 2020
@ti-srebot
Copy link
Contributor Author

@lonng,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: execution(slack).

@imtbkcat imtbkcat modified the milestones: v4.0.4, v4.0.5 Jul 28, 2020
Copy link
Member

@zz-jason zz-jason 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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 28, 2020
@zz-jason
Copy link
Member

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 28, 2020
@ti-srebot
Copy link
Contributor Author

Your auto merge job has been accepted, waiting for:

  • 18368
  • 17926
  • 17823
  • 18531
  • 18667
  • 18806
  • 18413
  • 18434
  • 18280
  • 18121
  • 18122
  • 18027
  • 18234
  • 18529
  • 18727
  • 18583
  • 18513
  • 17972
  • 17231

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor Author

Your auto merge job has been accepted, waiting for:

  • 18280
  • 18121
  • 18122
  • 18027
  • 18234
  • 18529
  • 18727
  • 18583
  • 18513
  • 17972
  • 17231
  • 17988
  • 18683
  • 18159
  • 17863
  • 17823
  • 18531
  • 18413
  • 18434
  • 18280
  • 18121
  • 18122
  • 18027
  • 18234
  • 18529
  • 18727
  • 18583
  • 18513
  • 17972
  • 17231

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot ti-srebot merged commit 8600bb3 into pingcap:release-4.0 Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bug-fix This PR fixes a bug. type/4.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants