- superseded by catboostEvaluate() which no longer uses the internal
repository for external models
- also removed was statement SYSTEM RELOAD MODELS and the monitoring view
SYSTEM.SYSTEMMODELS
This commit moves the catboost model evaluation out of the server
process into the library-bridge binary. This serves two goals: On the
one hand, crashes / memory corruptions of the catboost library no longer
affect the server. On the other hand, we can forbid loading dynamic
libraries in the server (catboost was the last consumer of this
functionality), thus improving security.
SQL syntax:
SELECT
catboostEvaluate('/path/to/model.bin', FEAT_1, ..., FEAT_N) > 0 AS prediction,
ACTION AS target
FROM amazon_train
LIMIT 10
Required configuration:
<catboost_lib_path>/path/to/libcatboostmodel.so</catboost_lib_path>
*** Implementation Details ***
The internal protocol between the server and the library-bridge is
simple:
- HTTP GET on path "/extdict_ping":
A ping, used during the handshake to check if the library-bridge runs.
- HTTP POST on path "extdict_request"
(1) Send a "catboost_GetTreeCount" request from the server to the
bridge, containing a library path (e.g /home/user/libcatboost.so) and
a model path (e.g. /home/user/model.bin). Rirst, this unloads the
catboost library handler associated to the model path (if it was
loaded), then loads the catboost library handler associated to the
model path, then executes GetTreeCount() on the library handler and
finally sends the result back to the server. Step (1) is called once
by the server from FunctionCatBoostEvaluate::getReturnTypeImpl(). The
library path handler is unloaded in the beginning because it contains
state which may no longer be valid if the user runs
catboost("/path/to/model.bin", ...) more than once and if "model.bin"
was updated in between.
(2) Send "catboost_Evaluate" from the server to the bridge, containing
the model path and the features to run the interference on. Step (2)
is called multiple times (once per chunk) by the server from function
FunctionCatBoostEvaluate::executeImpl(). The library handler for the
given model path is expected to be already loaded by Step (1).
Fixes#27870
While pushign to MVs, there is a low-level code that create
ThreadGroupStatus/ThreadStatus, it is required to gather some metrics
for system.query_views_log.
But, one should not use ThreadGroupStatus of the MainThreadStatus, since
this structure can hold some state, that may not be cleaned, plus this
may be racy, instead it is better to create new ThreadGroupStatus and
attach it instead.
Also this place misses detachQuery(), and because of this it leaks
ThreadGroupStatus::finished_threads_counters_memory. But it is only the
problem pushing to MVs is done w/o query context (i.e. from Kafka/...),
since when it has query context detachQuery() will be called eventually.
Before this patch series, when I've tried the reproducer with
500 MVs attached to Kafka engine (that @den-crane suggested), jemalloc
report looks like this:
$ ../jeprof --text ~/ch/tmp/upstream/clickhouse-binary --base jeprof.44384.0.i0.heap jeprof.44384.167.i167.heap
Using local file /home/azat/ch/tmp/upstream/clickhouse-binary.
Using local file jeprof.44384.167.i167.heap.
Total: 915.6 MB
910.7 99.5% 99.5% 910.7 99.5% Snapshot (inline)
9.5 1.0% 100.5% 9.5 1.0% std::__1::__libcpp_operator_new (inline)
0.5 0.1% 100.6% 0.5 0.1% DB::TasksStatsCounters::create
And with focus to this place:
$ ../jeprof --focus Snapshot --text ~/ch/tmp/upstream/clickhouse-binary --base jeprof.44384.0.i0.heap jeprof.44384.167.i167.heap
Using local file /home/azat/ch/tmp/upstream/clickhouse-binary.
Using local file jeprof.44384.167.i167.heap.
Total: 915.6 MB
910.7 100.0% 100.0% 910.7 100.0% Snapshot (inline)
0.0 0.0% 100.0% 910.7 100.0% DB::QueryPipeline::reset
0.0 0.0% 100.0% 910.7 100.0% DB::StorageKafka::streamToViews
0.0 0.0% 100.0% 910.7 100.0% DB::StorageKafka::threadFunc
0.0 0.0% 100.0% 910.7 100.0% ProfileEvents::Counters::getPartiallyAtomicSnapshot
0.0 0.0% 100.0% 910.7 100.0% ~ThreadStatus
0.0 0.0% 100.0% 910.7 100.0% ~ViewRuntimeData
0.0 0.0% 100.0% 910.7 100.0% ~ViewRuntimeStats (inline)
Actually this report does not looks great (you understand it because I
stripped it), because --text does not that smart, but if you will use
--pdf for the report you will see the stacktrace (will attach pdf to the
pull request).
But after this patch series the process RSS does not goes beyond
~700MiB.
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
This can create leaks, since detachQuery() responsible for cleaning,
i.e. ThreadGroupStatus::finished_threads_counters_memory
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
One should not use MainThreadStatus, since ThreadGroupStatus can hold
some states, and it is better not to play with this, since this may
create leaks.
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
I have to do this, since I have libraries compiled with DWARF5 (i.e.
glibc).
ClickHouse changes:
- use camel_case
- add NOLINT
- avoid using folly:: (use std:: instead)
- avoid using boost:: (use std:: instead)
Refs: 490b287ca3
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
The problem is that the buffer size validated after marks reading in
MergeTreeReaderStream::init(), since it requires to read marks first.
And later it is passed to AsynchronousReadBufferFromFileDescriptor,
which throws LOGICAL_ERROR because buffer_size < alignment.
Fix this my simply disallow such values for max_read_buffer_size (I
thougt about modifying createReadBufferFromFileBase(), but it is not
used for remote reads -- remote_fs_buffer_size).
Fixes: #40669
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
