fix(plan): enforce CHECK constraints in REPLACE#25070
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
I reviewed the latest head from the CHECK-constraint / REPLACE unhappy-path angle and did not find a blocker.
The important ordering looks right: REPLACE builds the final replacement-row projection, injects the CHECK filter/assert before LOCK_OP and before MULTI_UPDATE, so a CHECK failure aborts before the old conflicting row can be deleted. INSERT and UPDATE are wired through the same row-level validation path, and the assert expression preserves SQL CHECK semantics by allowing TRUE or NULL and failing FALSE.
I also checked the catalog round-trip path. Persisting CHECK defs through the hidden stream config key is separated back out in GetTableDef, so normal table properties stay visible while CHECK metadata is restored into TableDef.Checks. The PlanDefToCstrDef signature changes have only the expected call sites.
One detail I specifically chased: this PR changes colName2Idx to mean final projection position so CHECK expressions address the written row, not table ordinals. I did not find a current blocker from that change; the INSERT index-table path that consumes the map uses final projection positions, and the REPLACE CHECK path runs on the final projection before writes.
Local checks:
git diff --check origin/main...HEAD
CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/vm/engine ./pkg/sql/plan -count=1
CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/sql/plan/function -run 'TestCheckConstraintAssertReturnsConstraintViolation|Test_funids' -count=1
CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/sql/plan -run 'TestReplacePlanChecksTableCheckConstraints|TestInsertPlanChecksTableCheckConstraints|TestUpdatePlanChecksTableCheckConstraints|TestBuildCreateTableStoresCheckConstraints|TestCheckConstraintRejectsSyntheticColumns|TestReplace' -count=1
CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/vm/engine/memoryengine ./pkg/vm/engine/disttae/cache ./pkg/vm/engine/disttae -run '^$' -count=0All passed. The PR is still behind main, so it will need an update/rebase before merge.
XuPeng-SH
left a comment
There was a problem hiding this comment.
I need to correct my previous approval after re-checking the active blocking reviews and the latest head. The REPLACE injection point itself is good, but the CHECK metadata that reaches that injection point can still be semantically wrong, so I do not think this is ready yet.
Blocker 1: ENFORCED / NOT ENFORCED is still dropped.
The parser AST already carries the enforcement mode: AttributeCheckConstraint.Enforced and CheckIndex.Enforced exist (pkg/sql/parsers/tree/create.go). But buildTableDefs() calls appendCheckDef() with only name + expression for both column and table checks, and appendCheckDef() persists only {Name, Check} into plan.CheckDef. Later appendCheckConstraintPlan() asserts every recovered check unconditionally.
That means an explicit CHECK (...) NOT ENFORCED is silently turned into an enforced runtime constraint. Please either persist the enforcement flag and skip runtime assertions for non-enforced checks, or reject unsupported NOT ENFORCED at DDL time instead of changing its meaning.
Relevant code:
pkg/sql/parsers/tree/create.go:AttributeCheckConstraint.Enforced,CheckIndex.Enforcedpkg/sql/plan/build_ddl.go: column/table CHECK calls toappendCheckDef()do not pass enforcement modepkg/sql/plan/build_constraint_util.go:appendCheckConstraintPlan()wraps every stored check incheck_constraint_assert(...)
Blocker 2: CHECK binding still uses the wrong scope/timing.
appendCheckDef() binds the expression immediately against the columns currently present in tableDef.Cols. That changes the expected scope rules:
- table-level CHECKs that appear before later column declarations cannot reference those later columns because they are not in
tableDef.Colsyet; - column-level CHECKs can see previously declared columns because the binder receives all columns accumulated so far, instead of only the column being defined.
Please collect the CHECK ASTs first with their kind/name/enforcement metadata, bind table-level checks after the full column list is known, and validate/bind column-level checks against only the defining column unless MatrixOne intentionally documents a different rule and adds tests for it.
The runtime ordering for REPLACE still looks right: once the stored CHECK metadata is correct, the filter/assert is placed before LOCK_OP and MULTI_UPDATE, so a failing REPLACE should not delete the old conflicting row.
Local checks I ran before re-checking the DDL blockers:
git diff --check origin/main...HEAD
CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/vm/engine ./pkg/sql/plan -count=1
CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/sql/plan/function -run 'TestCheckConstraintAssertReturnsConstraintViolation|Test_funids' -count=1
CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/sql/plan -run 'TestReplacePlanChecksTableCheckConstraints|TestInsertPlanChecksTableCheckConstraints|TestUpdatePlanChecksTableCheckConstraints|TestBuildCreateTableStoresCheckConstraints|TestCheckConstraintRejectsSyntheticColumns|TestReplace' -count=1
CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/vm/engine/memoryengine ./pkg/vm/engine/disttae/cache ./pkg/vm/engine/disttae -run '^$' -count=0Those pass, but they do not cover the two DDL semantics cases above.
What type of PR is this?
Which issue(s) this PR fixes:
issue #24950
What this PR does / why we need it:
Test Plan
go test ./pkg/vm/engine ./pkg/sql/plan -count=1./run.sh -p /Users/LoveYY/Develop/matrixorigin/matrixone/.worktrees/issue-24950-main/test/distributed/cases/dml/replace/replace.test -g -n