Skip to content

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented Nov 17, 2023

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:

struct DependencyDependent {
    CatalogEntryInfo entry;
    DependencyDependentFlags flags;
};

The entry targetted by the dependency:

struct DependencySubject {
    CatalogEntryInfo entry;
    DependencySubjectFlags flags;
};

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 name
  • EntryMangledName, this corresponds to the right side of the mangled name
  • SourceInfo, this corresponds to the left side of the mangled name
  • SourceMangledName, this corresponds to the left side of the mangled name
CREATE SEQUENCE seq;
CREATE TABLE tbl (i integer default nextval('seq'));

tbl relies on the existence of seq to continue existing.
If we attempt to drop seq while tbl is still alive, it will result in an error, unless CASCADE is provided, in that case tbl is silently dropped along with seq.

This creates two DependencyCatalogEntry's:
Dependent:
seq -> tbl

'tbl' is a Dependent of 'seq'

Dependency:
tbl -> seq

'seq' is a Dependency of 'tbl'

DependencyCatalogEntry names follow this format:
MangledEntryName\0MangledEntryName
MangledEntryName follows this format:
type\0schema\0name

Example for a dependency from Table main.tbl on Sequence not_main.seq:

Table\0main\0tbl\0Sequence\0not_main\0seq

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.


Tishj added 30 commits October 24, 2023 18:35
…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
… 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
@github-actions github-actions bot marked this pull request as draft November 24, 2023 20:54
@Tishj Tishj marked this pull request as ready for review November 25, 2023 08:15
Copy link
Collaborator

@Mytherin Mytherin left a 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

@github-actions github-actions bot marked this pull request as draft November 28, 2023 09:30
@Tishj Tishj marked this pull request as ready for review November 30, 2023 10:14
@github-actions github-actions bot marked this pull request as draft December 6, 2023 12:08
@Tishj Tishj marked this pull request as ready for review December 8, 2023 13:24
@Mytherin Mytherin merged commit 7d5150c into duckdb:main Dec 11, 2023
@Mytherin
Copy link
Collaborator

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants