ddl, mvservice: backport materialized view maintenance fixes#68947
Conversation
* fix * fixed; need more check and test * add tests
ti-chi-bot
Bot
added
release-note
📝 WalkthroughWalkthroughThis PR adds DDL validation for materialized view logs (duplicate-column detection and partition/exchange constraints) and implements server deduplication in mvservice by extending ChangesMaterialized View DDL Validation
Server Deduplication in mvservice
Sequence Diagram(s)sequenceDiagram
participant Client
participant Executor as DDL Executor
participant Tracker as Schema Tracker
Client->>Executor: CREATE MATERIALIZED VIEW LOG (col1, col1)
Executor->>Executor: Check duplicates in s.Cols
Executor->>Executor: Return ErrColumnExists
Note over Executor: Validation blocks creation
sequenceDiagram
participant Client
participant Executor as DDL Executor
participant Validator as MV Constraint Checker
Client->>Executor: ALTER TABLE t PARTITION BY ...
Executor->>Validator: checkBaseTableMaterializedViewDependencyConstraints
Validator->>Validator: Check for MView/MLog dependencies
alt Has MV Dependencies
Validator->>Executor: Return error
else No MV Dependencies
Executor->>Executor: Proceed with partition change
end
sequenceDiagram
participant Discovery as Server Discovery
participant Helper as Service Helper
participant Dedup as Deduplication Logic
Discovery->>Helper: getAllServerInfo()
Helper->>Dedup: latestServerInfosByInstance(serverMap)
Dedup->>Dedup: For each server, compute execution ID
Dedup->>Dedup: Keep newest StartTimestamp per instance
Dedup->>Helper: Return deduplicated map
Helper->>Discovery: Return deduplicated servers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @windtalker. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/executor/test/ddl/materialized_view_ddl_test.go (1)
63-75: ⚡ Quick winAdd test case for case-insensitive duplicate columns.
The duplicate column detection is case-insensitive (using
c.Lin the implementation), but this test only covers exact duplicates(id, id). Add a test case for mixed-case duplicates to verify the case-insensitive behavior.✅ Suggested additional test case
func TestCreateMaterializedViewLogRejectsDuplicateColumns(t *testing.T) { store := testkit.CreateMockStore(t) tk := testkit.NewTestKit(t, store) tk.MustExec("use test") tk.MustExec(`create table t_dup ( id bigint not null primary key, g1 int not null )`) err := tk.ExecToErr("create materialized view log on t_dup (id, id)") require.ErrorContains(t, err, "Duplicate column name") + + // Verify case-insensitive duplicate detection + err = tk.ExecToErr("create materialized view log on t_dup (id, ID)") + require.ErrorContains(t, err, "Duplicate column name") + + err = tk.ExecToErr("create materialized view log on t_dup (g1, G1)") + require.ErrorContains(t, err, "Duplicate column name") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/test/ddl/materialized_view_ddl_test.go` around lines 63 - 75, Add a second assertion to TestCreateMaterializedViewLogRejectsDuplicateColumns that verifies case-insensitive duplicate detection by attempting to create the materialized view log with the same column name in different case (e.g., "create materialized view log on t_dup (id, ID)") and asserting tk.ExecToErr returns an error containing "Duplicate column name"; locate this in the existing TestCreateMaterializedViewLogRejectsDuplicateColumns function and reuse the same test setup (store, tk, and table t_dup) so the new ExecToErr/require.ErrorContains line mirrors the existing duplicate-check assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/executor/test/ddl/materialized_view_ddl_test.go`:
- Around line 63-75: Add a second assertion to
TestCreateMaterializedViewLogRejectsDuplicateColumns that verifies
case-insensitive duplicate detection by attempting to create the materialized
view log with the same column name in different case (e.g., "create materialized
view log on t_dup (id, ID)") and asserting tk.ExecToErr returns an error
containing "Duplicate column name"; locate this in the existing
TestCreateMaterializedViewLogRejectsDuplicateColumns function and reuse the same
test setup (store, tk, and table t_dup) so the new
ExecToErr/require.ErrorContains line mirrors the existing duplicate-check
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 10254a95-18fb-41ac-a8e6-1bc25ac2bf54
📒 Files selected for processing (7)
pkg/ddl/executor.gopkg/ddl/schematracker/dm_tracker.gopkg/executor/test/ddl/materialized_view_ddl_test.gopkg/mvservice/BUILD.bazelpkg/mvservice/server_maintainer.gopkg/mvservice/service_helper.gopkg/mvservice/task_handler_test.go
|
@solotzg: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/release-8.5-materialized-view #68947 +/- ##
==========================================================================
Coverage ? 57.6700%
==========================================================================
Files ? 1812
Lines ? 655605
Branches ? 0
==========================================================================
Hits ? 378088
Misses ? 251288
Partials ? 26229
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/test unit-test |
|
@windtalker: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test pull-unit-test-ddlv1 |
|
@windtalker: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gengliqi, solotzg, xzhangxian1008 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
ti-chi-bot
Bot
merged commit 7a3b5d9
into
pingcap:feature/release-8.5-materialized-view
5 participants
What problem does this PR solve?
Issue Number: ref #18023
Problem Summary:
This PR backports several materialized view maintenance fixes to the release-8.5 materialized view feature branch.
What changed and how does it work?
Check List
Tests
Unit tests and checks:
go test ./pkg/executor/test/ddl -run 'Test(ExchangePartitionRejectsMaterializedViewRelatedTable|AlterTablePartitioningRejectsMaterializedViewBaseTable)$' -tags=intest,deadlock -count=1make bazel_lint_changedSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit