feat(plan): support ALTER TABLE AUTO_INCREMENT = value#25883
feat(plan): support ALTER TABLE AUTO_INCREMENT = value#25883VioletQwQ-0 wants to merge 90 commits into
Conversation
…INCREMENT column The previous error message was misleading (copy-pasted from MySQL's "too many auto columns" error). The new message includes the table name and accurately describes the problem.
…ets table without auto-increment column The old message was misleading — it said "there can be only one auto column and it must be defined as a key", which is MySQL's error for a different condition. The new message accurately states "Table '%s' does not have an AUTO_INCREMENT column" and includes the table name.
Previously the mock silently returned nil when SetOffset was called with an unknown tableID, while real implementations (memStore, sqlStore) panic or return errors. This could mask test bugs.
…TER TABLE AUTO_INCREMENT Add SetOffset to AutoIncrementService and IncrValueStore interfaces with guard that prevents lowering the offset — matching MySQL behavior where AUTO_INCREMENT counter can never go below existing values. Includes: - Interface + 3 implementations (memStore, sqlStore, mock) with < guard - service.SetOffset adapter (store update + Reload) - compile/ddl.go wiring: call SetOffset after AlterTable for UpdateAutoIncrement - Engine no-op cases for AlterKind_UpdateAutoIncrement (TAE + disttae) - Regenerated gomock stubs for frontend tests - Regression test: AUTO_INCREMENT=5 on populated table
- Skip AUTO_INCREMENT=0 action creation in inplace path (consistent with copy path) - Add backtick to SQL injection guard in store_sql.go - Add explicit nil check for uncommitted map in memStore.SetOffset - Add nil guard for GetAutoIncrementService in compile post-processing - Remove stale *tree.TableOptionAutoIncrement from unsupported comment list
The plan.Type protobuf has field auto_incr, which generates GetAutoIncr() not GetAutoIncrement(). Use direct field access (col.Typ.AutoIncr) to match the codebase convention. Fixes CI build failure for PR matrixorigin#24531.
- Add AutoIncr field to mock col struct and wire it to dept tables - Replace fmt.Errorf with moerr in mock_increament_service.go - Fixes TestAlterTableAutoIncrement test failure and SCA violations
… mock
The col struct used positional initialization (~60 instances in mock.go).
Adding AutoIncr bool to col broke every literal that wasn't updated, causing
SCA "too few values in struct literal" errors and cascading e2e failures.
Move auto_increment marking to Schema.autoIncrColName, aligning with MySQL's
model where AUTO_INCREMENT is a table-level property. This avoids changing
any col{} literal and reduces the fix surface from ~60 locations to 5.
…ly at SQL layer Remove engine-layer involvement for AlterKind_UpdateAutoIncrement. Following MySQL's kernel approach, ALTER TABLE t AUTO_INCREMENT = N now directly calls incrservice.SetOffset() in the SQL compile step, eliminating unnecessary AlterTableReq construction and post-processing. - proto/api.proto: remove UpdateAutoIncrement enum, message, oneof - pkg/pb/api/api.go: remove NewUpdateAutoIncrementReq() - pkg/sql/compile/ddl.go: move incrservice call into action handler - pkg/incrservice/store_mem.go: fix txnOp nil-check in SetOffset - pkg/incrservice/store_sql.go: replace ContainsAny with isValidColumnName whitelist - engine files: revert no-op UpdateAutoIncrement cases
- Inplace path: treat Value=0 as offset=0 (MySQL 8.0 compat), always create a valid action to avoid nil pointer dereference in deepcopy. - Copy path: same Value=0 handling, always initialize AutoIncrOffset. - store_mem.go: remove intermediate key variable (staticcheck SA6001). Fixes CI failures for issue matrixorigin#23143.
The ALTER TABLE AUTO_INCREMENT handler now returns a clearer error message when the table has no AUTO_INCREMENT column.
The `tableHasAutoIncrementColumn` helper was returning true for all tables without a primary key because `__mo_fake_pk_col` has `AutoIncrement=true` and `Hidden=true`. This caused ALTER TABLE AUTO_INCREMENT on tables without a user-defined AUTO_INCREMENT column to silently succeed instead of returning an error. Fix: add `!col.Hidden` condition to only consider user-visible columns.
…ENT support - Add !col.Hidden filter in GetAutoColumnFromDef to exclude hidden columns - Fix SetOffset cache ordering: Reload (invalidate) before store write to prevent dirty read - Fix dead nil checks in store_mem Allocate/UpdateMinValue (check c==nil instead of !ok) - Extract autoIncrementValueToOffset helper, use in CREATE TABLE, COPY, and inplace paths - Add never-decrease guard for AUTO_INCREMENT offset in COPY and inplace paths - Remove selective svc==nil check in compile/ddl.go to match existing caller pattern - Add TestAlterTableAutoIncrementOffsetValue for offset value assertions
govet flagged the `c != nil` guard as tautological after the preceding panic-on-nil check was added.
The !col.Hidden filter prevented maybeCreateAutoIncrement from creating incrservice caches for tables with only __mo_fake_pk_col. During bootstrap, this caused INSERT into information_schema.KEYWORDS to fail because getCommittedTableCache fell back to GetColumns with nil txnOp, creating a new transaction that couldn't see the uncommitted mo_increment_columns table. The ALTER TABLE AUTO_INCREMENT path is already gated by tableHasAutoIncrementColumn in build_alter_table.go, which correctly filters hidden columns.
Add end-to-end BVT test that exercises the ALTER TABLE AUTO_INCREMENT success path through SetOffset. The existing BVT only covered the error path (ALTER on a table without AUTO_INCREMENT column). This test covers: - ALTER TABLE AUTO_INCREMENT = 100 on table with auto_increment column - Verifies subsequent INSERT starts from the new offset - Verifies consecutive INSERTs continue incrementing correctly
XuPeng-SH
left a comment
There was a problem hiding this comment.
Blocking finding
P1 — Make MEMKV recovery delivery cancellation-aware (pkg/txn/storage/mem/kv_txn_storage.go:180)
replica.close() now calls CancelRecovery() and then waits for Start() to publish startedC. The recovery consumer exits when that context is canceled, but KVTxnStorage.StartRecovery still performs an unconditional c <- klog.Txn for every replayed user record. Since txnC has capacity 16, canceling after the consumer exits while more than 16 records are replayed leaves the producer blocked forever. It cannot return or close txnC, Start() cannot finish, and replica.close() hangs.
Please make each MEMKV recovery send cancellation-aware, as the TAE implementation already is:
select {
case <-ctx.Done():
return
case c <- klog.Txn:
}Please also add a regression that replays more than cap(txnC) records, cancels recovery after the consumer stops, and asserts startup/replica close completes.
|
@XuPeng-SH @gouhongshen Addressed the MEMKV recovery cancellation finding in 6c44b46. StartRecovery now selects on ctx.Done() for every transaction delivery, so cancellation terminates even when the 16-entry txn channel is full after the consumer exits. Added TestRecoveryCancellationUnblocksFullTxnChannel with 17 replay records; it reproduced the pre-fix hang and now passes. Validation: full pkg/txn/storage/mem, pkg/txn/service, and pkg/tnservice tests; focused race tests across storage/service/replica cancellation; go vet/build for all three packages; git diff --check; unhappy-path Q1-Q3 and mo-self-review. |
|
@fengttt Thank you for calling this out. You are right: even though transaction fencing and recovery are related to the intended correctness guarantee, putting all of them together with the SQL feature made this PR far too large to review reliably. The description also failed to explain which changes were prerequisites and which were general recovery fixes. I am sorry for creating that review burden. I have closed this PR and will not reopen or rewrite its branch. I am rebuilding the work from the latest Split tracker:
The generic recovery items are being reproduced on current I will update this same tracker as the serial PRs are merged instead of adding repeated status comments. |
## What type of PR is this? - [x] API-change - [ ] BUG - [x] Improvement - [ ] Documentation - [ ] Feature - [x] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: issue #23143 ## What this PR does / why we need it: This is the first AUTO_INCREMENT-specific prerequisite split from the closed #25883. It does not expose `ALTER TABLE ... AUTO_INCREMENT=N` yet. The invariant introduced here is: a CN write carries the AUTO_INCREMENT epoch observed when its values were allocated, and TN accepts the write only when that epoch matches the table epoch. Once a table enters a fenced epoch, legacy writers that cannot provide an epoch fail closed. | Correctness requirement | Change in this PR | | --- | --- | | Preserve the allocator generation across the write path | Persist the epoch in `SchemaExtra` / `plan.TableDef`, propagate it through disttae workspace, row and object writes, flush/compaction, RPC, and TN relations | | Prevent stale cached values from committing after a reset | Validate the known epoch at TN prepare/commit and require every epoch transition to be exactly `current + 1` | | Keep the fence effective after restart | Rebuild committed DML watermarks during replay and keep prepared replay transactions fenced until commit or rollback | | Remain compatible before the feature is enabled | Treat an unknown legacy epoch as zero only while the table epoch is zero; after the first reset, unknown writers are rejected | Additional guards reject epoch exhaustion before mutation and keep ordinary schema ALTER operations from advancing the AUTO_INCREMENT epoch. ### Explicit non-goals - no allocator offset/cache reset API; - no COPY/CLONE allocator reconciliation; - no user-facing `ALTER TABLE ... AUTO_INCREMENT=N` planner or compile entry point; - no generic prepared/committing 2PC recovery redesign. The independent TN route/cancellation prerequisite was merged in #25956. ### Diff size - handwritten code and tests: 23 files, `+1283/-131`; - generated protobuf code: 2 files, `+1565/-1079`; - total: 25 files. Proto sources and generated files are committed together. ### Validation - `go test ./pkg/vm/engine/disttae ./pkg/vm/engine/tae/catalog ./pkg/vm/engine/tae/rpc ./pkg/vm/engine/tae/txn/txnimpl ./pkg/vm/engine/tae/db/test` - focused `-race` coverage for epoch transitions and replay commit/rollback lifecycle - protobuf regeneration stability check - `git diff --check` - `mo-pr-preflight-review`: PASS at exact head `4122a7add078c79757121a4add2d8771113c7980` - `mo-self-review`: no open blocker The PR is intentionally Draft while its full CI runs and while this prerequisite is reviewed independently. --------- Co-authored-by: ULookup <CHBulookup@outlook.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
What type of PR is this?
Which issue(s) this PR fixes:
issue #23143
What this PR does / why we need it:
This PR continues the unfinished work from #24531 while preserving the original commits and authorship.
It implements
ALTER TABLE ... AUTO_INCREMENT = Nacross planning, execution, allocator state, COPY ALTER, schema epochs, recovery, and regression coverage.The takeover review also closes the two remaining P1 invariants:
Validation:
main;go test -count=1passed forpkg/sql/colexec/table_clone,pkg/sql/compile,pkg/vm/engine/tae/rpc, andpkg/vm/engine/tae/txn/txnimpl;go vetandgo buildpassed for the same affected packages;