Skip to content

Fix LOGICAL_ERROR when Merge table tries to read from parametrized view#75318

Merged
kssenii merged 6 commits intomasterfrom
norge
Feb 18, 2025
Merged

Fix LOGICAL_ERROR when Merge table tries to read from parametrized view#75318
kssenii merged 6 commits intomasterfrom
norge

Conversation

@al13n321
Copy link
Copy Markdown
Member

@al13n321 al13n321 commented Jan 30, 2025

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Stress test failed like this: #74703 . There was a Merge table that merges lots of tables (everything named test.* in the whole stress test). The error implies that one of these tables had StorageInMemoryMetadata with no physical columns.

I spent a while skimming the create table test.* queries in server log looking for weird engines or column attributes or data types and trying to reproduce the error using those create table statements, but failed.

Also skimmed call sites of setInMemoryMetadata looking for one that may pass empty list of columns, and found one: patametrized view (when no parameters were passed). And it reproduces the crash!:

create view v as select number from numbers(5) where number%2={parity:Int8};
create table vv (number Int8) engine Merge('default','v');
select * from vv;

But the stress test failure was probably caused by something else as there's no create view (default\.)?test[^\. ]* .*\{ in server log.

So this PR makes StorageMerge:

  • Check for parametrized view and throw a non-LOGICAL_ERROR.
  • Add child table name to exception message.
  • Print child table name before building query plan for that table (because in debug build the server will SIGABRT on LOGICAL_ERROR before reaching the code that extends the exception message). This should make this more investigatable next time it happens.

Fixes: #74703

@al13n321 al13n321 added the 🍃 green ci 🌿 Fixing flaky tests in CI label Jan 30, 2025
@robot-ch-test-poll robot-ch-test-poll added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jan 30, 2025
@robot-ch-test-poll2
Copy link
Copy Markdown
Contributor

robot-ch-test-poll2 commented Jan 30, 2025

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

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

Check nameDescriptionStatus
Fast testNormally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here❌ failure
Successful checks
Check nameDescriptionStatus
Docs checkBuilds and tests the documentation✅ success
Style checkRuns a set of checks to keep the code style clean. If some of tests failed, see the related log from the report✅ success

Comment on lines +640 to +644
/// (Assuming that view has empty list of columns iff it's parametrized.)
if (storage->isView())
throw Exception(ErrorCodes::STORAGE_REQUIRES_PARAMETER, "Parametrized view can't be queried through a Merge table.");
else
throw Exception(ErrorCodes::LOGICAL_ERROR, "Table has no columns.");
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

(The real changes are: this, getLogger call above, and try-catch outside this. Everything else is indentation change from wrapping code in try-catch. Idk why git/github's diff algorithm failed so badly on this.)

ASTPtr required_columns_expr_list = std::make_shared<ASTExpressionList>();
ASTPtr column_expr;
/// (Assuming that view has empty list of columns iff it's parametrized.)
if (storage->isView())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

may be better like this?

Suggested change
if (storage->isView())
if (storage->isView() && storage->as<StorageView>() && storage->as<StorageView>()->isParameterizedView())

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Feb 14, 2025

Workflow [PR], commit [b7808a7]

@azat
Copy link
Copy Markdown
Member

azat commented Feb 16, 2025

We need this to make CI greener :)

query_info_.input_order_info = input_sorting_info;
}

auto logger = getLogger("StorageMerge");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BTW maybe it make sense to make it a member already (we can include a Merge table name as well there, as for MergeTree)?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's do this in follow-up PR, this PR is too green, it would be a shame not to merge :)

@nikitamikhaylov nikitamikhaylov added this pull request to the merge queue Feb 18, 2025
@nikitamikhaylov nikitamikhaylov removed this pull request from the merge queue due to a manual request Feb 18, 2025
@kssenii kssenii added this pull request to the merge queue Feb 18, 2025
Merged via the queue into master with commit 4a46778 Feb 18, 2025
117 checks passed
@kssenii kssenii deleted the norge branch February 18, 2025 12:36
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🍃 green ci 🌿 Fixing flaky tests in CI pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Logical error: 'No available columns'. (StorageMerge.cpp:1245: DB::ReadFromMerge::createPlanForTable)

6 participants