Improve performance of SELECTs with active mutations

[azat]

Changelog category (leave one):

• Performance Improvement

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

Improve performance of SELECTs with active mutations

getAlterMutationCommandsForPart() can be a hot path for query execution when there are pending mutations.

• LOG_TEST - it is not only check one bool, but actually a bunch of atomics as well.

• Return std::vector over std::map (map is not required there) - no changes in performance.

• Copy only RENAME_COLUMN (since only this mutation is required by AlterConversions).

And here are results:

run	result
SELECT w/o ALTER	queries: 1565, QPS: 355.259, RPS: 355.259
SELECT w/ ALTER unpatched	queries: 2099, QPS: 220.623, RPS: 220.623
SELECT w/ ALTER and w/o LOG_TEST	queries: 2730, QPS: 235.859, RPS: 235.859
SELECT w/ ALTER and w/o LOG_TEST and w/ RENAME_COLUMN only	queries: 2995, QPS: 290.982, RPS: 290.982

But there are still room for improvements, at least MergeTree engines could implement getStorageSnapshotForQuery().

[robot-ch-test-poll]

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

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

Successful checks

Check name	Description	Status
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	✅ success
ClickBench	Runs [ClickBench](https://github.com/ClickHouse/ClickBench/) with instant-attach table	✅ success
ClickHouse build check	Builds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process	✅ success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker keeper image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docker server image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docs check	Builds and tests the documentation	✅ success
Fast test	Normally 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	✅ success
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integrational tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Mergeable Check	Checks if all other necessary checks are successful	✅ success
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	✅ success
SQLTest	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
SQLancer	Fuzzing tests that detect logical bugs with SQLancer tool	✅ success
Sqllogic	Run clickhouse on the sqllogic test set against sqlite and checks that all statements are passed	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Style check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success
Unit tests	Runs the unit tests for different release types	✅ success

Check name	Description	Status
A Sync	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	❌ failure
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	⏳ pending
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	❌ failure
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	❌ failure
Upgrade check	Runs stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts	❌ failure

[al13n321]

Looks reasonable. Please fix the perf test: https://s3.amazonaws.com/clickhouse-test-reports/59531/3d73043c12623fd746f2888e21d50b6144af00a2/performance_comparison_[3_4]/report.html

Other test failures look unrelated:

• Stress test: RESTORE TABLE: Failed assertion (internal || CurrentThread::get().getQueryContext()) #59570
• Integration tests: test_keeper_disks/test.py::test_logs_with_disks is flaky #59547

[al13n321]

Consider adding a comment about whether the returned vector is sorted. Looks like it isn't, and the caller doesn't care about the order? Also mention that only RENAME_COLUMN are included.

(I'm confused by this. I would've expected the order to matter: if a column was renamed twice A -> B -> C then it wouldn't work if we tried to do B -> C before A -> B. But I see that AlterConversions doesn't handle that. I guess such chain-rename is just not handled correctly here? It's also weird that getAlterMutationCommandsForPart() doesn't take a metadata snapshot, and instead returns commands to promote the part all the way to the latest schema; I guess that's also incorrect? If it used a metadata snapshot, maybe the whole thing would be correct because at most one ALTER can be in progress at a time, and dependent renames are not allowed within one ALTER. This is all just speculation, I'm not familiar with this code.)

[azat]

Also mention that only RENAME_COLUMN are included.

Ok, I've added the comment.

I would've expected the order to matter: if a column was renamed twice A -> B -> C then it wouldn't work if we tried to do B -> C before A -> B. But I see that AlterConversions doesn't handle that.

It don't need to, since only one ALTER_METADATA can be executed concurrently, so if you have two ALTER that modifies metadata the first one should be finished completely, only after that the second can proceed -

ClickHouse/src/Storages/MergeTree/ReplicatedMergeTreeQueue.cpp

Line 1455 in b22118a

if (!alter_sequence.canExecuteMetaAlter(entry.alter_version, state_lock))

.

It's also weird that getAlterMutationCommandsForPart() doesn't take a metadata snapshot, and instead returns commands to promote the part all the way to the latest schema; I guess that's also incorrect? If it used a metadata snapshot, maybe the whole thing would be correct because at most one ALTER can be in progress at a time, and dependent renames are not allowed within one ALTER

This code is required to handle RENAME COLUMN that had not been finished yet, while the metadata had been already updated, so when query contains new names it can transform new names to old ones, to find this data on the disk. Metadata snapshot does not have information about mutations so it cannot be used here.

[al13n321]

Does anything prevent the following scenario?:

1. A SELECT calls StorageMergeTree::read().
2. StorageMergeTree::read() grabs the current list of active parts. Say, it has one part: all_1_1_1, with one column A.
3. ALTER RENAME COLUMN A TO B starts and finishes.
4. ALTER RENAME COLUMN B TO C starts and finishes.
5. The SELECT from step 1 calls getAlterMutationCommandsForPart() for part all_1_1_1.
6. getAlterMutationCommandsForPart() returns both renames: A->B, B->C.
7. AlterConversions gets confused (with or without this PR), and the SELECT fails.
8. Even if AlterConversions could handle A->B->C correctly, this SELECT would still fail because it expects data to match the schema from the metadata snapshot obtained in step 1, so column C would be unexpected.

[azat]

Non-mutated part (all_1_1_1) should not have any mutations (i.e. getAlterConversionsForPart should return nothing), so it should be OK.

[al13n321]

getAlterConversionsForPart() -> getAlterMutationCommandsForPart() take all mutations that happened in the table (current_mutations_by_version) after this part was created (part_data_version). For all_1_1_1 that would be both of the renames. On SELECT path, getAlterConversionsForPart() is called from MergeTreeData::getStorageSnapshot().

Reproduced this scenario by adding a sleep in the middle of MergeTreeData::getStorageSnapshot() and doing ALTER while SELECT is stuck. The SELECT fails even with one ALTER, presumably because the metadata snapshot expects the old column name, while AlterConversions produce the new name:

:) desc (select * from a); select * from a;

DESCRIBE TABLE
(
    SELECT *
    FROM a
)

Query id: 0af8ba13-838c-4a21-9337-c0a1c2800636

┌─name─┬─type──┬─default_type─┬─default_expression─┬─comment─┬─codec_expression─┬─ttl_expression─┐
│ k    │ Int64 │              │                    │         │                  │                │
│ x    │ Int64 │              │                    │         │                  │                │
└──────┴───────┴──────────────┴────────────────────┴─────────┴──────────────────┴────────────────┘

2 rows in set. Elapsed: 0.001 sec. 


SELECT *
FROM a

Query id: b42a9aca-0d37-43bc-92f8-2c001efd203f


Elapsed: 10.014 sec. 

Received exception from server (version 24.2.1):
Code: 47. DB::Exception: Received from localhost:9000. DB::Exception: Missing columns: 'z' while processing query: 'SELECT k, z FROM a', required columns: 'k' 'z', maybe you meant: 'k' or 'z'. (UNKNOWN_IDENTIFIER)

(Also directly confirmed that the getAlterMutationCommandsForPart() returns 2 commands in this case, if there were 2 ALTERs.)

So I guess my understanding was correct.

This is all mostly unrelated to this PR, I just wanted to figure out if this code is as broken as it looks.

[azat]

Oh, yes, you are right, actually my comment was wrong it should be the opposite all_1_1_1 should have mutations, so yes, the SELECT will fail.

I guess this can be fixed by looking at which metadata version had bee used for the SELECT, but this is completely different story...

[azat]

AST fuzzer (asan) — Logical error: 'Cannot capture column 2 because it has incompatible type: got UInt8, but Nullable(UInt8) is expected.'.

• arrayFold: Logical error: 'Cannot capture column 3 because it has incompatible type: got Int64, but LowCardinality(Int64) is expected.' #59809

Stateless tests (debug) [2/5] — fail: 3, passed: 1174, skipped: 10

Timeouts:

• 02782_uniq_exact_parallel_merging_bug
• 01459_manual_write_to_replicas
• 01171_mv_select_insert_isolation_long

[al13n321]

Oops, turns out the cloud version of CH actually implements other mutation types in AlterConversions, not just RENAME_COLUMN (https://clickhouse.com/docs/en/guides/developer/lightweight-update).

I pushed a commit to this PR that adds AlterConversions::supportsMutationCommandType() and uses it instead of directly checking for RENAME_COLUMN in getAlterMutationCommandsForPart(), to make it less error-prone to maintain. And also flattened vector<MutationCommands> to just MutationCommands along the way. Feel free to review the changes, or revert them and push something else, or whatever.

[azat]

Thanks, LGTM! Though clang-tidy fails:

Feb 16 03:10:25 /build/src/Storages/MergeTree/ReplicatedMergeTreeQueue.cpp:1814:41: error: Dereference of undefined pointer value [clang-analyzer-core.NullDereference,-warnings-as-errors]
Feb 16 03:10:25 1814 | for (const auto & command : mutation_status->entry->commands | std::views::reverse)

[al13n321]

Huh, that warning makes absolutely no sense to me. Shuffled the code a little to work around it, guess it's a clang issue.