storage-controller: keep export descriptions in sync on ALTER SINK and skip no-op connection alters#37653
Draft
ggevay wants to merge 2 commits into
Draft
Conversation
alter_export sent the new sink definition to the cluster and updated the ExportState, but never wrote the new ExportDescription into the collection's DataSource::Sink. The stored description therefore kept the definition from sink creation forever. The staleness dates back to the sinks-as-collections refactor (273b3e4), which moved the description from ExportState into CollectionState but dropped the update here. alter_export_connections reads that stored description as the currently running sink and checks a newly computed connection against it with alter_compatible. Any catalog change to a connection (ALTER CONNECTION, GRANT, REVOKE, owner change) emits a Connection Altered implication, which recomputes the connection of every dependent sink from the catalog and calls alter_export_connections. After an ALTER SINK ... SET FROM had changed the sink's value_desc or relation_key_indices, that check failed with InvalidAlter and panicked the coordinator with "altering export connections after txn must succeed" (SQL-517). Fix by storing the new description in the collection state after sending the RunSink command, mirroring alter_export_connections. This also makes run_export rerun the current sink definition instead of the stale one. Add a regression test to alter-sink.td that grants on the sink's connection after ALTER SINK ... SET FROM. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A Connection Altered implication re-alters every dependent sink, also for catalog changes that don't affect the connection's runtime config, like GRANT and REVOKE. Restarting the sink dataflow in that case is wasted work and briefly interrupts the sink. Skip updates where the connection is unchanged, mirroring alter_ingestion_connections. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes SQL-517
Motivation
GRANT,REVOKE,ALTER ... OWNER, orALTER CONNECTIONon a connectionpanicked the coordinator with
whenever a dependent sink had previously been changed with
ALTER SINK ... SET FROM. Seen in the Nightly Parallel Workload jobs"rename + naughty identifiers" and "DDL Only".
Root cause:
StorageController::alter_export(theALTER SINKpath) sends thenew sink definition to the cluster and updates the
ExportState, but neverwrote the new
ExportDescriptioninto the collection'sDataSource::Sink. Thecontroller's stored export description therefore kept the sink definition from
sink creation forever. This update was lost when the description moved from
ExportStateintoCollectionStatein 273b3e4 ("Make sinks a variant ofcollections").
Any catalog change to a connection item (including privilege and owner changes)
emits a
Connection(Altered)implication, which recomputes the connection ofevery dependent sink from the catalog and calls
alter_export_connections.That method diffs the new connection against the stale stored description with
alter_compatible, which requires fields likevalue_descandrelation_key_indicesto match. After anALTER SINK ... SET FROMhad changedthose, the check failed with
InvalidAlter, and the coordinator treats that asa violated invariant and panics.
Description
Two commits:
Keep the stored export description current on
ALTER SINK(the bug fix):alter_exportnow stores the newExportDescriptionin the collection stateafter sending the
RunSinkcommand, mirroringalter_export_connections.With the controller's description tracking the catalog, later connection
alters diff against the sink that is actually running. This also makes
run_exportrerun the current sink definition instead of the stale one.Skip no-op sink connection alters (separable optimization): catalog
changes that don't affect a connection's runtime config, like
GRANTandREVOKE, also arrive atalter_export_connections. Previously theyrestarted every dependent sink dataflow for no reason. Skip updates where
the connection is unchanged, mirroring
alter_ingestion_connections. Thisdoes not mask the bug above: with a stale description the recomputed
connection differs, so the compatibility check still runs.
User-facing changes:
GRANT/REVOKE/ALTERon a connection no longer panics the coordinatorwhen a dependent sink had been altered with
ALTER SINK ... SET FROM.restart dependent sink dataflows.
Verification
test/testdrive/alter-sink.td:GRANTon the sink'sconnection after
ALTER SINK ... SET FROM, then verify data still flows.Before the fix, a minimal repro (CREATE SINK, ALTER SINK SET FROM, GRANT ON
CONNECTION) deterministically reproduced the exact panic and backtrace from
the Nightly failures. After the fix it passes.
alter-sink.td,connection-alter.td, andkafka-sink-statistics.td(whichbreaks and restores a sink's connection via real
ALTER CONNECTION,exercising the non-skipped path) all pass with both commits.
bin/cargo-test -p mz-storage-controllerpasses.🤖 Generated with Claude Code