Hello @serxa , could you have a look please since you worked on async table loading?

The change really cures the issue, although I am still working on it because I am not sure why

1. TablesLoader and DataBaseCatalog coexist and keep same or similar dependencies
2. we need checkDependencies() for Kafka (StorageKafkaUtil.cpp) and other streaming engines, instead of having proper load dependencies. In other words, why "loading dependencies are a subset of referential dependencies". What prevents us from treating referential dependencies as loading dependencies?

This is an automated comment for commit cd1cc0c with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Test failures:

1. test_s3_zero_copy_replication/test.py::test_move_shared_lock_fail_keeper_unavailable (not unique, but rare https://play.clickhouse.com/play?user=play#c2VsZWN0IGNoZWNrX25hbWUsIHRlc3RfbmFtZSwgcHVsbF9yZXF1ZXN0X251bWJlciwgdGVzdF9zdGF0dXMKZnJvbSBjaGVja3MgCndoZXJlICcyMDI0LTEwLTEwJyA8PSBjaGVja19zdGFydF90aW1lICBhbmQgdGVzdF9uYW1lIGlsaWtlICclOnRlc3RfbW92ZV9zaGFyZWRfbG9ja19mYWlsX2tlZXBlcl91bmF2YWlsYWJsZSUnIGFuZCBjaGVja19uYW1lIGlsaWtlICclaW50ZWdyJScgYW5kIHRlc3Rfc3RhdHVzICE9ICdPSycKb3JkZXIgYnkgdGVzdF9kdXJhdGlvbl9tcyBkZXNjCmxpbWl0IDEwMDsKCg==
2. 01086_window_view_cleanup is definitely flaky https://play.clickhouse.com/play?user=play#c2VsZWN0IGNoZWNrX25hbWUsIHRlc3RfbmFtZSwgcHVsbF9yZXF1ZXN0X251bWJlciwgdGVzdF9zdGF0dXMKZnJvbSBjaGVja3MgCndoZXJlICcyMDI0LTEwLTEwJyA8PSBjaGVja19zdGFydF90aW1lICBhbmQgdGVzdF9uYW1lIGlsaWtlICclMDEwODZfd2luZG93X3ZpZXdfY2xlYW51cCUnIGFuZCB0ZXN0X3N0YXR1cyAhPSAnT0snCm9yZGVyIGJ5IHRlc3RfZHVyYXRpb25fbXMgZGVzYwpsaW1pdCAxMDA7Cgo=

Hello @serxa , could you have a look, please.

1. TablesLoader and DataBaseCatalog coexist and keep same or similar dependencies

They serve different purposes. DataBaseCatalog is a thing that stores everything consistently, a source of truth if you will. TablesLoader is just a simple helper class that exists to extract common code from two different places: (a) server startup and (b) CREATE/ATTACH TABLE queries. This helper arranges "loading" in a way that is legitimate for the catalog to process without issues. This is the main reason for them to share similar things like dependency graphs. The catalog is just not smart enough to plan the chain (the order) of actions needed to load a database. Previously there was a synchronous loading-by-levels algorithm which was replaced by AsyncLoader. And now, probably, TablesLoader might be a part of DataBaseCatalog, because it is now much easier to track all the dependencies. But still, it would be a large refactoring to do with no clear objectives.

2. we need checkDependencies() for Kafka (StorageKafkaUtil.cpp) and other streaming engines, instead of having proper load dependencies. In other words, why "loading dependencies are a subset of referential dependencies". What prevents us from treating referential dependencies as loading dependencies?

Any unnecessary load dependency may potentially slow down server startup because more tables will be loaded sequentially instead of in parallel. So we tried our best to separate different kinds of dependencies. IIRC referential dependencies exist only to avoid dropping a table that is mentioned in the definition of another table, but such tables could be loaded in parallel. cc @tavplubix

@serxa, thanks for reacting and for clarification.

I have to admit that I am not sure that current solution is optimal/most elegant.
Basically what we do is carefully detect dependencies, then ignore them starting up streaming tables too early (before their dependencies are ready) only to make all streaming tables to call checkDependencies() from their setup methods to wait proper moment to start.

I see following options

1. limit the scope by fixing the bug, make this PR suitable for merge (cleanup + rename pushDependencies to preSetup() or postLoad()) as it currently is.
2. move addViewDependency call out of MaterializedView, somewhere to TablesLoader, probably to DDLDependencyVisitor (not sure if it is possible, but should be)
3. include view dependencies to load dependencies for tables with a streaming engine.

(3) eliminates need of checkDependencies, while these methods are needed for (1) and (2), but since the code is the same, it can be a single method of DataBaseCatalog.

Suggestions are appreciated.

BTW, if I am not mistaken, check for ErrorCodes::TOO_MANY_MATERIALIZED_VIEWS
is not reliable.

@ilejn

Basically what we do is carefully detect dependencies, then ignore them starting up streaming tables too early (before their dependencies are ready) only to make all streaming tables to call checkDependencies() from their setup methods to wait proper moment to start.

You are completely right, this is nonsense. I think option (3) is almost the right way to do it. We should get rid of checkDependencies(). And add correct dependencies in AsyncLoader for views. Now we have the following order of execution in case of load dependency (note that dependencies point backward to arrows in the diagram):

flowchart TD
    load_table1["load table1"] --> startup_table1["startup table1"]
    load_table2["load table2"] --> startup_table2["startup table2"]
    load_table1 --> load_table2

Which prevents execution parallel loading. But for views, we could instead use dependencies between startup jobs to make the following order of execution:

flowchart TD
    load_table1["load table1"] --> startup_table1["startup table1"]
    load_table2["load table2"] --> startup_table2["startup table2"]
    startup_table1 --> startup_table2

This won't prevent parallel execution of load jobs. And startup jobs are not long and it would not be a problem. Whenever a table is accessed by INSERT query it waits (blocks) until the startup job is done. So this logic remains the same and will automagically resolve the issue instead of the checkDependencies() hack.

To implement this we should pass view_dependencies into TablesLoader and use it for job construction in TablesLoader::startupTablesAsync() method. It could be done in a way similar to TablesLoader::loadTablesAsync(). Iteration should be done in view_dependencies.getTablesSortedByDependency() order as well.

Seems not too hard at the first glance. Would you like to give it a try in this PR?

BTW, if I am not mistaken, check for ErrorCodes::TOO_MANY_MATERIALIZED_VIEWS
is not reliable.

What do you mean?

BTW, if I am not mistaken, check for ErrorCodes::TOO_MANY_MATERIALIZED_VIEWS
is not reliable.

What do you mean?

We do this check in StorageMaterializedView ctor based on data collected in StorageMaterializedView::startup .
Switch to creating view dependencies in TablesLoader::loadTablesAsync solves this issues among others.

To implement this we should pass view_dependencies into TablesLoader

Yes.
Retrieving view dependencies by DDLDependencyVisitorData is not perfect, but ok for me.

and use it for job construction in TablesLoader::startupTablesAsync() method. It could be done in a way similar to TablesLoader::loadTablesAsync(). Iteration should be done in view_dependencies.getTablesSortedByDependency() order as well.

Should we limit the scope of this extra loop by streaming alike tables only?

Seems not too hard at the first glance.

Will see ;)

Would you like to give it a try in this PR?

Yes, sure.

BTW, I have an impression that in case of streaming tables and their MVs we don't have an implicit synchronization point, "Whenever a table is accessed by INSERT query it waits (blocks) until the startup job is done" does not work for them, that's why checkDependencies() are needed. Does it make sense?

I have an impression that in case of streaming tables and their MVs we don't have an implicit synchronization point

I'm not very deep in that streaming code, but yes, looks like that is true.

Should we limit the scope of this extra loop by streaming alike tables only?

No, I believe it is needed for all tables. So it will be not an extra loop, but improvement of existing one that is already present there (current loop just iterates tables in some unspecified order). In #72589 streaming is not mentioned, but async_load_tables was identified as the root cause. It is just because without async loading there was no way to do INSERT in the middle (when the source table is started, but target is not started yet). With async load of tables it is now possible.

So we will fix both issues by introducing these new dependencies directly into AsyncLoader DAG.

@serxa, I have several assorted concerns/questions/assumptions (probably not all of them make sense) based on your graphs and explanations.

1. all loads precede all startups, right? (would be nice to make timeline more verbose on the graphs)
2. I used to think that startup is much more time consuming than load.
3. Your second picture illustrates CREATE MATERIALIZED VIEW mv TO table1 AS SELECT * FROM table2 leaving mv out of the scope, right?
4. view_dependencies we have in master are from view to table (view depends on its target table), I am not sure that current view dependencies by themselves are suitable to track dependencies from MV's target table to source(s) tables(s)
5. What we probably need - MV' source table(s) depends on target table
6. The whole point of the suggested change is to add more dependencies for startup (besides current load_table[table_id.getFullTableName()]->goals() we plan to introduce some extra ones that come from view dependencies)
7. Why do we need extra dependencies for normal tables? Implicit synchronization upon INSERT/SELECT works fine for them. Of course, we have to avoid race condition creating view dependencies, and it should be enough.
8. For streaming tables we have to have startup of all dependent (child) tables completed at the moment parent table' startup is called. For normal tables A and B that have a MV between, we can run startup methods in parallel. That is why I think that we have to treat streaming tables in a special way.

It might be more convenient to communicate via Telegram. May I write you (I know username)?

Looking into test_storage_s3_queue/test.py::test_registry failure.
Despite the fact it is known to be flaky.

Update: matter of delay. I've managed to reproduce this on my laptop, artificially slowing it down.

@kssenii , I suggest increasing timeout.

--- a/tests/integration/test_storage_s3_queue/test.py
+++ b/tests/integration/test_storage_s3_queue/test.py
@@ -2612,7 +2612,7 @@ def test_registry(started_cluster):
         )
 
     expected_rows = files_to_generate
-    for _ in range(20):
+    for _ in range(40):
         if expected_rows == get_count():
             break
         time.sleep(1)

I resolved new conflicts. It seems now mergeable again. Tests are green except the flaky one.

PR / Bugfix validation (pull_request)Failing after 60m

This thing I do not understand:

Run action failed for: [Bugfix validation] with exit code [-15]
Job timed out: [Bugfix validation] exit code [-15]
ERROR: Run action failed with timeout and did not generate JobReport - update dummy report with execution time
Command `python3 /home/ubuntu/actions-runner/_work/ClickHouse/ClickHouse/tests/ci/bugfix_validate_check.py` has failed, timeout 2400s is exceededRun action done for: [Bugfix validation]
ERROR: Job was killed - generate evidence
INFO:botocore.credentials:Found credentials from IAM Role: ec2_admin
Posting slack message, dry_run [False]
{}
ERROR: Run failed with exit code [241]

This thing I do not understand

It seems to be the same problem of long tests, but it showed an empty report, so I did not realize it at the first glance. Let's override mergeable check one more time and wait for private CI to finish.

This thing I do not understand

It seems to be the same problem of long tests, but it showed an empty report, so I did not realize it at the first glance. Let's override mergeable check one more time and wait for private CI to finish.

Seems reasonable unfortunately.
The test behaves like flaky check does.

Hello @serxa ,
what can I do to have it merged?

I think I messed up while resolving conflicts in private branch. And now there are failed tests. I was distracted, but I'll try to fix it now.

Private tests should be fixed now. At least local check does not reveal any problems. But public CI is in bad shape at the moment...

@ilejn Let's merge master into this branch one more time

Private Sync is finally green, but for some reason, CI here does not recognize this fact here.

I'm so glad it is finally merged. Thanks to everybody involved!

For visibility: #82222

Any chance of backporting it to 25.3?

Any chance of backporting it to 25.3?

No chances, this rework is too big