Skip to content
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

Fix distributed subquery max_query_size limitation inconsistency #34078

Merged

Conversation

godliness
Copy link
Contributor

@godliness godliness commented Jan 28, 2022

Changelog category (leave one):

  • Bug Fix (user-visible misbehaviour in official stable or prestable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix distributed subquery max_query_size limitation inconsistency

Information about CI checks: https://clickhouse.tech/docs/en/development/continuous-integration/

When insert distributed table under max_query_size limitation:
insert into demo values ('11', 11, 'haha') sql clause will be rewrite to INSERT INTO default.demo(name, id, dt) VALUES and send to local node from distributed node. when max_query_size set to 26, distributed node can parse the sql but local node not, because local node received the longer sql.
So we only let the initial node to check the max_query_size, secondary node for distributed table do not need to check it.

How to reproduce:

./clickhouse-client

drop table if exists demo_local;
CREATE TABLE default.demo_local
(
    `name` String,
    `id` UInt32,
    `dt` String
)

ENGINE = MergeTree
PARTITION BY dt
ORDER BY name
SETTINGS index_granularity = 8192

drop table if exists demo;
CREATE TABLE default.demo
(
    `name` String,
    `id` UInt32,
    `dt` String
)
ENGINE = Distributed('store', 'default', 'demo_local', rand())

set max_query_size=25;
insert into demo values ('11', 11, 'haha');

Syntax error: failed at position 21 ('values'):

insert into demo001 values ('11', 11, 'haha');

Max query size exceeded: 'values'
========================================
set max_query_size=26;
insert into demo values ('11', 11, 'haha');

Received exception from server (version 21.11.3):
Code: 62. DB::Exception: Received from localhost:9000. DB::Exception: Received from chi-ck-store-1-0.share-store.svc.cluster.local:9000. DB::Exception: Syntax error: failed at position 21 ('demo001'): demo001 (name, id, dt) VALUES. Max query size exceeded: 'demo001'. (SYNTAX_ERROR)

@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label Jan 28, 2022
@CLAassistant
Copy link

CLAassistant commented Jan 28, 2022

CLA assistant check
All committers have signed the CLA.

@godliness godliness changed the title Fix distributed subquery max query size limitation Fix distributed subquery max_query_size limitation inconsistency Jan 28, 2022
@alexey-milovidov alexey-milovidov added the can be tested Allows running workflows for external contributors label Jan 28, 2022
@godliness godliness force-pushed the fix-distributed-max-query-size branch from 2b1d6a9 to 826a532 Compare January 28, 2022 07:56
@godliness
Copy link
Contributor Author

I think these failed tests has no relationship with this PR.

@novikd novikd self-assigned this Jan 31, 2022
@novikd
Copy link
Member

novikd commented Jan 31, 2022

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jan 31, 2022

update

✅ Branch has been successfully updated

Copy link
Member

@novikd novikd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test from PR description

@godliness godliness force-pushed the fix-distributed-max-query-size branch from daa992e to fb13ffe Compare February 10, 2022 03:21
@godliness
Copy link
Contributor Author

godliness commented Feb 10, 2022

Add test from PR description

Done.

I think the failed tests are not related to this PR.

@novikd Would you please take a look?

@godliness godliness force-pushed the fix-distributed-max-query-size branch 2 times, most recently from 84e4e50 to 59b511c Compare February 13, 2022 12:39
@godliness
Copy link
Contributor Author

godliness commented Feb 14, 2022

What should i do next? @alexey-milovidov @novikd

The failed tests are not related to this PR.

@godliness godliness force-pushed the fix-distributed-max-query-size branch 3 times, most recently from ebbd9b5 to be8d9c7 Compare February 17, 2022 12:41
@Felixoid
Copy link
Member

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Feb 19, 2022

update

✅ Branch has been successfully updated

@godliness godliness force-pushed the fix-distributed-max-query-size branch 4 times, most recently from bec31fe to c28a9e9 Compare February 19, 2022 09:07
@Felixoid
Copy link
Member

Sorry, it has a failing test

@godliness godliness force-pushed the fix-distributed-max-query-size branch from c28a9e9 to 895396e Compare February 20, 2022 02:08
@godliness
Copy link
Contributor Author

godliness commented Feb 20, 2022

Sorry, it has a failing test

Thanks a lot, fixed the failed test and run again. @Felixoid PTAL

@Felixoid Felixoid self-assigned this Feb 21, 2022
@Felixoid
Copy link
Member

I've checked on 21.8, it raises Code: 62. DB::Exception: Received from localhost:9000. DB::Exception: Received from localhost:9000. DB::Exception: Syntax error: failed at position 25 ('data_00612'): data_00612. Max query size exceeded: 'data_00612'.

LGTM

@Felixoid Felixoid merged commit f690aba into ClickHouse:master Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can be tested Allows running workflows for external contributors pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants