Resolve table dependencies on metadata loading by tavplubix · Pull Request #28373 · ClickHouse/ClickHouse

tavplubix · 2021-08-30T19:35:53Z

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Resolve table dependencies on metadata loading. Closes #8004, closes #15170.

Detailed description / Documentation draft:

tavplubix · 2021-09-03T18:36:27Z

Test failures are irrelevant

alesapin · 2021-09-08T10:54:39Z

@Mergifyio update

mergify · 2021-09-08T10:55:12Z

Command update: success

Branch has been successfully updated

tavplubix · 2021-09-08T12:54:51Z

Functional stateless tests flaky check (address) - Dictionary (`test_9dzpz8.dict1`) not found ... - looks like race in ExternalLoader, I will try to debug it

tavplubix · 2021-09-10T10:02:32Z

Functional stateless tests flaky check (address) - Dictionary (`test_9dzpz8.dict1`) not found ... - looks like race in ExternalLoader, I will try to debug it

#28853

alesapin

In general looks good to me, simple and straightforward implementation.

alesapin · 2021-09-13T08:53:50Z

src/Databases/DDLDependencyVisitor.cpp

+
+void DDLDependencyVisitor::visit(const ASTFunction & function, Data & data)
+{
+    if (function.name == "joinGet" ||


Maybe it worth to make a property for such functions?

We will have to create a function through FunctionFactory just to check if it can take table/dictionary name as argument or not, so I'm not sure.

alesapin · 2021-09-13T08:55:57Z

src/Databases/DDLDependencyVisitor.cpp

+        if (literal->value.getType() != Field::Types::String)
+            return;
+
+        String maybe_qualified_name = literal->value.get<String>();


We still don't have a function for parsing qualified names?

Seems like we don't, because for historical reasons parsing of qualified name from string literal is a bit different from parsing of compound identifier from SQL query.

alesapin · 2021-09-13T08:56:42Z

src/Databases/DDLDependencyVisitor.cpp

+    else if (const auto * identifier = arg->as<ASTIdentifier>())
+    {
+        auto table_identifier = identifier->createTable();
+        if (!table_identifier)


Maybe add a comment? Not clear to me how we can get here.

We can get here if create query is incorrect and compound identifier has 3 (or more) name parts.

alesapin · 2021-09-13T08:57:55Z

src/Databases/DDLDependencyVisitor.cpp

+    if (!dict_source.elements)
+        return;
+
+    auto config = getDictionaryConfigurationFromAST(data.create_query->as<ASTCreateQuery &>(), data.global_context);


Also very soon we will have predefined connections... I'd prefer to move this part into separate function like isLocalDictionaryFromClickHouseTable or something like this.

alesapin · 2021-09-13T09:11:36Z

src/Databases/DDLDependencyVisitor.h

+class ASTFunctionWithKeyValueArguments;
+
+
+class DDLDependencyVisitor


Need comment.

alesapin · 2021-09-13T10:18:17Z

src/Databases/TablesLoader.h

+    String default_database;
+
+    std::mutex mutex;
+    ParsedMetadata metadata;


Metadata inside metadata :) Maybe just ParsedTables?

alesapin · 2021-09-13T10:19:42Z

src/Interpreters/loadMetadata.h

 /// Use separate function to load system tables.
 void loadMetadata(ContextMutablePtr context, const String & default_database_name = {});

+void startupSystemTables();


Maybe add a comment why it is required?

alesapin · 2021-09-13T10:20:27Z

src/Databases/DatabaseOrdinary.cpp

-    iterateMetadataFiles(local_context, process_metadata);
-
-    size_t total_tables = file_names.size() - total_dictionaries;
+    ParsedTablesMetadata metadata;


metadata -> parsed_tables?

alesapin · 2021-09-13T10:24:57Z

src/Databases/TablesLoader.h

+
+    void startLoadingIndependentTables(ThreadPool & pool, size_t level);
+
+    void checkCyclicDependencies() const;


Should be called only after we have loaded everything what was possible to load.

alesapin · 2021-09-13T10:27:55Z

src/Databases/DatabaseOrdinary.cpp

 }

-void DatabaseOrdinary::startupTablesImpl(ThreadPool & thread_pool)
+void DatabaseOrdinary::startupTables(ThreadPool & thread_pool, bool /*force_restore*/, bool /*force_attach*/)


Slightly unclear interface -- why we create pool out of this function scope, but wait it here?

alesapin · 2021-09-17T10:07:55Z

Integration tests failures related?

tavplubix · 2021-09-17T10:18:31Z

Integration tests failures related?

Seems like they are

tavplubix · 2021-09-17T19:33:27Z

Functional stateless tests (release, DatabaseReplicated) - merge stuck with Directory tmp_merge_9_0_64_2/ already exists

2021-09-14 11:20:15 sync failed, queue:	default	alter_table_1	r_1auto_r1	0	queue-0000003927	MERGE_PARTS	2021-09-14 11:13:34	0	r_8auto_r2	9_0_64_2	['9_0_1_1','9_2_2_0','9_3_3_0','9_5_5_0','9_6_6_0','9_7_7_0','9_9_16_1','9_17_27_1','9_28_28_0','9_29_29_0','9_30_30_0','9_33_45_1','9_47_47_0','9_49_49_0','9_52_52_0','9_54_54_0','9_56_64_1']	0	0	1622	Code: 84. DB::Exception: Directory /var/lib/clickhouse/store/397/3976fa6a-afd1-4c7b-b976-fa6aafd11c7b/tmp_merge_9_0_64_2/ already exists. (DIRECTORY_ALREADY_EXISTS) (version 21.11.1.8075)	2021-09-14 11:20:14	26	Not executing log entry queue-0000003927 of type MERGE_PARTS for part 9_0_64_2 because source parts size (8.59 MiB) is greater than the current maximum (4.45 MiB). 1 free of 16 threads, required 8 free threads.	2021-09-14 11:13:46	REGULAR

filimonov · 2021-09-18T04:33:05Z

What do you think of #7944 - needed for schema dumps & recreate on the other node

filimonov · 2021-09-18T04:48:37Z

src/Databases/TablesLoader.h

+    /// How many dependencies this table have
+    size_t dependencies_count = 0;
+    /// List of tables/dictionaries which depend on this table/dictionary
+    TableNames dependent_database_objects;


Dependency_kind (enum) may also be useful sometimes (could be exposed in system.tables for introspection / schema analise) but can be done later

alesapin

LGTM

tavplubix · 2021-09-20T11:45:53Z

Functional stateless tests (release, DatabaseReplicated) - 01161_all_system_tables - fixed in #29061

Follow up to #28373

alexey-milovidov · 2021-09-26T17:29:01Z

@tavplubix It did not help: https://clickhouse-test-reports.s3.yandex.net/29380/0e9af142dfb6c9418a277aa78a77ffb9a4e58297/stress_test_(thread)/test_run.txt.out.log

tavplubix · 2021-09-26T18:00:34Z

It's because table test_9.join does not exist.

Improvement for #28373

vitlibar · 2022-12-19T14:08:33Z

src/Databases/DDLDependencyVisitor.cpp

+    else if (Poco::toLower(function.name) == "in")
+    {
+        extractTableNameFromArgument(function, data, 1);
+    }


@tavplubix
Do you remember what is the reason that loading of a table depends on the default expressions for its columns? The default expression should be required only during insertion to that table, shouldn't they?

I'm not sure, there was a check that the default expression type is compatible with the column type, but probably there's another reason.

resolve table dependencies on metadata loading

b387f05

robot-clickhouse added the pr-improvement Pull request with some product improvements label Aug 30, 2021

fix

c8d8f0a

tavplubix force-pushed the tables_topsort branch from 55fdb23 to c8d8f0a Compare August 31, 2021 12:07

tavplubix added the force tests label Aug 31, 2021

robot-ch-test-poll1 added the submodule changed At least one submodule changed in this PR. label Sep 1, 2021

tavplubix added 2 commits September 2, 2021 12:19

better code, moar logging

024a24a

add another test

666a3ae

tavplubix force-pushed the tables_topsort branch from fed00a8 to 666a3ae Compare September 2, 2021 13:35

tavplubix removed the submodule changed At least one submodule changed in this PR. label Sep 2, 2021

tavplubix marked this pull request as ready for review September 2, 2021 13:36

support function IN

1e2844f

alesapin self-assigned this Sep 8, 2021

Merge branch 'master' into tables_topsort

9f0c503

Merge branch 'master' into tables_topsort

3f8fb1e

alesapin reviewed Sep 13, 2021

View reviewed changes

tavplubix added 3 commits September 13, 2021 16:58

Merge branch 'master' into tables_topsort

8ac19ca

fixes after review

df56e99

fix

2ec9b6f

tavplubix removed the force tests label Sep 13, 2021

tavplubix added 2 commits September 14, 2021 00:39

fix

f5c38fe

fix test

f03484e

Update PostgreSQLDictionarySource.cpp

749b913

filimonov reviewed Sep 18, 2021

View reviewed changes

alesapin approved these changes Sep 20, 2021

View reviewed changes

tavplubix merged commit 922cf7e into master Sep 20, 2021

tavplubix deleted the tables_topsort branch September 20, 2021 11:46

tavplubix mentioned this pull request Sep 20, 2021

Follow up to #28373 #29201

Merged

tavplubix added a commit that referenced this pull request Sep 21, 2021

Merge pull request #29201 from ClickHouse/follow_up_to_28373

5f4c665

Follow up to #28373

This was referenced Oct 3, 2021

Follow-up for #26231 #29626

Merged

Improvement for #28373 #29804

Merged

tavplubix added a commit that referenced this pull request Oct 8, 2021

Merge pull request #29804 from ClickHouse/improvement_for_28373

f0e3122

Improvement for #28373

tavplubix mentioned this pull request Nov 1, 2021

Check dependencies on DROP TABLE #30977

Merged

filimonov mentioned this pull request Dec 6, 2021

XML dictionaries + MatView + 21.8.11 = DB::Exception: Dictionary not found #31963

Closed

vitlibar reviewed Dec 19, 2022

View reviewed changes

		class ASTFunctionWithKeyValueArguments;


		class DDLDependencyVisitor


		void startLoadingIndependentTables(ThreadPool & pool, size_t level);

		void checkCyclicDependencies() const;

Conversation

tavplubix commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tavplubix commented Sep 3, 2021

Uh oh!

alesapin commented Sep 8, 2021

Uh oh!

mergify bot commented Sep 8, 2021

Uh oh!

tavplubix commented Sep 8, 2021

Uh oh!

tavplubix commented Sep 10, 2021

Uh oh!

alesapin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alesapin Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alesapin commented Sep 17, 2021

Uh oh!

tavplubix commented Sep 17, 2021

Uh oh!

tavplubix commented Sep 17, 2021

Uh oh!

filimonov commented Sep 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alesapin left a comment

Choose a reason for hiding this comment

Uh oh!

tavplubix commented Sep 20, 2021

Uh oh!

alexey-milovidov commented Sep 26, 2021

Uh oh!

tavplubix commented Sep 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tavplubix commented Aug 30, 2021 •

edited

Loading

alesapin Sep 13, 2021 •

edited

Loading