-
Notifications
You must be signed in to change notification settings - Fork 2.6k
[DependencyManager] Rework internals of the DependencyManager #9715
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
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
… together with a designated type alias
…n't be run yet because DropObject is recursive, we cant perform both the Old and New version..
… | also skip registering dependencies for database entries, they cant have dependencies and they can't have dependents, seems like wasted effort even without this change
… comes to getting types and names
…new_obj name differs
… this is required in the DependencyManager when looking up the schema in the LookupEntry, if we're dropping the schema it will be already marked as deleted - but we still need to find the entries of the schema
…child relationship
Mytherin
reviewed
Nov 27, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fixes - some more comments below
…two types of flags (blocking|owned_by) and (ownership)
Thanks for all the changes! Looks great. |
krlmlr
added a commit
to duckdb/duckdb-r
that referenced
this pull request
Dec 14, 2023
Merge pull request duckdb/duckdb#9985 from hawkfish/summarize-overflow Merge pull request duckdb/duckdb#9972 from yiyuanliu/lyy/fix-9949 Merge pull request duckdb/duckdb#9962 from Tishj/statement_error_expected_non_optional Merge pull request duckdb/duckdb#9961 from Tishj/python_supply_config_to_connect Merge pull request duckdb/duckdb#9947 from taniabogatsch/list-intersect Merge pull request duckdb/duckdb#9568 from Jens-H/append-BigDecimal Merge pull request duckdb/duckdb#9959 from Tishj/python_udf_arrow_side_effects Merge pull request duckdb/duckdb#9855 from StarveZhou/issue_9795_arg Merge pull request duckdb/duckdb#9944 from Tishj/pyproject_toml Merge pull request duckdb/duckdb#9946 from lnkuiper/issue9718 Merge pull request duckdb/duckdb#9953 from mlafeldt/fix-httpfs-null-ptr Merge pull request duckdb/duckdb#9897 from Tmonster/left_semi_anti_feature_rebased Merge pull request duckdb/duckdb#9936 from hawkfish/empty-frames Merge pull request duckdb/duckdb#9715 from Tishj/dependency_set
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.
Catalog Entry
Made the internals of CatalogEntry private and added getters/setters for the Child/Parent relation, fixing a bug in the process.
CatalogSet
CatalogSet used to contain an extra level of indirection between the mapping of name to CatalogEntry.
This was necessary because of the logic surrounding renames.
This PR cleans up that logic, a rename now starts a new chain of versions under the new name and is fully transactional.
A rename will now result in a
RENAMED_ENTRY
being added to the end of the old name’s chain.The new chain will add a
RENAMED_ENTRY
before adding the new entry to the new name’s chain.Because of this we can now simplify this connection to just the mapping of (case insensitive)name to CatalogEntry (version chain).
As a result of this:
The MappingValue class has been removed
The EntryValue class has been removed
The EntryIndex class has been removed
The EntryDropper class has been removed
CatalogEntryMap
This class manages the entry chains, it provides methods to update, add, remove and get entries.
The CatalogSet contains a catalog entry map.
DependencyManager
The DependencyManager was not transaction-aware, this resulted in having to grab a lock in the CatalogSet before making changes to the dependency manager.
Then inside the DependencyManager we needed to call private methods of the CatalogSet that assumed a lock was already held.
We now store the dependency connections in CatalogSets, making them transaction aware.
DependencyEntry
Added a DependencyEntry abstract class.
The entry initiating the dependency:
The entry targetted by the dependency:
The internals of these are identical, except for:
The
name
of the CatalogEntry, the sides are switched in the DependencyReliant/DependencySubject for lookup purposes.Overloaded virtual methods:
EntryInfo
, this corresponds to the right side of the mangled nameEntryMangledName
, this corresponds to the right side of the mangled nameSourceInfo
, this corresponds to the left side of the mangled nameSourceMangledName
, this corresponds to the left side of the mangled nametbl
relies on the existence ofseq
to continue existing.If we attempt to drop
seq
whiletbl
is still alive, it will result in an error, unless CASCADE is provided, in that casetbl
is silently dropped along withseq
.This creates two DependencyCatalogEntry's:
Dependent:
seq
->tbl
Dependency:
tbl
->seq
DependencyCatalogEntry names follow this format:
MangledEntryName\0MangledEntryName
MangledEntryName follows this format:
type\0schema\0name
Example for a dependency from Table
main.tbl
on Sequencenot_main.seq
:To abstract this prefixing logic away we use a
DependencyCatalogSet
, this acts like a wrapper around a CatalogSet, adding the prefix to every interaction (get/remove/create)Earlier on in development these were actual CatalogSets, but that led to discovering that our Catalog design is not built for nested CatalogSets.