adapter: derive implications from catalog changes (rebased) #33999
Conversation
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
NOTE: This is the first of two commits. In here, we add the basic scaffolding for deriving implications from catalog changes, the parsing logic, all the plumbing. In the next commit we actually move code over from closures in the sequencer and the drop logic from ddl.rs. First step towards MaterializeInc/database-issues#8488 This change introduces a new architectural pattern for handling implications for in-memory state, controller changes, or anything else that might have to be done as a result of updating the catalog. Instead of directly updating state or instructing controllers within the sequencer logic (via closures) or in the drop logic of ddl.rs, we now derive these implications from catalog state updates and apply them in a separate phase. Key architectural changes: - Introduces `ParsedStateUpdate` that represent catalog changes that need to be applied - Adds a new `catalog_update_implications` module containing logic to derive and apply implications from consolidated catalog updates - Implements `apply_catalog_update_implications` that processes catalog updates and derives implications for items, clusters, and replicas, ongoing selects, subscribes, etc. - Adds consolidation logic to handle "fluctuating" updates (e.g., from DROP OWNED BY) where intermediate states cancel out, ensuring only net changes are applied - Moves update logic from inline closures in sequencer operations (particularly in `create_source_inner` and related methods) to the new centralized implication processing The refactoring specifically handles: - Table creation/deletion - Source creation/deletion - Storage policy initialization for newly created collections - Proper handling of temporary items alongside durable ones This approach provides better separation of concerns between catalog mutations and applying implications, making the system more maintainable and setting the foundation for eventually handling all implications through this pattern. Future work will extend this pattern to handle all catalog item types including indexes, materialized views, sinks, and other database objects.
NOTE: This is the second commit that moves over code from closures in sequencer and the drop logic from ddl.rs. First step towards MaterializeInc/database-issues#8488 This change introduces a new architectural pattern for handling implications for in-memory state, controller changes, or anything else that might have to be done as a result of updating the catalog. Instead of directly updating state or instructing controllers within the sequencer logic (via closures) or in the drop logic of ddl.rs, we now derive these implications from catalog state updates and apply them in a separate phase. Key architectural changes: - Introduces `ParsedStateUpdate` that represent catalog changes that need to be applied - Adds a new `catalog_update_implications` module containing logic to derive and apply implications from consolidated catalog updates - Implements `apply_catalog_update_implications` that processes catalog updates and derives implications for items, clusters, and replicas, ongoing selects, subscribes, etc. - Adds consolidation logic to handle "fluctuating" updates (e.g., from DROP OWNED BY) where intermediate states cancel out, ensuring only net changes are applied - Moves update logic from inline closures in sequencer operations (particularly in `create_source_inner` and related methods) to the new centralized implication processing The refactoring specifically handles: - Table creation/deletion - Source creation/deletion - Storage policy initialization for newly created collections - Proper handling of temporary items alongside durable ones This approach provides better separation of concerns between catalog mutations and applying implications, making the system more maintainable and setting the foundation for eventually handling all implications through this pattern. Future work will extend this pattern to handle all catalog item types including indexes, materialized views, sinks, and other database objects.
Picking the op_id, as before, seems bogus, because this is just an increasing number within the transaction, and so the temporary updates wouldn't be sorted in one batch with other updates when chunking up durable and temporary catalog updates.
When applying updates that are derived from catalog changes to the controllers we want the updates to be consolidated. Before we were doing that right on the parsed controller updates, but we had to implement bespoke Ord/Eq that would ignore the in-memory fields. Now, we sort earlier in the transact/apply "pipeline", right when we have the StateUpdates. This allows us to remove the bespoke Ord/Eq impls, but comes with other downsides as explained in code comments.
which ... temporary sources are not a thing, but the types support it and this way the code is the same as for non-temp items
This reverts commit 17f22ef.
…s not only about controllers
Contributor
Author
|
Looking into the Nightly failures:
We are still at the same spot after 48m. Edit: We timed out at this spot. Edit 2: I don't understand how can we be at the exact same
Can this be related to this PR? Maybe we are trying to connect to the replicas at wrong addresses due to something broken with how the new code drives the compute controller? Edit: Or, does Edit: Using the "Show timestamps" button it's very clearly visible that all the runs hang at this same point for 30+ minutes. It might be a test issue that the test hangs there, instead of some specific operation (e.g., a specific Testdrive query) timing out and failing the test. But the above replica connection issue is probably the real problem, independently from the test issue. Slack thread. Other failures:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This is just @aljoscha's #29673 rebased. Gonna run a new Nightly.
There was a somewhat scary conflict resolution in
sequence_create_source: #29673 deleted the closure of thecatalog_transact_with_ddl_transactioncall, and moved its functionality intohandle_create_source, but in the meantime the closure was changed onmainby 6dd815f (around thecoord.catalog().get_entry(&item_id).latest_global_id();line of this commit). I applied this same change tohandle_create_source: copied the line from 6dd815f that callslatest_global_id(), made it use theitem_idargument given tohandle_create_source(this argument was previously unused), and made theIngestionDescription::newuse the newitem_global_idinstead ofsource.global_id. I hope this is correct.Nightly runs:
K8s recovery: envd on failing nodewith theserve_connectiondebug prints)serve_connection)Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.