LOGICAL_ERROR: Next task callback is not set for query by CheSema · Pull Request #84753 · ClickHouse/ClickHouse

CheSema · 2025-07-30T20:29:33Z

a new test 03579_create_table_populate_from_s3 catches
DB::Exception: Next task callback is not set for query .
Stateless tests (amd_binary, old analyzer, s3 storage, DatabaseReplicated, parallel)

It turned out that all *Cluster functions were broken with replicated DDL.

/// QueryKind is overused.
/// It is used to distinguish queries that are executed on worker nodes in the following cases:
/// 1. When a query is executed on a worker node in a distributed query with parallel *MergeTree replicas
/// 2. When a query is executed on a worker node in *Cluster functions like S3Cluster, etc.
/// 3. When a query is executed on a worker node in ON CLUSTER operations
/// As a result, it is difficult or impossible to run a query with parallel *MergeTree replicas that uses *Cluster functions,
/// because the query is marked as QueryKind::SECONDARY_QUERY but it is not clear for what purpose: parallel replicas or *Cluster functions.
/// In contrast to ON CLUSTER queries, when queries are executed on worker nodes in DDL replication, they are not marked as QueryKind::SECONDARY_QUERY.
/// Related to that case, there was a bug when a query in DDL replication had *Cluster functions; *Cluster functions expect a distributed file iterator which is not set in DDL replication.
/// When a query is marked as QueryKind::SECONDARY_QUERY, parsing and execution of the query is different.
/// Not all optimizations are applied, not all checks are made. The query is considered as already well-prepared and safe to execute.

Changelog category (leave one):

Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fixing exception LOGICAL_ERROR: Next task callback is not set for query

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

clickhouse-gh · 2025-07-30T20:29:59Z

Workflow [PR], commit [c585f41]

Summary: ❌

job_name	test_name	status
Stateless tests (amd_binary, old analyzer, s3 storage, DatabaseReplicated, sequential)		failure
	01154_move_partition_long	FAIL
Bugfix validation (functional tests)		failure
Stress test (amd_ubsan)		failure
	Server died	FAIL
	Hung check failed, possible deadlock found (see hung_check.log)	FAIL
	Killed by signal (in clickhouse-server.log)	FAIL
	Fatal message in clickhouse-server.log (see fatal_messages.txt)	FAIL
	Killed by signal (output files)	FAIL
	Found signal in gdb.log	FAIL

CheSema · 2025-07-31T11:02:38Z

It seems to be relevant to the feature #70659

CheSema · 2025-08-01T17:57:43Z

src/Interpreters/DDLTask.cpp

 {
    auto query_context = DDLTaskBase::makeQueryContext(from_context, zookeeper);
-    query_context->setQueryKind(ClientInfo::QueryKind::SECONDARY_QUERY);
+    // Do not set here SECONDARY_QUERY


Here is the root cause of the issue.

CheSema · 2025-08-01T20:58:03Z

Integration tests (asan, old analyzer, 5/6)required
flaky tests test_restore_replica
#84855

src/Planner/Planner.cpp

src/Storages/ObjectStorage/StorageObjectStorageSource.cpp

tests/queries/0_stateless/03579_create_table_populate_from_s3.sh

devcrafter · 2025-08-05T13:15:15Z

Let's clarify the context of the problem somewhere in the comments, - the conflict on query_kind in context between DDL and DML parts of the query with CREATE AS SELECT. Can't we in general create another context for SELECT part?

CheSema · 2025-08-07T12:11:39Z

Can't we in general create another context for SELECT part?

I think, yes, we can create different context for different parts of query.

devcrafter · 2025-08-07T21:12:47Z

Can't we in general create another context for SELECT part?

I think, yes, we can create different context for different parts of query.

Then wouldn't it be easier to create new context for select and use update the query kind there?

src/Planner/Planner.cpp

CheSema · 2025-08-07T21:15:33Z

Can't we in general create another context for SELECT part?

I think, yes, we can create different context for different parts of query.

Then wouldn't it be easier to create new context for select and use update the query kind there?

I do not know how. My response was more theoretical. That could be good approach, but I do not know how to implement it. Research is required.

devcrafter · 2025-08-08T12:28:10Z

Can't we in general create another context for SELECT part?

I think, yes, we can create different context for different parts of query.

Then wouldn't it be easier to create new context for select and use update the query kind there?

I do not know how. My response was more theoretical. That could be good approach, but I do not know how to implement it. Research is required.

I assume this is the place where we can update the context and run insert select with it

ClickHouse/src/Interpreters/InterpreterCreateQuery.cpp

Lines 2229 to 2236 in 9fd86cc

    
           return InterpreterInsertQuery( 
        
                      insert, 
        
                      getContext(), 
        
                      getContext()->getSettingsRef()[Setting::insert_allow_materialized_columns], 
        
                      /* no_squash */ false, 
        
                      /* no_destination */ false, 
        
                      /* async_isnert */ false) 
        
               .execute();

CheSema · 2025-08-12T14:32:41Z

Can't we in general create another context for SELECT part?

I think, yes, we can create different context for different parts of query.

Then wouldn't it be easier to create new context for select and use update the query kind there?

I do not know how. My response was more theoretical. That could be good approach, but I do not know how to implement it. Research is required.

I assume this is the place where we can update the context and run insert select with it

ClickHouse/src/Interpreters/InterpreterCreateQuery.cpp

Lines 2229 to 2236 in 9fd86cc

return InterpreterInsertQuery(

insert,

getContext(),

getContext()->getSettingsRef()[Setting::insert_allow_materialized_columns],

/* no_squash */ false,

/* no_destination */ false,

/* async_isnert */ false)

.execute();

It does not work.

https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=84753&sha=e10634b9148e2bc1ab809184cd994c4b147e360a&name_0=PR&name_1=Stateless+tests+%28amd_binary%2C+old+analyzer%2C+s3+storage%2C+DatabaseReplicated%2C+parallel%29

at QueryAnalyzer::resolveTableFunction
table function is executed scope_context->getQueryContext()->executeTableFunction
with context from QueryAnalyzer auto & scope_context = scope.context;.
In that context query kind is set as SECONDARY_QUERY.

devcrafter · 2025-08-13T21:10:25Z

src/Interpreters/InterpreterCreateQuery.cpp

+    SCOPE_EXIT({
+        getContext()->setQueryKind(previous_query_kind);
+    });
+    getContext()->setQueryKind(ClientInfo::QueryKind::INITIAL_QUERY);


Please check CREATE AS SELECT queries (w/o REPLACE). AFAIS, it's different code path and doesn't use Context::setQueryContext()

I do not understand what do you mean.

InterpreterCreateQuery::doCreateOrReplaceTable also calls fillTableIfNeeded.

CheSema · 2025-08-14T15:14:56Z

We got in dead end here.

I think that the version on f8e3e07ec248e8fe01336b0bffc9247fc1c32ae6 commit it the right way to go.
But Igor does not have a strong opinion here and could not approve that version of code.
In other hand I could not agree with the current way. This is a tricky way, if some thing goes wrong, I do not know how it should be transformed further.

The alternative way is forbid to run create as select with *Cluster functions.

devcrafter · 2025-08-15T12:22:41Z

The alternative way is forbid to run create as select with *Cluster functions.

Let's first, simply prevent replacing s3 with s3cluster in DDL queries. cc @thevar1able

thevar1able · 2025-08-16T11:10:04Z

Test 03579_create_table_populate_from_s3 is green on master.
Closing in favor of #85734

add a test

003e10a

clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jul 30, 2025

CheSema changed the title ~~add a test when create table as select from s3 function~~ LOGICAL_ERROR: Next task callback is not set for query Jul 31, 2025

fix cluster function in DDL queries

6a6a629

clickhouse-gh bot added pr-bugfix Pull request with bugfix, not backported by default and removed pr-not-for-changelog This PR should not be mentioned in the changelog labels Jul 31, 2025

thevar1able self-assigned this Jul 31, 2025

CheSema added 6 commits August 1, 2025 13:26

DDL replication create INITIAL_QUERY context

ec62650

style fix

2aca7c7

fix debug logs

cc6cd64

fix is_replicated_database condition

868e02f

fix style

d949357

fix on cluster DDL

9753800

CheSema assigned devcrafter Aug 1, 2025

CheSema commented Aug 1, 2025

View reviewed changes

CheSema marked this pull request as ready for review August 1, 2025 21:08

CheSema added 3 commits August 1, 2025 23:15

less logs

700260b

test memory engine

475cd36

simplify test

17b21b9

devcrafter reviewed Aug 5, 2025

View reviewed changes

src/Planner/Planner.cpp Show resolved Hide resolved

src/Storages/ObjectStorage/StorageObjectStorageSource.cpp Outdated Show resolved Hide resolved

tests/queries/0_stateless/03579_create_table_populate_from_s3.sh Show resolved Hide resolved

restore log messg, add comment

5b26b3a

CheSema requested a review from devcrafter August 7, 2025 12:30

rm comment

668a445

CheSema added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Aug 7, 2025

devcrafter reviewed Aug 7, 2025

View reviewed changes

src/Planner/Planner.cpp Show resolved Hide resolved

CheSema added 2 commits August 7, 2025 23:17

revert changes in Planner.cpp

0c5594f

Merge branch 'master' into chesema-task-callback

f8e3e07

set INITIAL_QUERY for insert part in create as select query

32f7bb9

CheSema requested a review from devcrafter August 11, 2025 14:25

CheSema added 3 commits August 11, 2025 16:29

revert first solution leave only second

e10634b

add logs

b8b3564

debug

ca58910

CheSema added 4 commits August 12, 2025 17:47

debug

d926ccb

fix style

3b248c8

debug

02a6284

style

c585f41

devcrafter reviewed Aug 13, 2025

View reviewed changes

thevar1able added a commit that referenced this pull request Aug 16, 2025

Add a test from #84753

53b01c8

thevar1able closed this Aug 16, 2025

Conversation

CheSema commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Documentation entry for user-facing changes

Uh oh!

clickhouse-gh bot commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CheSema commented Jul 31, 2025

Uh oh!

CheSema Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

CheSema commented Aug 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devcrafter commented Aug 5, 2025

Uh oh!

CheSema commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devcrafter commented Aug 7, 2025

Uh oh!

Uh oh!

CheSema commented Aug 7, 2025

Uh oh!

devcrafter commented Aug 8, 2025

Uh oh!

CheSema commented Aug 12, 2025

Uh oh!

devcrafter Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

CheSema Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

CheSema commented Aug 14, 2025

Uh oh!

devcrafter commented Aug 15, 2025

Uh oh!

thevar1able commented Aug 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CheSema commented Jul 30, 2025 •

edited

Loading

clickhouse-gh bot commented Jul 30, 2025 •

edited

Loading

CheSema commented Aug 7, 2025 •

edited

Loading