Skip to content

fix(plan): reject invalid ALTER COLUMN defaults - #26880

Merged
XuPeng-SH merged 8 commits into
matrixorigin:mainfrom
ck89119:issue-26844-main
Aug 10, 2026
Merged

fix(plan): reject invalid ALTER COLUMN defaults#26880
XuPeng-SH merged 8 commits into
matrixorigin:mainfrom
ck89119:issue-26844-main

Conversation

@ck89119

@ck89119 ck89119 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #26844

What this PR does / why we need it:

Reject ALTER TABLE ... ALTER COLUMN ... SET DEFAULT for auto-increment and generated columns before mutating the copied table definition. This prevents successful no-op DDL when the requested default cannot be persisted.

Add planner unit tests for auto-increment, stored generated, and virtual generated columns, plus distributed SQL coverage that verifies rejected operations leave the schema unchanged and valid defaults still work.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

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

Reviewed exact head 2f4b955 after the successful CI rollup. The SET DEFAULT guards correctly reject auto-increment and generated columns before mutating the copied definition, with planner and BVT coverage.

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

Deep review — approve

Reviewed exact head 2f4b955b0b26cd109cf0f9237fcd27df11f7c0f2 against merge-base 8be242b25bf9a44d73a3b1cc1db75fd0264a293c.

No blocking issue found.

The new guards reject SET DEFAULT for auto-increment and both stored and virtual generated columns before assigning to CopyTableDef, so planner failure cannot publish a partially changed schema. The normal-column path and DROP DEFAULT behavior remain unchanged. The test matrix includes the three rejected metadata states, a normal-column control, copied-definition immutability, and public SQL checks that verify post-error schema and insert behavior.

Correctness, compatibility, performance, and unhappy-path review are clean: the change adds only constant-time checks on a DDL planning path; the existing temporary AST remains released by its defer; no new waits, shared state, resource ownership, or unbounded growth are introduced. The branch is one unrelated commit behind current main, and the merge is clean.

Validation on the exact head:

  • focused planner regressions passed;
  • full ./pkg/sql/plan tests passed under the controlled CGo environment;
  • planner build and vet passed;
  • CI executed alter_table_set_default_restrictions.sql, and the required CI rollup is green.

@XuPeng-SH
XuPeng-SH merged commit df49b09 into matrixorigin:main Aug 10, 2026
25 checks passed
@ck89119
ck89119 deleted the issue-26844-main branch August 10, 2026 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/M Denotes a PR that changes [100,499] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants