Skip to content

feat(plan): support ALTER TABLE AUTO_INCREMENT = value#25883

Closed
VioletQwQ-0 wants to merge 90 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/takeover-24531-alter-auto-increment
Closed

feat(plan): support ALTER TABLE AUTO_INCREMENT = value#25883
VioletQwQ-0 wants to merge 90 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/takeover-24531-alter-auto-increment

Conversation

@VioletQwQ-0

@VioletQwQ-0 VioletQwQ-0 commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

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 = N across planning, execution, allocator state, COPY ALTER, schema epochs, recovery, and regression coverage.

The takeover review also closes the two remaining P1 invariants:

  • COPY allocator reconciliation now carries source state by stable destination column identity, including drop/add/reorder/rename and hidden fake-PK movement, instead of mixing source and destination positional indexes;
  • legacy CN writes without the epoch presence bit are treated as epoch zero, so they remain compatible before the first fenced ALTER but fail closed after the table epoch advances.

Validation:

  • GitHub reports the PR mergeable against current main;
  • go test -count=1 passed for pkg/sql/colexec/table_clone, pkg/sql/compile, pkg/vm/engine/tae/rpc, and pkg/vm/engine/tae/txn/txnimpl;
  • go vet and go build passed for the same affected packages;
  • focused race tests passed 10 consecutive runs for COPY identity reconciliation and TN epoch convergence;
  • changed-commit whitespace checks passed.

ULookup added 30 commits July 15, 2026 14:13
…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 XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@VioletQwQ-0

Copy link
Copy Markdown
Collaborator Author

@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.

@VioletQwQ-0

VioletQwQ-0 commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator Author

@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 main, with one independently reproducible invariant per PR and serial dependencies so reviewers never need to read an accumulated stack.

Split tracker:

The generic recovery items are being reproduced on current main and tracked independently. The feature prerequisites will reference #23143, while only the final user-facing PR will close it. Each PR will start as draft, stay within a small handwritten diff, list explicit non-goals, and pass focused tests plus self-review before being made ready.

I will update this same tracker as the serial PRs are merged instead of adding repeated status comments.

XuPeng-SH pushed a commit that referenced this pull request Jul 22, 2026
## 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants