From 14ff60dca07cc1e91e00adc3f424c9a3cd52d053 Mon Sep 17 00:00:00 2001 From: jiyfhust Date: Wed, 8 Nov 2023 10:56:12 +0800 Subject: [PATCH] ddl: fix create and alter check constraints problems (#47633) close pingcap/tidb#47567, close pingcap/tidb#47631, close pingcap/tidb#47632 --- pkg/ddl/constraint.go | 72 +++++++++++-------- pkg/ddl/rollingback.go | 49 +++++++++---- tests/integrationtest/r/ddl/constraint.result | 32 ++++++++- tests/integrationtest/t/ddl/constraint.test | 21 +++++- 4 files changed, 127 insertions(+), 47 deletions(-) diff --git a/pkg/ddl/constraint.go b/pkg/ddl/constraint.go index 02b9843887caf..1eaee3c618fa0 100644 --- a/pkg/ddl/constraint.go +++ b/pkg/ddl/constraint.go @@ -29,7 +29,6 @@ import ( "github.com/pingcap/tidb/pkg/parser/model" "github.com/pingcap/tidb/pkg/parser/mysql" "github.com/pingcap/tidb/pkg/sessionctx" - "github.com/pingcap/tidb/pkg/table" "github.com/pingcap/tidb/pkg/util/dbterror" "github.com/pingcap/tidb/pkg/util/sqlexec" ) @@ -37,11 +36,7 @@ import ( func (w *worker) onAddCheckConstraint(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64, err error) { // Handle the rolling back job. if job.IsRollingback() { - ver, err = onDropCheckConstraint(d, t, job) - if err != nil { - return ver, errors.Trace(err) - } - return ver, nil + return rollingBackAddConstraint(d, t, job) } failpoint.Inject("errorBeforeDecodeArgs", func(val failpoint.Value) { @@ -84,23 +79,37 @@ func (w *worker) onAddCheckConstraint(d *ddlCtx, t *meta.Meta, job *model.Job) ( constraintInfoInMeta = constraintInfoInJob } - originalState := constraintInfoInMeta.State + // If not enforced, add it directly. + if !constraintInfoInMeta.Enforced { + constraintInfoInMeta.State = model.StatePublic + ver, err = updateVersionAndTableInfo(d, t, job, tblInfo, true) + if err != nil { + return ver, errors.Trace(err) + } + // Finish this job. + job.FinishTableJob(model.JobStateDone, model.StatePublic, ver, tblInfo) + return ver, nil + } + switch constraintInfoInMeta.State { case model.StateNone: job.SchemaState = model.StateWriteOnly constraintInfoInMeta.State = model.StateWriteOnly - ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, originalState != constraintInfoInMeta.State) + ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, true) case model.StateWriteOnly: job.SchemaState = model.StateWriteReorganization constraintInfoInMeta.State = model.StateWriteReorganization - ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, originalState != constraintInfoInMeta.State) + ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, true) case model.StateWriteReorganization: - err = w.verifyRemainRecordsForCheckConstraint(dbInfo, tblInfo, constraintInfoInMeta, job) + err = w.verifyRemainRecordsForCheckConstraint(dbInfo, tblInfo, constraintInfoInMeta) if err != nil { + if dbterror.ErrCheckConstraintIsViolated.Equal(err) { + job.State = model.JobStateRollingback + } return ver, errors.Trace(err) } constraintInfoInMeta.State = model.StatePublic - ver, err = updateVersionAndTableInfo(d, t, job, tblInfo, originalState != constraintInfoInMeta.State) + ver, err = updateVersionAndTableInfo(d, t, job, tblInfo, true) if err != nil { return ver, errors.Trace(err) } @@ -151,12 +160,11 @@ func onDropCheckConstraint(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64, return ver, errors.Trace(err) } - originalState := constraintInfo.State switch constraintInfo.State { case model.StatePublic: job.SchemaState = model.StateWriteOnly constraintInfo.State = model.StateWriteOnly - ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, originalState != constraintInfo.State) + ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, true) case model.StateWriteOnly: // write only state constraint will still take effect to check the newly inserted data. // So the dependent column shouldn't be dropped even in this intermediate state. @@ -167,16 +175,11 @@ func onDropCheckConstraint(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64, tblInfo.Constraints = append(tblInfo.Constraints[0:i], tblInfo.Constraints[i+1:]...) } } - ver, err = updateVersionAndTableInfo(d, t, job, tblInfo, originalState != constraintInfo.State) + ver, err = updateVersionAndTableInfo(d, t, job, tblInfo, true) if err != nil { return ver, errors.Trace(err) } - // Finish this job. - if job.IsRollingback() { - job.FinishTableJob(model.JobStateRollbackDone, model.StateNone, ver, tblInfo) - } else { - job.FinishTableJob(model.JobStateDone, model.StateNone, ver, tblInfo) - } + job.FinishTableJob(model.JobStateDone, model.StateNone, ver, tblInfo) default: err = dbterror.ErrInvalidDDLJob.GenWithStackByArgs("constraint", tblInfo.State) } @@ -212,29 +215,38 @@ func (w *worker) onAlterCheckConstraint(d *ddlCtx, t *meta.Meta, job *model.Job) return ver, errors.Trace(err) } + if job.IsRollingback() { + return rollingBackAlterConstraint(d, t, job) + } + + // Current State is desired. + if constraintInfo.State == model.StatePublic && constraintInfo.Enforced == enforced { + job.FinishTableJob(model.JobStateDone, model.StatePublic, ver, tblInfo) + return + } + // enforced will fetch table data and check the constraint. if enforced { - originalState := constraintInfo.State switch constraintInfo.State { case model.StatePublic: job.SchemaState = model.StateWriteReorganization constraintInfo.State = model.StateWriteReorganization constraintInfo.Enforced = enforced - ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, originalState != constraintInfo.State) + ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, true) case model.StateWriteReorganization: job.SchemaState = model.StateWriteOnly constraintInfo.State = model.StateWriteOnly - ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, originalState != constraintInfo.State) + ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, true) case model.StateWriteOnly: - err = w.verifyRemainRecordsForCheckConstraint(dbInfo, tblInfo, constraintInfo, job) + err = w.verifyRemainRecordsForCheckConstraint(dbInfo, tblInfo, constraintInfo) if err != nil { - if !table.ErrCheckConstraintViolated.Equal(err) { - return ver, errors.Trace(err) + if dbterror.ErrCheckConstraintIsViolated.Equal(err) { + job.State = model.JobStateRollingback } - constraintInfo.Enforced = !enforced + return ver, errors.Trace(err) } constraintInfo.State = model.StatePublic - ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, originalState != constraintInfo.State) + ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, true) if err != nil { return ver, errors.Trace(err) } @@ -336,7 +348,7 @@ func findDependentColsInExpr(expr ast.ExprNode) map[string]struct{} { return colsMap } -func (w *worker) verifyRemainRecordsForCheckConstraint(dbInfo *model.DBInfo, tableInfo *model.TableInfo, constr *model.ConstraintInfo, job *model.Job) error { +func (w *worker) verifyRemainRecordsForCheckConstraint(dbInfo *model.DBInfo, tableInfo *model.TableInfo, constr *model.ConstraintInfo) error { // Inject a fail-point to skip the remaining records check. failpoint.Inject("mockVerifyRemainDataSuccess", func(val failpoint.Value) { if val.(bool) { @@ -363,8 +375,6 @@ func (w *worker) verifyRemainRecordsForCheckConstraint(dbInfo *model.DBInfo, tab } rowCount := len(rows) if rowCount != 0 { - // If check constraint fail, the job state should be changed to canceled, otherwise it will tracked in. - job.State = model.JobStateCancelled return dbterror.ErrCheckConstraintIsViolated.GenWithStackByArgs(constr.Name.L) } return nil diff --git a/pkg/ddl/rollingback.go b/pkg/ddl/rollingback.go index 3ea1082a28b35..44ee0f2d0b452 100644 --- a/pkg/ddl/rollingback.go +++ b/pkg/ddl/rollingback.go @@ -498,8 +498,7 @@ func convertJob2RollbackJob(w *worker, d *ddlCtx, t *meta.Meta, job *model.Job) model.ActionModifyTableCharsetAndCollate, model.ActionModifySchemaCharsetAndCollate, model.ActionRepairTable, model.ActionModifyTableAutoIdCache, model.ActionAlterIndexVisibility, - model.ActionModifySchemaDefaultPlacement, - model.ActionRecoverSchema, model.ActionAlterCheckConstraint: + model.ActionModifySchemaDefaultPlacement, model.ActionRecoverSchema: ver, err = cancelOnlyNotHandledJob(job, model.StateNone) case model.ActionMultiSchemaChange: err = rollingBackMultiSchemaChange(job) @@ -507,6 +506,8 @@ func convertJob2RollbackJob(w *worker, d *ddlCtx, t *meta.Meta, job *model.Job) ver, err = rollingBackAddConstraint(d, t, job) case model.ActionDropCheckConstraint: ver, err = rollingBackDropConstraint(t, job) + case model.ActionAlterCheckConstraint: + ver, err = rollingBackAlterConstraint(d, t, job) default: job.State = model.JobStateCancelled err = dbterror.ErrCancelledDDLJob @@ -554,7 +555,6 @@ func convertJob2RollbackJob(w *worker, d *ddlCtx, t *meta.Meta, job *model.Job) } func rollingBackAddConstraint(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64, err error) { - job.State = model.JobStateRollingback _, tblInfo, constrInfoInMeta, _, err := checkAddCheckConstraint(t, job) if err != nil { return ver, errors.Trace(err) @@ -565,18 +565,17 @@ func rollingBackAddConstraint(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int6 job.State = model.JobStateCancelled return ver, dbterror.ErrCancelledDDLJob } - // Add constraint has stored constraint info into meta, that means the job has at least - // arrived write only state. - originalState := constrInfoInMeta.State - constrInfoInMeta.State = model.StateWriteOnly - job.SchemaState = model.StateWriteOnly - - job.Args = []interface{}{constrInfoInMeta.Name} - ver, err = updateVersionAndTableInfo(d, t, job, tblInfo, originalState != constrInfoInMeta.State) - if err != nil { - return ver, errors.Trace(err) + for i, constr := range tblInfo.Constraints { + if constr.Name.L == constrInfoInMeta.Name.L { + tblInfo.Constraints = append(tblInfo.Constraints[0:i], tblInfo.Constraints[i+1:]...) + break + } } - return ver, dbterror.ErrCancelledDDLJob + if job.IsRollingback() { + job.State = model.JobStateRollbackDone + } + ver, err = updateVersionAndTableInfo(d, t, job, tblInfo, true) + return ver, errors.Trace(err) } func rollingBackDropConstraint(t *meta.Meta, job *model.Job) (ver int64, err error) { @@ -594,3 +593,25 @@ func rollingBackDropConstraint(t *meta.Meta, job *model.Job) (ver int64, err err job.State = model.JobStateRunning return ver, nil } + +func rollingBackAlterConstraint(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64, err error) { + _, tblInfo, constraintInfo, enforced, err := checkAlterCheckConstraint(t, job) + if err != nil { + return ver, errors.Trace(err) + } + + // StatePublic means when the job is not running yet. + if constraintInfo.State == model.StatePublic { + job.State = model.JobStateCancelled + return ver, dbterror.ErrCancelledDDLJob + } + + // Only alter check constraints ENFORCED can get here. + constraintInfo.Enforced = !enforced + constraintInfo.State = model.StatePublic + if job.IsRollingback() { + job.State = model.JobStateRollbackDone + } + ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, true) + return ver, errors.Trace(err) +} diff --git a/tests/integrationtest/r/ddl/constraint.result b/tests/integrationtest/r/ddl/constraint.result index 803643e47e192..06b0679415857 100644 --- a/tests/integrationtest/r/ddl/constraint.result +++ b/tests/integrationtest/r/ddl/constraint.result @@ -720,7 +720,6 @@ insert into t values(1), (2), (3); alter table t add constraint check(a < 2); Error 3819 (HY000): Check constraint 't_chk_1' is violated. alter table t add constraint check(a < 2) not enforced; -Error 3819 (HY000): Check constraint 't_chk_1' is violated. drop table if exists t; set @@global.tidb_enable_check_constraint = 1; create table t(a int not null check(a>0), b int, constraint haha check(a < b), check(a 0)) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin +drop table if exists t; +set @@global.tidb_enable_check_constraint = 1; +CREATE TABLE `t` (`a` int(11) DEFAULT NULL); +show create table t; +Table Create Table +t CREATE TABLE `t` ( + `a` int(11) DEFAULT NULL +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin +insert t values(1); +select * from t; +a +1 +alter table t ADD CONSTRAINT chk CHECK (a > 1) ENFORCED; +Error 3819 (HY000): Check constraint 'chk' is violated. +alter table t ADD CONSTRAINT chk CHECK (a > 1) ENFORCED; +Error 3819 (HY000): Check constraint 'chk' is violated. +alter table t ADD CONSTRAINT chk CHECK (a > 1) NOT ENFORCED; +ALTER TABLE t ALTER CONSTRAINT chk ENFORCED; +Error 3819 (HY000): Check constraint 'chk' is violated. +show create table t; +Table Create Table +t CREATE TABLE `t` ( + `a` int(11) DEFAULT NULL, +CONSTRAINT `chk` CHECK ((`a` > 1)) /*!80016 NOT ENFORCED */ +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin +alter table t drop CONSTRAINT chk; +show create table t; +Table Create Table +t CREATE TABLE `t` ( + `a` int(11) DEFAULT NULL +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin set @@global.tidb_enable_check_constraint = 0; diff --git a/tests/integrationtest/t/ddl/constraint.test b/tests/integrationtest/t/ddl/constraint.test index 8a53e8a79ad37..d197575970820 100644 --- a/tests/integrationtest/t/ddl/constraint.test +++ b/tests/integrationtest/t/ddl/constraint.test @@ -658,4 +658,23 @@ show create table t; # _, err = tk.Exec("alter table t alter constraint haha enforced") # require.Errorf(t, err, "[table:3819]Check constraint 'haha' is violated.") -set @@global.tidb_enable_check_constraint = 0; \ No newline at end of file +# Related issue TiDB#47567, #47631 and #47632. +# TestCheckConstraintIssue47567 +drop table if exists t; +set @@global.tidb_enable_check_constraint = 1; +CREATE TABLE `t` (`a` int(11) DEFAULT NULL); +show create table t; +insert t values(1); +select * from t; +-- error 3940 +alter table t ADD CONSTRAINT chk CHECK (a > 1) ENFORCED; +-- error 3940 +alter table t ADD CONSTRAINT chk CHECK (a > 1) ENFORCED; +alter table t ADD CONSTRAINT chk CHECK (a > 1) NOT ENFORCED; +-- error 3940 +ALTER TABLE t ALTER CONSTRAINT chk ENFORCED; +show create table t; +alter table t drop CONSTRAINT chk; +show create table t; + +set @@global.tidb_enable_check_constraint = 0;