Disambiguate alter commands list

[antaljanosbenjamin]

Changelog category (leave one):

• Improvement

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

Allow alter operations to be surrounded by parenthesis. The emission of parentheses can be controlled by the format_alter_operations_with_parentheses config. By default in formatted queries the parentheses are emitted as we store the formatted alter operations in some places as metadata (e.g.: mutations). The new syntax clarifies some of the queries where alter operations end in a list. E.g.: ALTER TABLE x MODIFY TTL date GROUP BY a, b, DROP COLUMN c cannot be parsed properly with the old syntax. In the new syntax the query ALTER TABLE x (MODIFY TTL date GROUP BY a, b), (DROP COLUMN c) is obvious. Older versions are not able to read the new syntax, therefore using the new syntax might cause issues if newer and older version of ClickHouse are mixed in a single cluster.

Documentation entry for user-facing changes

• Documentation is written (mandatory for new features)

[robot-clickhouse-ci-1]

This is an automated comment for commit 425799b 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
A Sync	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
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
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	✅ 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
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	✅ success

Check name	Description	Status
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
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	❌ failure

[UnamedRus]

Can it cover as well #17303?

[antaljanosbenjamin]

@UnamedRus I will check that issue, but it seems like not a parsing, but a logical issue there.

[UnamedRus]

Actually yeah, my memory fails me.
There was another issue with comma, but for TTL GROUP BY #17304 and it fixed.

[antaljanosbenjamin]

The scope of the PR is changed: because the necessary part is only the alter commands and the TTL elements are harder to parse properly ((expr) can be either a TTL element with parens inside and with the expression expr, or just a TTL element without parens with the expression (expr), it won't try to fix that now.

[antaljanosbenjamin]

it fixed.

Actually there is still an issue if you don't use the SET keyword, so e.g.:

CREATE TABLE test_table
(
    `key_a` UInt32,
    `key_b` UInt32,
    `ts` DateTime,
    `value` UInt32
)
ENGINE = MergeTree
PARTITION BY tuple()
ORDER BY (key_a, key_b)
TTL
    ts GROUP BY key_a, key_b,
    ts + toIntervalMonth(2)


Received exception from server (version 24.2.1):
Code: 450. DB::Exception: Received from localhost:9000. DB::Exception: TTL Expression GROUP BY key should be a prefix of primary key. (BAD_TTL_EXPRESSION)

Here ts + toIntervalMonth(2) is parsed as GROUP BY, which is bad. I will create an issue about this.

[antaljanosbenjamin]

So about this issue, I got stuck with some build issues. With Sasha we talked about that I shouldn't format the queries with parenthesis by default, because that makes online upgrading dangerous: if an alter command serialized by a new version reaches a node with an older version, then the parsing will fail on that node probably causing similar issues like when a user now runs ALTER TABLE x MODIFY TTL expr GROUP BY expr queries, but for all alter queries. That sounds a bit scary to me. As a result I tried to introduce a server setting to control the behavior of the parser, but it turns out this is more tricky than I thought. The two main approach here:

1. Access the server settings in ASTAlterQuery::formatImpl and print the parentheses if the flag is set.
2. Introduce a new field in FormatSettings and set that flag from the places where we know we want to serialize alter commands. This could cover most of the places, most critically where we serialize the mutation commands. However as we serialize ASTs in many different ways, we should get the server setting and transfer the flag to FormatSettings and then serialize the AST. For me this is kind of a bad solution, because then we don't have a well centralized place to handle this and there might be inconsistencies between different places. Right now most of the things are handled by the formatAST and serializeAST functions. It would be nice to read the server settings there, because then we have a somewhat centralized place to deal with this, but if this would be possible, than we could also go with option1.

However there is an issue with option1: in order to access the server settings, we need to call Context::getGlobalContextInstance()->getServerSettings(), which is defined in src/Interpreters/Context.cpp which is part of the dmbs lib in our CMake. This means everytime we link clickhouse_parsers to a somewhere, we should also link dbms. For me this is too intrusive solution, .e.g.: we would need to link dbms to clickhouse_compression, because it links to clickhouse_parsers. That means, we have to link dbms to clickhouse_common_zookeeper_no_log and clickhouse_common_zookeeper, which doesn't seem like a good idea from the dependency point of view.

If we opted for do not have server settings, then we would have to wait until the oldest LTS version of clickhouse will contain the new parsers capable of parsing alter commands with parentheses to fix this bug, or we should introduce a change where the upgrade from an older version can break online clusters if they do alters during upgrades.

I don't feel any if the above choice is a good solution, so I am here to ask you, which one should we do:

1. Have a server setting
   • access the server settings directly from ASTAlterQuery and link dbms to every target which links against clickhouse_compression
   • access the server settings only in places where we think it is necessary to fix the bug
2. Do not have a server setting
   • first roll out the new parsers in the next CH version, then in the near future roll out a version which not just parses, but also formats the alter queries with parentheses
   • just roll out the new parsing and formatting and make a fat note about this in our release notes
3. something else

I really don't see a clearly good solution here.

As a result I will introduce a global variable that will be updated with the value of the regarding server setting in order to bridge the gap between server settings and the parsers. It is meant to a temporary solution until we can enforce the serialization with the new format.

[UnamedRus]

BTW, when change happened to D DDL worker, it was done via user level setting distributed_ddl_entry_format_version, with ~1 year delay

#35463

[tavplubix]

BTW, when change happened to D DDL worker, it was done via user level setting distributed_ddl_entry_format_version, with ~1 year delay

#35463

This change is not anyhow related to DDLWorker. There's already a compatibility setting (format_alter_operations_with_parentheses, disabled by default) that controls the new behavior, so nothing to do with distributed_ddl_entry_format_version

[tavplubix]

Should we call it in clickhouse-client and clickhouse-local as well? They print a formatted query:

localhost.localdomain :) SeLeCt NuLl

SELECT NULL

Query id: fdbaa7b8-75c0-4f58-b79c-46b9ae290e1e

┌─NULL─┐
│ ᴺᵁᴸᴸ │
└──────┘

1 row in set. Elapsed: 0.005 sec. 

I think it's fine to unconditionally set it to true for that purpose

[tavplubix]

Does it make sense to always allow round brackets when parsing, despite the value of the setting? So the setting will be only used for formatting, and the parser will be able to always understand both versions

[antaljanosbenjamin]

The idea is by allowing parsing with parentheses by default is the transition can be smoother during migrating to newer versions. E.g. during live migration if there are one replica which produces alter commands with parens, then the older replicas will parse it properly.

[antaljanosbenjamin]

Maybe my previous answer wasn't the best. So I wanted to avoid allowing the "mix" of the old and new syntax. So in an alter command list each command must or mustn't have to be surrounded with parentheses. E.g.: ALTER TABLE x MODIFY TTL, DROP COLUM a is fine, ALTER TABLE x (MODIFY TTL), (DROP COLUMN a) also fine, but ALTER TABLE x (MODIFY TTL), DROP COLUMN a is not fine.

This means in some way ParserAlterCommandList has to be able to enforce it. This could be done in two ways in my opinion:

1. The list parser figures this out and passes it to the individual command parser.
2. The individual command parser decides whether parens should be used or not and because in ParserAlterCommandList::parseImpl only a single parser is used, it can carry over this information. It is doable, but it would introduce in my opnion a hidden relation between the first parsed command and every command after that. I think right now parsers doesn't have this kind of "memory" in general. Thus, I opted for option 1.

That's why here determining with_round_bracket is necessary. What do you think?

[tavplubix]

So in an alter command list each command must or mustn't have to be surrounded with parentheses.

Yes, I agree.

This means in some way ParserAlterCommandList has to be able to enforce it. ... That's why here determining with_round_bracket is necessary.

Sorry, I missed this when reading the code, it was unobvious.
Yes, the first option is better because in the worst case, someone will pass true or false unconditionally instead of proper with_round_bracket flag (so only one version of the syntax will work in this place). And with the second option, someone may use one ParserAlterCommand for multiple queries, and it will cause an obscure bug

[tavplubix]

We can enable it in functional tests by adding this config to tests/config/install.sh
Btw, functional tests are much better than integration tests: #39359 (comment)
(but it's not necessary to rewrite existing tests)

[antaljanosbenjamin]

I checked this, and if we would change this in functional tests, then we would change this for all of the functional tests. I think it shouldn't cause an issue, but we would test something that is not the "default" behavior of clickhouse.

Furthermore I used the same queries in functional tests, but expecting the old behavior. So I would say the only question is which way should be the default in functional tests? The other one has to be tested in integration test either way, because we have to change the server setting.

[antaljanosbenjamin]

Test failures:

• AST Fuzzer (ubsan) failed with Logical error: 'Expected single dictionary argument for function.'. Logical error: 'Expected single dictionary argument for function.'. (Analyzer) #58979
• Integration Tests (asan, Analyzer), Integration Tests (tsan) failed test_replicated_database/test.py::test_startup_without_zk Revert "Do not retry queries if container is down in integration tests" #60215
• Stateless Tests (tsan, S3 Storage) failed 00984_parser_stack_overflow Revert "Check stack size in Parser" #60216
• Stress Test (msan) failed with a fatal error, see below.

2024.02.21 02:22:36.637567 [ 33028 ] {} <Trace> DatabaseAtomic (test_15): Loading table test_15.test_param_view2
2024.02.21 02:22:36.637759 [ 33028 ] {} <Fatal> : Logical error: 'Invalid storage definition in metadata file: it's a bug or result of manual intervention in metadata files'.

The table test_param_view2 is created in tests/queries/0_stateless/02818_parameterized_view_with_cte_multiple_usage.sql:

create view test_param_view2 as
with {param_test_val:UInt8} as param_test_val
select param_test_val,
       arrayCount((a)->(a < param_test_val), t.arr) as cnt1,
       arrayCount((a)->(a < param_test_val+1), t.arr) as cnt2
from (select [1,2,3,4,5] as arr) t;

I don't think it has anything to do with my changes, so I will merge, but because our CI was a bit messy lately, I will merge a current master to see the actual state of tests. Maybe some unreported issues are present.

[antaljanosbenjamin]

• test_backup_restore_s3/test.py::test_user_specific_auth Support specifying users for S3 settings #60144 (comment)
• test_materialized_mysql_database/test.py::test_table_table Fix race on Context::async_insert_queue #59082
• 01747_system_session_log_long, 01702_system_query_log, 01656_test_query_log_factories_info, 01600_log_queries_with_extensive_info CI: Timeouts on SYSTEM FLUSH LOGS #57984