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.

Sign up for GitHub

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

ddl: fix create and alter check constraints problems #47633

Merged
merged 5 commits into from
Nov 8, 2023
Merged

ddl: fix create and alter check constraints problems #47633

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2445f9d
fix some check constrants ddl bugs
jiyfhust Oct 14, 2023
2f9f363
fix
jiyfhust Oct 15, 2023
ccdf1ff
Merge branch 'master' of github.com:jiyfhust/tidb into fix_check_ddl
jiyfhust Oct 28, 2023
5ab124c
fix
jiyfhust Oct 28, 2023
a9b1fd8
fix test
jiyfhust Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 41 additions & 31 deletions pkg/ddl/constraint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,14 @@ 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"
)

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) {
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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.
Expand All @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand Down
18 changes: 18 additions & 0 deletions pkg/ddl/constraint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,3 +287,21 @@ func TestAlterEnforcedConstraintStateChange(t *testing.T) {
tk.MustExec("alter table t alter constraint c1 enforced")
tk.MustQuery("select * from t").Check(testkit.Rows("12"))
}

// Related issue TiDB#47567, #47631 and #47632.
func TestCheckConstraintIssue47567(t *testing.T) {
tiancaiamao marked this conversation as resolved.
Show resolved Hide resolved
store := testkit.CreateMockStore(t)
tk := testkit.NewTestKit(t, store)

tk.MustExec("set @@global.tidb_enable_check_constraint = 1")
tk.MustExec("use test")
tk.MustExec("CREATE TABLE `t` (`a` int(11) DEFAULT NULL)")
tk.MustExec("insert t values(1)")
tk.MustGetErrMsg("alter table t ADD CONSTRAINT chk CHECK (a > 1) ENFORCED", "[ddl:3819]Check constraint 'chk' is violated.")
Copy link
Member

@CbcWestwolf CbcWestwolf Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why are there two same SQLs here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As issue #47632 reproduce steps，the first sql will rollback failed, and the second will report "Duplicate check constraint name" error.

Copy link
Member

Choose a reason for hiding this comment

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

Got it!

tk.MustGetErrMsg("alter table t ADD CONSTRAINT chk CHECK (a > 1) ENFORCED", "[ddl:3819]Check constraint 'chk' is violated.")
tk.MustExec("alter table t ADD CONSTRAINT chk CHECK (a > 1) NOT ENFORCED")
tk.MustGetErrMsg("ALTER TABLE t ALTER CONSTRAINT chk ENFORCED;", "[ddl:3819]Check constraint 'chk' is violated.")
tk.MustQuery("select constraint_name from information_schema.CHECK_CONSTRAINTS where constraint_schema = 'test'").Check(testkit.Rows("chk"))
tk.MustExec("alter table t drop CONSTRAINT chk")
tk.MustQuery("select constraint_name from information_schema.CHECK_CONSTRAINTS where constraint_schema = 'test'").Check(testkit.Rows())
}
50 changes: 36 additions & 14 deletions pkg/ddl/rollingback.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,15 +498,16 @@ 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)
case model.ActionAddCheckConstraint:
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
Expand Down Expand Up @@ -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)
Expand All @@ -565,18 +565,18 @@ 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)
// Is there a case constrInfoInMeta.State become StatePublic that means the constraint have already been added successfully?
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) {
Expand All @@ -594,3 +594,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)
}