Do not fuzz queries with implicit transactions#97861
Do not fuzz queries with implicit transactions#97861antaljanosbenjamin wants to merge 3 commits intomasterfrom
Conversation
|
Workflow [PR], commit [bcebafb] Summary: ❌
|
clickhouse-gh
bot
added
the
pr-not-for-changelog
@antaljanosbenjamin care to elaborate or provide a proof to understand why it's broken with implicit transactions? |
src/Interpreters/executeQuery.cpp
Outdated
| // Implicit transactions cause issues with fuzzing because we copy the query context which can contain an already | ||
| // committed transaction. Because of the different assertions in setCurrentTransaction, initCurrentTransaction and | ||
| // MergeTreeTransactionHolder, it is not trivial how to start a new transaction in the same session context for a | ||
| // new query, thus let's just not fuzz queries with implicit transactions. |
There was a problem hiding this comment.
I see. The explanation is here. I think this comment should use /// as per the coding style. Also, this same explanation could have been added to the commit message itself, and definitely should have been added to the description (and IMHO also in the changelog)
|
Maybe #97835 is better |
|
Interesting - if we disable transactions and clean up the copied context from the in-progress transaction, will it be better? |
|
I am not sure how to make that work. I am not sure if having different transactions in session and query context is okay or not. Second, to make it work properly, we have to use MergeTreeTransactionHolder which can be used only with session context as far as I understood. I opted to disable it, because it is an experimental feature. Azat's PR might make it work though. |
azat
left a comment
azat
left a comment
There was a problem hiding this comment.
Let's merge this one first to fix CI, I will finish mine later (need to solve leaking of transaction from session context)
|
Although last version should work |
|
Mine failed the tests, your passed. Will close this PR. |
4 participants
#97568 introduced server side fuzzing, but it is broken with implicit transactions.
Implicit transactions cause issues with fuzzing because we copy the query context which can contain an already committed transaction. Because of the different assertions in setCurrentTransaction, initCurrentTransaction and MergeTreeTransactionHolder, it is not trivial how to start a new transaction in the same session context for a new query, thus let's just not fuzz queries with implicit transactions.
Changelog category (leave one):
Changelog entry (a [user-readable short description]
Do not use server side query fuzzer with implicit transactions because it violates the logic around transactions.
Documentation entry for user-facing changes