Skip to content

sql: support EXCLUDE ALL NOT NULL CONSTRAINTS for MySQL sources - #39873

Merged
peterdukelarsen merged 3 commits into
MaterializeInc:mainfrom
peterdukelarsen:plarsen/mysql-exclude-not-null-constraints
Oct 16, 2026
Merged

peterdukelarsen merged 3 commits into
MaterializeInc:mainfrom
peterdukelarsen:plarsen/mysql-exclude-not-null-constraints

Conversation

@peterdukelarsen

@peterdukelarsen peterdukelarsen commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Closes SS-518

Motivation

#39680 added EXCLUDE ALL NOT NULL CONSTRAINTS to CREATE TABLE .. FROM SOURCE. The parser, option plumbing, and feature flag gate are source-agnostic, but only the Postgres purifier applied it, so MySQL tables silently kept their NOT NULL constraints.

Description

Thread the flag through the MySQL purifier and mark every column nullable, matching the Postgres behavior. The unsupported-source guard in purify_create_table_from_source now also covers this option, so source types without support reject it instead of ignoring it.

Columns of a retained primary key stay NOT NULL. The planner declares them NOT NULL regardless of the purified desc, so marking them nullable disabled the decoder's NULL check for a column persist treats as non-nullable. MySQL schema verification compares keys by name and columns, so a key dropped and restored while the source was behind is not reported, and a NULL written in between panicked clusterd. Postgres compares keys by constraint OID, so a recreated key is reported before any such row is decoded, and its purifier is unchanged.

Verification

Extends test/mysql-cdc/exclude-constraints.td with the same cases #39680 added for Postgres: SHOW CREATE roundtrip, index key, column nullability, the combination with EXCLUDE CONSTRAINTS ('PRIMARY'), DROP NOT NULL as a non-event, and PRIMARY KEY and UNIQUE drops still erroring with the hint. Also adds the drop-and-restore-while-paused scenario from the review, which panicked clusterd before the second commit.

🤖 Generated with Claude Code

The option is parsed and gated for every source type but only the
Postgres purifier applied it, so MySQL tables silently kept their NOT
NULL constraints. Thread the flag through the MySQL purifier and mark
every column nullable, matching the Postgres behavior. The unsupported
source guard now also covers this option so other source types reject
it instead of ignoring it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@peterdukelarsen
peterdukelarsen requested a review from a team as a code owner October 15, 2026 15:57
@def-

def- commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

QA LLM Review

1. MEDIUM -- Retained primary-key columns lose their NULL guard, so a NULL id crash-loops clusterd

src/sql/src/pure/mysql.rs:475

The new loop marks every column nullable in the stored MySqlTableDesc, including columns of a retained PRIMARY KEY, but plan_source_export_desc (src/sql/src/plan/statement/ddl.rs:1547-1553) still declares those columns NOT NULL in the table's RelationDesc (the test asserts id false). The decoder's NOT NULL check (src/mysql-util/src/decoding.rs:254) is therefore off for a column persist treats as non-nullable. A NULL that reaches it panics clusterd at src/repr/src/row/encode.rs:150 ("tried pushing Null into non-nullable column"), and the replica crash-loops until the table is dropped.

Details

The NULL gets in when the source is behind while upstream briefly drops and restores the key. verify_schemas (src/storage/src/source/mysql/schemas.rs:21) compares against the current upstream schema, not the schema as of the DDL event. If the PK is back by the time the DDL is processed, the PK drop is never flagged.

Reproduced on the PR's CI image. Upstream table t (id INT PRIMARY KEY, v INT NOT NULL), then CREATE TABLE t_nn FROM SOURCE ... WITH (EXCLUDE ALL NOT NULL CONSTRAINTS). With the replica at REPLICATION FACTOR 0 (a paused container gives the same result), run this upstream:

ALTER TABLE t DROP PRIMARY KEY, MODIFY id INT NULL;
INSERT INTO t VALUES (NULL, 20);
UPDATE t SET id = 2 WHERE id IS NULL;
ALTER TABLE t MODIFY id INT NOT NULL, ADD PRIMARY KEY (id);

After the replica is restored, clusterd panicked in persist_sink::write_batches 5 times in 25 s, taking down everything else on the cluster. Control: the same image and sequence on a table without the option did not panic. The NULL row decoded to a ValueDecodeError, the UPDATE retracted it, and the table ended up correct.

Fix: leave retained primary-key columns as they are, so the stored desc matches what the planner declares. Making a PK column nullable upstream requires dropping the PK, which is already reported as KeyDropped, so nothing is lost:

             if exclude_all_not_null_constraints {
+                let pk_cols: BTreeSet<&str> = parsed_table
+                    .keys
+                    .iter()
+                    .filter(|k| k.is_primary)
+                    .flat_map(|k| k.columns.iter().map(String::as_str))
+                    .collect();
                 for c in &mut parsed_table.columns {
+                    if pk_cols.contains(c.name.as_str()) {
+                        continue;
+                    }
                     if let Some(column_type) = &mut c.column_type {

The Postgres path that this PR copies (src/sql/src/pure/postgres.rs:533) also marks PK columns nullable. As a result, generate_column_casts (postgres.rs:706) drops its ErrorIfNull guard for them. I did not reproduce it on Postgres.

… NULL CONSTRAINTS

The planner declares primary key columns NOT NULL in the RelationDesc
regardless of the purified desc, so marking them nullable at purification
disabled the decoder's NULL check for a column persist treats as
non-nullable. MySQL schema verification compares keys by name and
columns, so a key dropped and restored while the source was behind is
not reported, and a NULL written in between panicked clusterd. The MySQL
purifier now leaves columns of a retained primary key as they are.

Adds the drop-and-restore scenario to the MySQL testdrive file. It
panicked before this change.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@peterdukelarsen
peterdukelarsen force-pushed the plarsen/mysql-exclude-not-null-constraints branch from 00fc841 to ed07b9b Compare October 15, 2026 18:36
Comment thread src/sql/src/pure/mysql.rs Outdated
// in a column it treats as non-nullable, which panics. Schema
// verification compares keys by name and columns, so a key
// dropped and restored while the source was behind is not
// reported, and the NULL rows written in between do arrive.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the reason we need this to avoid panics for MySQL sources and not for Postgres sources is because of the detail that schemas are compared by name/column. Postgres has an OID that would differ and prevent this case.

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.

I vote to amend the comment.... with your comment!

@peterdukelarsen

peterdukelarsen commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor Author

I've addressed the LLM-review by leaving primary key not null constraints intact.

@peterdukelarsen
peterdukelarsen requested a review from a team October 15, 2026 19:21

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

code looks good, some updates to the comments would be nice, but not blocking. thanks @peterdukelarsen!

Comment thread src/sql/src/pure/mysql.rs
Comment on lines +491 to +496
if pk_cols.contains(c.name.as_str()) {
continue;
}
if let Some(column_type) = &mut c.column_type {
column_type.nullable = true;
}

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.

worth a comment to say why in PG we don't check PK cols, but here we do.

@peterdukelarsen
peterdukelarsen merged commit 3530ba6 into MaterializeInc:main Oct 16, 2026
82 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants