Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance Issue in ClickHouseBulkCopy #153

Closed
mbtolou opened this issue Jul 19, 2022 · 15 comments
Closed

Performance Issue in ClickHouseBulkCopy #153

mbtolou opened this issue Jul 19, 2022 · 15 comments
Labels
enhancement New feature or request

Comments

@mbtolou
Copy link

mbtolou commented Jul 19, 2022

The driver for .net (.NET client for ClickHouse) (https://github.com/DarkWanderer/ClickHouse.Client) performs a select operation whenever there is a BulkCopy request (

using (var reader = (ClickHouseDataReader)await connection.ExecuteReaderAsync($"SELECT {GetColumnsExpression(columns)} FROM {DestinationTableName} LIMIT 0").ConfigureAwait(false))
), The consequence of this is an unnecessary increase of the execution time.

Suggested solution:

1 - Is it possible that in line 103 (

using (var reader = (ClickHouseDataReader)await connection.ExecuteReaderAsync($"SELECT {GetColumnsExpression(columns)} FROM {DestinationTableName} LIMIT 0").ConfigureAwait(false))
) provide the option for types to be declared by the developer, and only perform the select ($"SELECT {GetColumnsExpression(columns)} FROM {DestinationTableName} LIMIT 0")) line in the absence of provided types. (Making it possible for the developer to insert the types instead of doing a lookup to find them)

2 - It is also possible to perform the select operation using the following syntax:

select c1,c2,c3,c4,c5,c6,c7,c8,c9,c10,c11
from
  db.data_for_users
where
  (1 = 0);

Performing the following select syntax

select c1,c2,c3,c4,c5,c6,c7,c8,c9,c10,c11
from
  db.data_for_users
where
  limit 0;

Takes ~2000(ms) to be completed, while the following syntax as an alternative:

select c1,c2,c3,c4,c5,c6,c7,c8,c9,c10,c11
from
  db.data_for_users
where
  (1 = 0);

Takes only 15-25 (ms) to be completed. How can this huge time difference be alleviated?

which seems to perform the select operation much faster compared to the limit 0 syntax.

image

@DarkWanderer
Copy link
Owner

1 - Is it possible that in line 103 (....) provide the option for types to be declared by the developer, and only perform the select ($"SELECT {GetColumnsExpression(columns)} FROM {DestinationTableName} LIMIT 0")) line in the absence of provided types. (Making it possible for the developer to insert the types instead of doing a lookup to find them)

I don't think this makes a lot of sense as the types need to match exactly, otherwise ClickHouse will not be able to parse them (due to the way CH binary format works). I can certainly think about caching the SELECT operation result within BulkCopy object

Takes only 15-25 (ms) to be completed. How can this huge time difference be alleviated?

I can change LIMIT 0 to WHERE 1=0 - are you sure however this is faster in all cases?

@DarkWanderer DarkWanderer added the enhancement New feature or request label Aug 2, 2022
@mbtolou
Copy link
Author

mbtolou commented Aug 3, 2022

For where 1=1
10.0.0.138:9000, queries 1000, 🔴QPS: 28.922, RPS: 0.000, MiB/s: 0.000, result RPS: 0.000, result MiB/s: 0.000.

0.000% 0.024 sec.
10.000% 0.033 sec.
20.000% 0.033 sec.
30.000% 0.034 sec.
40.000% 0.034 sec.
50.000% 0.034 sec.
60.000% 0.034 sec.
70.000% 0.035 sec.
80.000% 0.035 sec.
90.000% 0.036 sec.
95.000% 0.038 sec.
99.000% 0.046 sec.
99.900% 0.081 sec.
99.990% 0.111 sec.

For limit 0
Queries executed: 1000.

10.0.0.138:9000, queries 1000, 🔵QPS: 4.252, RPS: 0.000, MiB/s: 0.000, result RPS: 0.000, result MiB/s: 0.000.

0.000% 0.199 sec.
10.000% 0.209 sec.
20.000% 0.212 sec.
30.000% 0.215 sec.
40.000% 0.218 sec.
50.000% 0.221 sec.
60.000% 0.225 sec.
70.000% 0.237 sec.
80.000% 0.251 sec.
90.000% 0.280 sec.
95.000% 0.312 sec.
99.000% 0.367 sec.
99.900% 0.468 sec.
99.990% 0.791 sec.

Suggestion: while as you mentioned, considering how the comparison engine of ClickHouse works under the hood, by making this input optional, you can give the user the warning that they need to be careful about the type matches (and as the developer they need to use the right type) and consequently they should handle any exceptions that might occur in the code themselves in case they use the wrong type.

The following picture is the report for such a query on database for the last four months, and as you can see it has taken near 20 hours of processor time, 2m+ times querying the DB, and when the DB is under heavy load it has taken 610+ ms.

image

Here you can see a few samples that it has taken a long time to perform SELECT on the databse.
image

Also, is it really necessary to lookup the version for the database each time you query the DB? Wouldn't it yield higher DB performance if you used a static variable to lookup the DB version just one time and store it in that?
image

@DarkWanderer
Copy link
Owner

DarkWanderer commented Aug 3, 2022

Thank you for performance comparison - I will change the selection criteria then

SELECT version() us used when opening new connection to determine which features the server supports. If you have a lot of these, it may be beneficial to keep connection objects for longer

P.S. maybe I can also cache table structure in BulkCopy object itself, will see

@mbtolou
Copy link
Author

mbtolou commented Aug 3, 2022

I will change the selection criteria then

thank.
Can't you determine features the first time you connect?

@mbtolou
Copy link
Author

mbtolou commented Aug 9, 2022

image

thank.

@mbtolou mbtolou closed this as completed Aug 10, 2022
@mbtolou
Copy link
Author

mbtolou commented Aug 18, 2022

Thank you for performance comparison - I will change the selection criteria then

SELECT version() us used when opening new connection to determine which features the server supports. If you have a lot of these, it may be beneficial to keep connection objects for longer

P.S. maybe I can also cache table structure in BulkCopy object itself, will see

According to your suggestion, I used static connection to inject data in the database.
But I ran into a strange problem.
My connection encounters an error once every few times of use.
I will explain my proposal in several parts:

  1. For the connection, define a version tag in the connection to avoid additional requests to get the version.
    I can keep the ClickHouseBulkCopy class static to avoid extra requests to get column names and types.

Currently, two additional requests are sent to the server for each BulkInsert:
Reading the version and getting the list of columns and their types
@DarkWanderer

⌚ 2022-08-17 00:05:53 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 00:30:44 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 00:34:56 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 01:51:09 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 02:05:40 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 02:22:17 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 02:47:22 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 05:07:53 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 05:16:24 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 05:46:43 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 06:23:58 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 07:09:43 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 07:19:39 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 08:17:16 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 09:00:56 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 09:45:37 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 11:27:37 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 11:58:36 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 12:35:47 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 12:54:18 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 13:00:29 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 13:35:37 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 15:34:34 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-17 19:29:38 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...
⌚ 2022-08-18 07:02:31 ⏩ [fatl] ⛪ [LogRepository:WriteDataWithBulk:258] ✅ Error Bulk Insert Into Table : ...

Call Stack IS:

⛔ System.AggregateException: One or more errors occurred. (An error occurred while sending the request.)
 ---> System.Net.Http.HttpRequestException: An error occurred while sending the request.
 ---> System.IO.IOException: The response ended prematurely.
   at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   --- End of inner exception stack trace ---
   at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.SendWithVersionDetectionAndRetryAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)
   at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.DecompressionHandler.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
   at ClickHouse.Client.ADO.ClickHouseCommand.PostSqlQueryAsync(String sqlQuery, CancellationToken token)
   at ClickHouse.Client.ADO.ClickHouseCommand.ExecuteDbDataReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken)
   at ClickHouse.Client.Copy.ClickHouseBulkCopy.WriteToServerAsync(IEnumerable`1 rows, IReadOnlyCollection`1 columns, CancellationToken token)
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
   at System.Threading.Tasks.Task.Wait()

@mbtolou mbtolou reopened this Aug 18, 2022
@mbtolou mbtolou closed this as completed Jul 5, 2023
@ruslanen
Copy link

ruslanen commented Jul 5, 2023

@mbtolou hi, how did you solve your problem?
I mean error: System.IO.IOException: The response ended prematurely.
It looks like some kind of floating bug. Could you describe, please?

@mbtolou
Copy link
Author

mbtolou commented Jul 6, 2023

I could not solve the problem

In version 6.7.1, the problem has decreased.
I thought maybe the problem was solved by the library creator.

I will reopen the issue to report again if it happens.

@mbtolou mbtolou reopened this Jul 6, 2023
@ruslanen
Copy link

ruslanen commented Jul 6, 2023

@mbtolou Thank you for your answer!
Yes, I had this problem but I can't reproduce it, if any information appears, I will post it here.

@DarkWanderer
Copy link
Owner

Do you use custom HttpClient, HttpClientHandler or IHttpClientFactory @ruslanen? I did indeed take measures to solve this, but it would still be possible to trigger it in certain circumstances

@ruslanen
Copy link

ruslanen commented Jul 6, 2023

