backup: fix table-level restore of tables with trigger function references#162586
Draft
rafiss wants to merge 5 commits intocockroachdb:masterfrom
Draft
backup: fix table-level restore of tables with trigger function references#162586rafiss wants to merge 5 commits intocockroachdb:masterfrom
rafiss wants to merge 5 commits intocockroachdb:masterfrom
Conversation
Previously, when a trigger function referenced another table, the backreference in DependedOnBy was structurally identical to view dependencies. This made it impossible to distinguish trigger dependencies from view dependencies when inspecting crdb_internal.forward_dependencies. This commit adds a trigger_id field to TableDescriptor_Reference in the protobuf schema. When creating backreferences for trigger dependencies, we now set the trigger_id to identify the originating trigger. The crdb_internal.forward_dependencies virtual table has been updated to report dependedonby_type='trigger' when the trigger_id is set, instead of 'view'. Resolves: cockroachdb#147801 Epic: CRDB-42942 Release note: None
Previously, trigger dependencies in DependedOnBy had TriggerID=0, making them indistinguishable from view dependencies. This caused issues when dropping a trigger that shared a referenced table with another trigger, as the drop would incorrectly remove the shared backref. This commit adds a migration that repairs existing trigger backrefs by: 1. Iterating through all tables with triggers in each database 2. For each trigger's DependsOn references, finding the corresponding backref with TriggerID=0 and setting the correct TriggerID 3. If multiple triggers reference the same table (which previously shared a single backref), creating separate backrefs for each trigger The migration processes descriptors in batches of 100 to avoid overly large transactions. Epic: CRDB-42942 Release note: none
…ation Previously, when dropping a trigger, if a policy on the same table referenced the same relation (e.g., a sequence via nextval()), the trigger's backref was incorrectly preserved. This occurred because UpdateTableBackReferencesInRelations checked whether ANY forward reference existed to that relation, including from policies. This commit fixes the issue by: 1. Renaming UpdateTableBackReferencesInRelations to UpdateTriggerBackReferencesInRelations to clarify it only handles trigger backrefs. 2. Simplifying the function to only check trigger forward refs, since policies and views manage their own generic backrefs separately. 3. Updating updateBackReferencesInSequences to only consider generic backrefs (TriggerID=0) when determining if a backref already exists. This ensures triggers create their own backrefs that are properly removed when the trigger is dropped. 4. Updating UpdateColumnsDependedOnBy to only match generic backrefs (TriggerID=0), ensuring trigger-specific backrefs are not incorrectly modified. Release note: None
…ences Previously, `remapFunctions` in restore planning assumed that function descriptors could never appear in table-level restores, since UDFs were not directly referenced by tables. This assumption became invalid when triggers were introduced, as triggers on a table can reference UDFs. Attempting a table-level restore (e.g., `RESTORE TABLE db.schema.*`) that included a trigger function would hit an assertion failure: "function descriptor seen when restoring tables". Additionally, the backref cleanup logic in `rewrite.TableDescs` was too aggressive when dropping triggers with missing dependencies. If two triggers on the same table both referenced the same relation, dropping one trigger would remove all backrefs from the referenced relation to the table, even though the other trigger still maintained a valid forward reference. This caused descriptor validation failures. This patch fixes both issues: 1. Updates `remapFunctions` to handle table-level restores by looking up the target database and creating proper rewrite entries, following the same pattern as `remapTables` and `remapTypes`. 2. Makes the backref cleanup in the second pass of `rewrite.TableDescs` trigger-ID-aware: when a backref has a non-zero TriggerID, the code now checks whether that specific trigger still exists on the referencing table before removing the backref. Epic: CRDB-42942 Release note: None
Previously, table-level restores of tables with triggers would fail when the target schema already contained the trigger's function. The restore had a TODO for implementing function collision detection (similar to tables and types), but it was not yet implemented. This change adds collision detection for functions during restore planning. Unlike tables and types, functions don't have namespace entries, so collision detection works by looking up the function by name in the schema descriptor's Functions map, then comparing input parameter types to find a matching overload. Type comparison accounts for descriptor rewrites on user-defined types. When a collision is detected, the function is marked as ToExisting and filtered out of the write set. Additionally, existing functions that are referenced by restored tables' triggers are updated with back-references (DependedOnBy) to maintain correct dependency tracking. Release note: none
Contributor
|
Merging to
|
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Member
|
This change is |
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
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.
stacked on top of #161910
backup: fix table-level restore of tables with trigger function references
Previously,
remapFunctionsin restore planning assumed that functiondescriptors could never appear in table-level restores, since UDFs were
not directly referenced by tables. This assumption became invalid when
triggers were introduced, as triggers on a table can reference UDFs.
Attempting a table-level restore (e.g.,
RESTORE TABLE db.schema.*)that included a trigger function would hit an assertion failure:
"function descriptor seen when restoring tables".
Additionally, the backref cleanup logic in
rewrite.TableDescswas tooaggressive when dropping triggers with missing dependencies. If two
triggers on the same table both referenced the same relation, dropping
one trigger would remove all backrefs from the referenced relation to
the table, even though the other trigger still maintained a valid
forward reference. This caused descriptor validation failures.
This patch fixes both issues:
Updates
remapFunctionsto handle table-level restores by looking upthe target database and creating proper rewrite entries, following the
same pattern as
remapTablesandremapTypes.Makes the backref cleanup in the second pass of
rewrite.TableDescstrigger-ID-aware: when a backref has a non-zero TriggerID, the code
now checks whether that specific trigger still exists on the
referencing table before removing the backref.
backup: add function collision detection for table-level restore
Previously, table-level restores of tables with triggers would fail when
the target schema already contained the trigger's function. The restore
had a TODO for implementing function collision detection (similar to
tables and types), but it was not yet implemented.
This change adds collision detection for functions during restore
planning. Unlike tables and types, functions don't have namespace
entries, so collision detection works by looking up the function by name
in the schema descriptor's Functions map, then comparing input parameter
types to find a matching overload. Type comparison accounts for
descriptor rewrites on user-defined types.
When a collision is detected, the function is marked as ToExisting and
filtered out of the write set. Additionally, existing functions that are
referenced by restored tables' triggers are updated with back-references
(DependedOnBy) to maintain correct dependency tracking.
Epic: CRDB-42942
Release note: None