ddl: correct the job.SchemaState in DROP cases #34235

tangenta · 2022-04-26T09:23:35Z

What problem does this PR solve?

Issue Number: ref #32939

Problem Summary:

This PR is a patch for #34157.

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility
In the scenario of the rolling upgrade, if the DDL owner is transferred from an old version of TiDB to a newer one, admin cancel job may not work correctly.

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Refine the `SchemaState` field of job for the `DROP` DDLs.

ti-chi-bot · 2022-04-26T09:23:36Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

wjhuang2016
zimulala

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

zimulala · 2022-04-26T09:31:51Z

ddl/index.go

@@ -709,6 +706,7 @@ func onDropIndex(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64, _ error) {
 	default:
 		err = dbterror.ErrInvalidDDLState.GenWithStackByArgs("index", indexInfo.State)


If we do this change, do we need to do a return here? Otherwise, there is an error, but also change the schema version?

I think this is fine because the DDL state is incorrect anyway.

job.SchemaState is exactly used to show the indexInfo.State here.

However, this may cause a problem if it is later written to KV (DDL job should be retried).

zimulala · 2022-04-26T09:38:28Z

ddl/ddl_api.go

@@ -527,10 +527,11 @@ func (d *ddl) DropSchema(ctx sessionctx.Context, schema model.CIStr) (err error)
 		return errors.Trace(infoschema.ErrDatabaseNotExists)


Do we need to consider ModifyColumn, TruncateTable, ModifySchemaCharsetAndCollate, DropTablePartition, ExchangeTablePartition, SetDefaultValue, and so on.

Some of them don't even have the concept of schema state(like truncate table, modify charset/collate), and some involve non-trivial changes(like drop partition, modify columns).

How about ActionDropColumns?

DropColumns will be removed when multi-schema change is supported.

wjhuang2016

It should be mentioned in the release note.

sre-bot · 2022-04-26T12:35:52Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/eee54ce71b06fd0ede417d2d7b00d3ca8d2c7053

hawkingrei · 2022-04-26T15:19:32Z

/run-mysql-test

ddl/column_change_test.go

zimulala · 2022-04-28T02:41:10Z

ddl/foreign_key.go

 		return ver, nil
 	default:
+		job.SchemaState = fkInfo.State


Here we needn't do this?

zimulala · 2022-04-28T02:47:21Z

ddl/index.go

@@ -709,6 +706,7 @@ func onDropIndex(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64, _ error) {
 	default:
 		err = dbterror.ErrInvalidDDLState.GenWithStackByArgs("index", indexInfo.State)


However, this may cause a problem if it is later written to KV (DDL job should be retried).

zimulala · 2022-04-28T02:53:02Z

ddl/ddl_api.go

@@ -527,10 +527,11 @@ func (d *ddl) DropSchema(ctx sessionctx.Context, schema model.CIStr) (err error)
 		return errors.Trace(infoschema.ErrDatabaseNotExists)


How about ActionDropColumns?

zimulala

LGTM

zimulala · 2022-04-28T03:31:54Z

/merge

ti-chi-bot · 2022-04-28T03:31:57Z

This pull request has been accepted and is ready to merge.

Commit hash: d539d8d

tangenta · 2022-04-28T03:58:52Z

/merge

ti-chi-bot · 2022-04-28T03:58:55Z

This pull request has been accepted and is ready to merge.

Commit hash: eee54ce

sre-bot · 2022-04-28T04:28:57Z

TiDB MergeCI notify

🔴 Bad News! [1] CI still failing after this pr merged.
These failed integration tests don't seem to be introduced by the current PR.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/common-test	🔴 failed 2, success 10, total 12	6 min 14 sec	Existing failure
idc-jenkins-ci-tidb/integration-common-test	🟢 all 11 tests passed	9 min 35 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	6 min 35 sec	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	6 min 1 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	5 min 57 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	4 min 58 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	4 min 41 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 23 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

ddl: correct the job.SchemaState in DROP cases

d4b7f76

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 26, 2022

zimulala reviewed Apr 26, 2022

View reviewed changes

tangenta added the compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. label Apr 26, 2022

wjhuang2016 reviewed Apr 26, 2022

View reviewed changes

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 26, 2022

fix integration tests

4ab6e53

wjhuang2016 approved these changes Apr 27, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 27, 2022

correct job.SchemaState for drop partition

f04d7b9

zimulala reviewed Apr 28, 2022

View reviewed changes

tangenta added 3 commits April 28, 2022 11:10

revert unnecessary change

0691d40

remove unnecessary change

99a6ee4

address comment

d539d8d

zimulala approved these changes Apr 28, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 28, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 28, 2022

ti-chi-bot and others added 2 commits April 28, 2022 11:32

Merge branch 'master' into fix-test-cancel

c419f8f

fix integration test TestDropAndTruncatePartition

eee54ce

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Apr 28, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 28, 2022

ti-chi-bot merged commit 93859dd into pingcap:master Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: correct the job.SchemaState in DROP cases #34235

ddl: correct the job.SchemaState in DROP cases #34235

tangenta commented Apr 26, 2022 •

edited

Loading

ti-chi-bot commented Apr 26, 2022 •

edited

Loading

zimulala Apr 26, 2022

tangenta Apr 26, 2022

zimulala Apr 28, 2022

zimulala Apr 26, 2022

tangenta Apr 26, 2022 •

edited

Loading

zimulala Apr 28, 2022

tangenta Apr 28, 2022

wjhuang2016 left a comment

sre-bot commented Apr 26, 2022 •

edited

Loading

hawkingrei commented Apr 26, 2022

zimulala Apr 28, 2022

tangenta Apr 28, 2022

zimulala Apr 28, 2022

zimulala Apr 28, 2022

zimulala left a comment

zimulala commented Apr 28, 2022

ti-chi-bot commented Apr 28, 2022

tangenta commented Apr 28, 2022

ti-chi-bot commented Apr 28, 2022

sre-bot commented Apr 28, 2022

		@@ -709,6 +706,7 @@ func onDropIndex(d ddlCtx, t meta.Meta, job *model.Job) (ver int64, _ error) {
		default:
		err = dbterror.ErrInvalidDDLState.GenWithStackByArgs("index", indexInfo.State)

		@@ -527,10 +527,11 @@ func (d *ddl) DropSchema(ctx sessionctx.Context, schema model.CIStr) (err error)
		return errors.Trace(infoschema.ErrDatabaseNotExists)

ddl: correct the job.SchemaState in DROP cases #34235

ddl: correct the job.SchemaState in DROP cases #34235

Conversation

tangenta commented Apr 26, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangenta Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjhuang2016 left a comment

Choose a reason for hiding this comment

sre-bot commented Apr 26, 2022 • edited Loading

hawkingrei commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

zimulala commented Apr 28, 2022

ti-chi-bot commented Apr 28, 2022

tangenta commented Apr 28, 2022

ti-chi-bot commented Apr 28, 2022

sre-bot commented Apr 28, 2022

TiDB MergeCI notify

tangenta commented Apr 26, 2022 •

edited

Loading

ti-chi-bot commented Apr 26, 2022 •

edited

Loading

tangenta Apr 26, 2022 •

edited

Loading

sre-bot commented Apr 26, 2022 •

edited

Loading