storage: report specific errors for incompatible Postgres schema changes

[peterdukelarsen]

Motivation

Part of SS-464 (adding support for exclude constraints) to Postgres.

Specifically, this we want to give end-users clean errors in the case where a table stalls.

Example from the PRD:

ERROR: incompatible schema change on public.users: UNIQUE constraint
"users_wallet_id_key" (wallet_id) was dropped upstream
HINT: To keep ingesting without this constraint, recreate the table in a new
versioned schema, excluding it, then swap your views to the new table:
  CREATE SCHEMA v2;
  CREATE TABLE v2.users
  FROM SOURCE pg_source (REFERENCE public.users)
  WITH (EXCLUDE CONSTRAINTS ('users_wallet_id_key'));

Description

determine_compatibility now steps through the differences like constraint changes, column nullability changes, or column detail changes and produces a tailored message for that. We wire up a hint field into SourceError for surfacing hints to the end-user.

Worth calling out that existing error messages (and future error messages + hints) will not be updated by this change because we're just plugging in to the SourceError type which basically just has opaque strings for error messages. There may be a better way to handle this encoding specific typed errors, but not a blocker because an error message that's months old and therefor stale is probably from a table that's not very important.

There are a couple of things I've learned wrt. upgrade safety worth calling out:

1. The None case for hint will serialize without any record of the hint field in the bytes. This means the addition of the hint field will be invisible for other errors. This is important for errors like DefiniteError::InvalidUTF8 which could potentially be retracted.
2. Speaking of retractions I've confirmed that IncompatibleSchema errors (which this change touches) aren't retracted. This should leave the output collections in a consistent end state.

Verification

Fleshed out testing here to validate hints are generated as desired

🤖 Generated with Claude Code

[linear-code]

SS-464

[def-]

QA LLM Review

1. MEDIUM -- Recovery hint recommends EXCLUDE [ALL] CONSTRAINTS, which is rejected unless a default-off feature flag is set

src/postgres-util/src/schema_change.rs:106

The hint emitted for a dropped NOT NULL constraint offers WITH (EXCLUDE ALL CONSTRAINTS) as its only suggested statement, and the dropped/renamed-key hint offers WITH (EXCLUDE CONSTRAINTS ('...')). Both options are gated behind enable_exclude_constraints_option, which defaults to false everywhere except mzcompose, so a user who follows the hint while their table is stalled gets a "not supported" planning error instead of a recovery path.

Details

enable_exclude_constraints_option is declared default: false at src/sql/src/session/vars/definitions.rs:2269, and the only place in the tree that turns it on is misc/python/materialize/mzcompose/__init__.py:105. Purification calls scx.require_feature_flag(&ENABLE_EXCLUDE_CONSTRAINTS_OPTION)? at src/sql/src/pure.rs:1824 for both spellings, so CREATE TABLE ... WITH (EXCLUDE ALL CONSTRAINTS) fails to plan in a default-configured environment. The option is also not documented under doc/user/.

The NotNullDropped case is the worst of the two, because recreate(Some("EXCLUDE ALL CONSTRAINTS")) at line 106 produces the entire hint. There is no fallback sentence, and the plain recreate that the hint's own prose describes would actually work today: after the upstream DROP NOT NULL, a fresh CREATE TABLE v2.t FROM SOURCE <source> (REFERENCE ns.t); records nullable = true and ingests normally. So the hint substitutes a statement that fails for one that would have succeeded. For KeyDropped/KeyAltered (line 100) the first half of the hint is still runnable; only the trailing "create the table with WITH (EXCLUDE CONSTRAINTS (...))" sentence is affected.

Two things make this hard to notice: the hint is persisted into the source's error output and into mz_source_status_history.details, so text emitted while the flag is off outlives the moment it was produced; and the new testdrive assertions (test/pg-cdc/alter-table-after-source-1.td:197, :230, :291) only match the hint string, never execute the SQL it recommends, and run under mzcompose where the flag is forced on.

Suggested fix: keep the EXCLUDE ... guidance out of the hint until the flag defaults on, or phrase it as conditional. Concretely, for the NotNullDropped arm, emit recreate(None) (which works today) and, if the extra guidance is worth keeping, append it as an optional hardening step rather than as the statement to run:

SchemaChange::NotNullDropped { .. } => Some(recreate(None)),

Worth noting separately: EXCLUDE CONSTRAINTS ('x') is validated against the current upstream constraint set (src/sql/src/pure/postgres.rs:466, PgSourcePurificationError::ConstraintsNotFound), so once the constraint is gone upstream that clause can never be used. The "before the upstream drop" qualifier in the hint is doing real work and should stay prominent; for KeyAltered (a rename) the named constraint is already gone under that name by the time the user reads the hint.

2. LOW -- Hint interpolates upstream schema and table names into SQL without identifier quoting

src/postgres-util/src/schema_change.rs:90

recreate splices self.namespace and self.name straight into CREATE TABLE v2.{} and FROM SOURCE <source> (REFERENCE {}.{}). Postgres identifiers that are not all-lowercase or that contain spaces or quotes are common in ORM-generated schemas, and for those the emitted statement is either invalid or silently wrong.

Details

For an upstream table created as CREATE TABLE "Orders" (...), the hint renders FROM SOURCE <source> (REFERENCE public.Orders). The parser folds the unquoted Orders ident to orders, which does not match the upstream Orders, so the suggested statement fails with a reference-not-found error rather than recovering the table. The constraint name is handled correctly (line 104 escapes it for a string literal, and EXCLUDE CONSTRAINTS matching is case-sensitive), so the gap is only in the two identifier positions. Rendering them with the same quoting used elsewhere for unresolved item names would close it.

[antiguru]

I think this is fine because the errors are terminal and we never try to retract a schema change error. Otherwise, we would leave errors that do no sum to 0 behind, which would be a violation of contract for data in persist. Please ensure that what I'm assuming is true!

[peterdukelarsen]

@antiguru I've confirmed the errors I'm changing the message of and adding hints to are not retracted (only plus ones no minus ones).

[peterdukelarsen]

LLM feedback is somewhat valid, but rejected for this PR. I'll handle it by flipping the flag alongside the tests PR to enable it by default (don't want to enable by default until tests are merged). I'll wait to merge this and merge both PRs at the same time.

[patrickwwbutler]

Stamped because none of my nits should be blocking, but a few suggestions for name improvements/error message nit

[patrickwwbutler]

maybe worth an updated name, esp given that compatibility is already used to refer to compatibility between schemas above, and the actual usage of the function is different now, as it's no longer a boolean function:

Suggested change

	fn is_compatible(
	fn get_incompatible_schema_changes(

this is a bit verbose, so feel free to change, but the idea is to make sure the name reflects what it's doing. a function name is_something seems like it should return a boolean

[peterdukelarsen]

sounds good

[patrickwwbutler]

I also don't love this name, not really clear that it's constructing an Error from an existing SchemaChange. I'd almost say that it doesn't really belong in this struct, and should be an additional constructor (but rusty) in the SchemaChangeError struct

[peterdukelarsen]

I think the trick with this one is that we're accessing the internal namespace, name and oid of this object. It would be weird to me for the SchemaChangeError struct to be aware of this PostgresTableDesc type. As a compromise let's go with fn build_schema_change_error.

[patrickwwbutler]

Suggested change

	/// The recovery steps for a dropped constraint, including the statements
	pub fn new(table_desc: &PostgresTableDesc, change: SchemaChange) -> Self {
	Self {
	table_desc.namespace.clone(),
	table_desc.name.clone(),
	table_desc.oid,
	change,
	}
	}
	/// The recovery steps for a dropped constraint, including the statements

a possible replacement for the schema_change func that I didn't like

[peterdukelarsen]

See https://github.com/MaterializeInc/materialize/pull/38525/changes#r3971373316

[patrickwwbutler]

so technically a mismatched oid, but matching namespace/name would indicate that the table was dropped/recreated (I think?), so the error message could be slightly misleading? Not sure it's that important, this is still an improvement over the baseline but I figured I'd mention

[peterdukelarsen]

That's a good point. I'm curious what case an OID could change and we'd still be comparing the table here? I think in practice it should just show up upstream with a TableDropped error. Let's plug in a warn that it's unexpected to see a mismatched oid and return a new TableDropped option for that.