I've noticed this in one of production setups, where lots of queries are
executed with distributed_group_by_no_merge=2 (automatically vai
optimize_skip_unused_shards optimization) and
optimize_aggregation_in_order=1, with this two settings initiator may
not read from some shards (i.e. X) for quite long period of time,
because it reads from other shards (i.e. Y) and it does not need
any data from X yet (due to it read everything in order), and this will
lead to query timeout, so timeouts had been increased.
Previously both timeouts had been tuned, but this leads to connection
hungs in case of abnormal machine reboots. So it is better to tune only
send_timeout (and this should be enough, since the only problem is the
sender) and this will allow to see that the connection is broken on the
initiator once it will read from this shard.
but after changing only send_timeout, the query still timedout, and the
reason is this place, which swaps this timeouts.
It had been introduced in 134efcd, with a comment:
NOTE: We use send_timeout for the receive timeout and vice versa (change arguments ordering in TimeoutSetter),
because send_timeout is client-side setting which has opposite meaning on the server side.
But it sounds odd to me, it may only make sense with the
clickhouse-client, since any other driver does not implement any server
settings.
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
* support async insert for native protocol
* use separate queue for async inserts via native protocol
* fix test
* better logging for async inserts and more tests
* disable mixed internal and external data in async inserts
* fix tests
* fix quota in async inserts
* disable async insert for secondary query of distributed
Added new type of authentication based on SSH keys. It works only for Native TCP protocol.
Co-authored-by: Nikita Mikhaylov <nikitamikhaylov@clickhouse.com>
Co-authored-by: Robert Schulze <robert@clickhouse.com>
Resolves: #51486
Until now, it was illegal to encode 64-bit (unsigned) integers with
MSB=1, i.e. values > (1ULL<<63) - 1, as var-int. In more detail, the
var-int code used by ClickHouse server and client spent at most 9 bytes
per value such that 9 * 7 = 63 bits could be encoded. Some 3rd party
clients (e.g. Rust clickhouse-rs) had the same limitation, whereas other
clients understand the full range (Python clickhouse-driver).
PRs #47608 and #48628 added sanity checks as asserts or exceptions
during var-int encoding on the server side. This was considered okay as
such huge integers so far occurred only during testing (usually fuzzing)
but not in practice.
Issue #51486 is a new fuzzing issue where the exception thrown from the
sanity check led to a half-baked progress packet and as a result, a
logical error / server crash.
The only fix which is not another bandaid is to allow the full range in
var-int coding. Clients will have to allow the full range too, a note
will be added to the changelog. (the alternative was to create another
protocol version but as var-int is used all over the place this was
considered infeasible)
Review note: this is the relevant commit.
Prevous implementation (DBMS_MIN_REVISION_WITH_INTERSERVER_SECRET)
accepts the salt from the client, which make it useless.
Reimplement the protocol to send the salt by the server and use it in
the client instead.
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
It is useful to understand what is going on, in some obscure cases, for
instance if someone will copy configuration from the production to some
docker env, and then you will see docker's private network addresses
in the logs.
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
It was possible back when destuctors may throw, in a pre C++11.
I've found one place where it may throw from 2012:
~UnionBlockInputStream()
{
...
if (exception && !std::uncaught_exception())
exception->rethrow();
...
}
Refs: 8a053aba54
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
* save format string for NetException
* format exceptions
* format exceptions 2
* format exceptions 3
* format exceptions 4
* format exceptions 5
* format exceptions 6
* fix
* format exceptions 7
* format exceptions 8
* Update MergeTreeIndexGin.cpp
* Update AggregateFunctionMap.cpp
* Update AggregateFunctionMap.cpp
* fix
- lots of static_cast
- add safe_cast
- types adjustments
- config
- IStorage::read/watch
- ...
- some TODO's (to convert types in future)
P.S. That was quite a journey...
v2: fixes after rebase
v3: fix conflicts after #42308 merged
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Move sending of progress and profile events after calling
BlockIO::onFinish() since it will call on_finish callback, that will do
the final flush of progress (at least WriteProgress).
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
This makes the target location consistent with other auto-generated
files like config_formats.h, config_core.h, and config_functions.h and
simplifies the build of clickhouse_common.
Right now RemoteInserter does not read ProfileEvents for INSERT, it
handles them only after sending the query or on finish.
But #37391 sends them for each INSERT block, but sometimes they can be
no ProfileEvents packet, since it sends only non-empty blocks.
And this adds too much complexity, and anyway ProfileEvents are useless
for the server, so let's send them only if the query is initial (i.e.
send by user).
Note, that it is okay to change the logic of sending ProfileEvents w/o
changing DBMS_TCP_PROTOCOL_VERSION, because there were no public
releases with the original patch included yet.
Fixes: #37391
Refs: #35075
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Official docs:
Some headers from C library were deprecated in C++ and are no longer
welcome in C++ codebases. Some have no effect in C++. For more details
refer to the C++ 14 Standard [depr.c.headers] section. This check
replaces C standard library headers with their C++ alternatives and
removes redundant ones.
Here is oneliner:
$ gg 'LOG_\(DEBUG\|TRACE\|INFO\|TEST\|WARNING\|ERROR\|FATAL\)([^,]*, [a-zA-Z]' -- :*.cpp :*.h | cut -d: -f1 | sort -u | xargs -r sed -E -i 's#(LOG_[A-Z]*)\(([^,]*), ([A-Za-z][^,)]*)#\1(\2, fmt::runtime(\3)#'
Note, that I tried to do this with coccinelle (tool for semantic
patchin), but it cannot parse C++:
$ cat fmt.cocci
@@
expression log;
expression var;
@@
-LOG_DEBUG(log, var)
+LOG_DEBUG(log, fmt::runtime(var))
I've also tried to use some macros/templates magic to do this implicitly
in logger_useful.h, but I failed to do so, and apparently it is not
possible for now.
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
v2: manual fixes
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
This allows starting and stopping separately each protocol server
without restarting ClickHouse.
This also allows adding or removing `listen_host` entries, which
start and stops servers for all enabled ports.
When stopping a server, the listening socket is immediately closed
(and available for another server).
Protocols with persistent connections try to wait for any currently
running query to finish before closing the connection, but idle
connection are closed quickly (depending on how often the protocol
is polled).
An extra ProfileEvent is added, `MainConfigLoads`, it is
incremented every time the configuration is reloaded. This helps
when trying to assess whether the new configuration was applied.
This is not that important, but because of packets overlaps the client
may not render the fatal error.
You will find TSan report in `details`.
<details>
```
WARNING: ThreadSanitizer: data race (pid=13384)
Write of size 8 at 0x7b24000ad818 by thread T47:
0 DB::WriteBuffer::next() obj-x86_64-linux-gnu/../src/IO/WriteBuffer.h:43:15 (clickhouse-tsan+0x16124d61)
1 DB::TCPHandler::sendProgress() obj-x86_64-linux-gnu/../src/Server/TCPHandler.cpp:1602:10 (clickhouse-tsan+0x16124d61)
2 DB::TCPHandler::runImpl()::$_6::operator()() const obj-x86_64-linux-gnu/../src/Server/TCPHandler.cpp:318:25 (clickhouse-tsan+0x1612cf54)
Previous write of size 8 at 0x7b24000ad818 by thread T229:
0 DB::WriteBuffer::next() obj-x86_64-linux-gnu/../src/IO/WriteBuffer.h:43:15 (clickhouse-tsan+0x1612b793)
1 DB::TCPHandler::sendLogData(DB::Block const&) obj-x86_64-linux-gnu/../src/Server/TCPHandler.cpp:1561:10 (clickhouse-tsan+0x1612b793)
2 DB::TCPHandler::sendLogs() obj-x86_64-linux-gnu/../src/Server/TCPHandler.cpp:1632:9 (clickhouse-tsan+0x161228ab)
3 DB::TCPHandler::runImpl()::$_1::operator()() const obj-x86_64-linux-gnu/../src/Server/TCPHandler.cpp:229:62 (clickhouse-tsan+0x1612c4e1)
```
</details>
Before this patch INSERT via Buffer/Kafka may re-use previously set user
for that connection, while this is not correct, it should reset the
user, and use global context.
Note, before [1] there was a fallback to default user, but that code had
been removed, and now it got back.
[1]: 0159c74f21 ("Secure inter-cluster query execution (with initial_user as current query user) [v3]")
Also note, that context for Buffer table (and others) cannot be changed,
since they don't have any user only profile.
I've tested this patch manually using the following:
create table dist (key Int) engine=Distributed(test_cluster_two_shards_secure, default, data, key);
create table buffer (key Int) engine=Buffer(default, dist, 1, 0, 0, 0, 0, 0, 0);
create table data (key Int) engine=Memory();
# to start the connection with readonly user
$ clickhouse-client --user readonly -q 'select * from dist'
$ clickhouse-client -q 'insert into buffer values (1)'
# before this patch this produces errors like:
# 2021.09.27 23:46:48.384920 [ 19474 ] {} <Error> default.dist.DirectoryMonitor: Code: 164. DB::Exception: Received from 127.0.0.2:9000. DB::Exception: readonly: Cannot execute query in readonly mode. Stack trace:
v2: reset the authentication instead of using default user (as suggested by @vitlibar)
v3: reset Session::user and introduce ClientInfo::resetAuthentication (as suggested by @vitlibar)
v4: reset the session every time in interserver mode (suggested by @vitlibar)
