Fix LOGICAL_ERROR on race between DROP and INSERT with materialized views #39477
Conversation
robot-clickhouse
added
the
pr-bugfix
Fixed. |
azat
force-pushed
the
fix-mv-drop-race
branch
3 times, most recently
from
bf79422
to
ff42fe7
Compare
One more race? |
|
@azat thank you for the test and fix. I have managed to make a test which reproduces easily (every run in my env). Also I've made a little bit different fix output headers. Now we will not have an exception at all (but it is funny that pipelines for some threads still could be different :). |
By simply using more threads, I see, thanks!
I thought about this at first, but then decided that different pipelines for different threads maybe misleading, since some threads will push to the MV, others won't, so I preferred more strict variant. But I'm fine with your fix too, thanks! |
|
I think it is ok to have different pipeline in case if we are going to drop MV anyway. Looks a little bit better than exception. |
Fixed (and also rebased a little bit). P.S. Your test did not reproduce the issue for me, but I'm trying on a server with 2 xeons, so maybe your setup less powerful. |
…iews In case of parallel INSERT (max_insert_threads > 1) it is possible for VIEW to be DROP/DETACH'ed while building pipeline for various paralell streams, and in this case the header will not match since when you have VIEW you will got empty header and non-empty header otherwise. And this leads to LOGICAL_ERROR later, while checking that output headers are the same (in QueryPipelineBuilder::addChains() -> Pipe::addChains()). However it also makes the pipeline different for various parallel streams, and it looks like it is better to fail in this case, so instead of always returning empty header from buildChainImpl() explicit check had been added. Note, that I wasn't able to reproduce the issue with the added test, but CI may have more "luck" (although I've verified it manually). Fixes: ClickHouse#35902 Cc: @KochetovNicolai Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Instead of checking that number of processors different for different threads, simply always return empty header from buildChainImpl(), by adding explicit conversion. v2: ignore UNKNOWN_TABLE errors in test
|
|
New test fails with Ordinary database, see #39985 |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix LOGICAL_ERROR on race between DROP and INSERT with materialized views
In case of parallel INSERT (max_insert_threads > 1) it is possible for
VIEW to be DROP/DETACH'ed while building pipeline for various paralell
streams, and in this case the header will not match since when you have
VIEW you will got empty header and non-empty header otherwise.
And this leads to LOGICAL_ERROR later, while checking that output
headers are the same (in QueryPipelineBuilder::addChains() -> Pipe::addChains()).
However it also makes the pipeline different for various parallel
streams, and it looks like it is better to fail in this case, so instead
of always returning empty header from buildChainImpl() explicit check
had been added.
Note, that I wasn't able to reproduce the issue with the added test,
but CI may have more "luck" (although I've verified it manually).
Fixes: #35902 (cc @tavplubix)
Cc: @KochetovNicolai