From 1f745e6174e6e9211d14261f80a90ce5b7160e42 Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Wed, 24 May 2023 15:48:54 +0200 Subject: [PATCH] Updated ColumnString::LoadBody made it more thread-safe, removed piece of code that confused lots of static analyzers (false positive on dereferencing nullptr-`block` variable) --- clickhouse/columns/string.cpp | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/clickhouse/columns/string.cpp b/clickhouse/columns/string.cpp index 937e6035..f6597bb4 100644 --- a/clickhouse/columns/string.cpp +++ b/clickhouse/columns/string.cpp @@ -252,27 +252,38 @@ void ColumnString::Append(ColumnRef column) { } bool ColumnString::LoadBody(InputStream* input, size_t rows) { - items_.clear(); - blocks_.clear(); + if (rows == 0) { + items_.clear(); + blocks_.clear(); + + return true; + } - items_.reserve(rows); - Block * block = nullptr; + decltype(items_) new_items; + decltype(blocks_) new_blocks; + + new_items.reserve(rows); + + // Suboptimzal if the first row string is >DEFAULT_BLOCK_SIZE, but that must be a very rare case. + Block * block = &new_blocks.emplace_back(DEFAULT_BLOCK_SIZE); - // TODO(performance): unroll a loop to a first row (to get rid of `blocks_.size() == 0` check) and the rest. for (size_t i = 0; i < rows; ++i) { uint64_t len; if (!WireFormat::ReadUInt64(*input, &len)) return false; - if (blocks_.size() == 0 || len > block->GetAvailable()) - block = &blocks_.emplace_back(std::max(DEFAULT_BLOCK_SIZE, len)); + if (len > block->GetAvailable()) + block = &new_blocks.emplace_back(std::max(DEFAULT_BLOCK_SIZE, len)); if (!WireFormat::ReadBytes(*input, block->GetCurrentWritePos(), len)) return false; - items_.emplace_back(block->ConsumeTailAsStringViewUnsafe(len)); + new_items.emplace_back(block->ConsumeTailAsStringViewUnsafe(len)); } + items_.swap(new_items); + blocks_.swap(new_blocks); + return true; }