@DarkWanderer yes, I use my own HttpClient:
Program.cs

  builder.Services.AddHttpClient("DataRefresh", client => client.Timeout = clickHouseConnectionStringBuilder.Timeout)
        .ConfigurePrimaryHttpMessageHandler(() =>
            new HttpClientHandler
            {
                AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate,
            });

For every query I create new ClickHouseConnection:

        var client = _httpClientFactory.CreateClient("DataRefresh");
        var connection = new ClickHouseConnection(_configuration["ClickHouseSettings:ConnectionString"], client);

I use it because I had an error: "Session is locked by concurrent client"
#152

But I have updated ClickHouse.Client recently from 4.2.2 to 6.7.1 and noticed this error System.IO.IOException: The response ended prematurely in logs of my app.

@ruslanen
Copy link

Stack trace:

Unhandled exception. System.Net.Http.HttpRequestException: An error occurred while sending the request.
 ---> System.IO.IOException: The response ended prematurely.
   at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   --- End of inner exception stack trace ---
   at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.SendWithVersionDetectionAndRetryAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)
   at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.DecompressionHandler.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
   at ClickHouse.Client.ADO.ClickHouseCommand.PostSqlQueryAsync(String sqlQuery, CancellationToken token)
   at ClickHouse.Client.ADO.ClickHouseCommand.ExecuteNonQueryAsync(CancellationToken cancellationToken)

(still don't have a scenario to reproduce)

@DarkWanderer
Copy link
Owner

Do you hold the connection reference until you read all of the results, or do you pass the results enumerator away while connection, command or reader object lose visibility? If GC collects objects while you read, you might see this issue

@ruslanen
Copy link

ruslanen commented Jul 13, 2023

Do you hold the connection reference until you read all of the results, or do you pass the results enumerator away while connection, command or reader object lose visibility? If GC collects objects while you read, you might see this issue

My queries don't read the result from reader (it's just insert or create table queries).
Common scenario:

        var client = _httpClientFactory.CreateClient("DataRefresh");
        var connection = new ClickHouseConnection(_configuration["ClickHouseSettings:ConnectionString"], client);
        await connection.OpenAsync();
        var command = connection.CreateCommand();
        command.CommandText = "create table ... as select ...";
        await command.ExecuteNonQueryAsync();

Btw, @DarkWanderer could you give some advices about cancellation, please?
#329

@plewam
Copy link

plewam commented Aug 23, 2023

Hi @ruslanen, @DarkWanderer,

I am facing the same issue. Getting this exception as well. In order to get to the bottom of it I implemented a stopwatch and it seems the exception is thrown immediately. After reading some issue reports in this repo I am quite sure the error was always there, however I am only seeing it since version 6.7.1 which fixed an issue with the exception not being thrown correctly. As of now I am creating a new ClickHouseConnection for every Bulk Insert by

using ClickHouseConnection connection = new(this.ConnectionString);
await connection.OpenAsync();

Do you think it would make sense to use a static HttpClient instead? The error reproduction is quite easy for me. I just need to create a high throughput scenario. I am now digging into two directions:

  1. Reduce the network throughput at my server
  2. Investigate multithreading issues
2023-08-22 16:23:21.246579 [ERR] [046_010] HttpRequestException occured after <0> ms. Exception <"System.Net.Http.HttpRequestException: An error occurred while sending the request.
 ---> System.IO.IOException: The response ended prematurely.
   at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage , Boolean , CancellationToken )
   --- End of inner exception stack trace ---
   at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage , Boolean , CancellationToken )
   at System.Net.Http.HttpConnectionPool.SendWithVersionDetectionAndRetryAsync(HttpRequestMessage , Boolean , Boolean , CancellationToken )
   at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage , Boolean , CancellationToken )
   at System.Net.Http.DecompressionHandler.SendAsync(HttpRequestMessage , Boolean , CancellationToken )
   at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage , HttpCompletionOption , CancellationTokenSource , Boolean , CancellationTokenSource , CancellationToken )
   at ClickHouse.Client.ADO.ClickHouseCommand.PostSqlQueryAsync(String sqlQuery, CancellationToken token)
   at ClickHouse.Client.ADO.ClickHouseCommand.ExecuteDbDataReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken)
   at ClickHouse.Client.Copy.ClickHouseBulkCopy.WriteToServerAsync(IEnumerable`1 rows, IReadOnlyCollection`1 columns, CancellationToken token)
   at Dft.Inside.Lib.ClickHouse.ClickHouseConn.Insert(IDataAgentEntity[] entities, Type type) in D:\DftInternalCollection\Inside\trunk\source\Dft.Inside.Lib.ClickHouse\ClickHouseConn.cs:line 289">  

@mbtolou mbtolou closed this as completed Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants