alter_replace: fix tests exposed by the 26.6 empty-source guard - #159
Open
CarlosFelipeOR wants to merge 4 commits into
Open
alter_replace: fix tests exposed by the 26.6 empty-source guard#159CarlosFelipeOR wants to merge 4 commits into
CarlosFelipeOR wants to merge 4 commits into
Conversation
26.6 refuses REPLACE PARTITION when the source has no parts in the requested partition. Of the 32 failures it produced, 26 were in tests that had been green while never replacing any data: the validation compares destination to source, so an empty source emptied the destination and empty == empty passed. - concurrent replace partitions: range(1, number_of_parts) ran zero inserts when the random draw was 1, creating partitions with no parts - engines with a SummingMergeTree source: rows were dropped at insert because all summing columns were zero; the insert now fills a summing column - engines with a CollapsingMergeTree source: the sign reused the partition column, so the partition was drawn at random; collapsing engines now take a dedicated Sign column - partition types and data integrity: intentional empty-source tests now expect the refusal on 26.6+ and assert the destination data is kept - partitioned_summing_ and partitioned_aggregating_ wrappers were swapped Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Running all suites to confirm these changes and check for fallout elsewhere: https://github.com/Altinity/clickhouse-regression/actions/runs/31151708532 Worth watching because |
…ssage so query() stops failing on the expected exception Signed-off-by: CarlosFelipeOR <carlosfelipeor@gmail.com>
…ver replace from a destroyed partition Signed-off-by: CarlosFelipeOR <carlosfelipeor@gmail.com>
…artition column as its sign Signed-off-by: CarlosFelipeOR <carlosfelipeor@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix #160
ClickHouse 26.6 refuses
REPLACE PARTITIONwhen the source has no parts in the requested partition. That broke 32 tests inalter_replaceplus theconcurrent actionsscenario — and 26 of the 32 turned out to be tests that had been green while never replacing any data.Why they were green before
Every scenario validates with
check_partition_was_replaced, which asserts:Until 26.5, replacing from an empty source silently emptied the destination partition. The assertion then compared empty to empty and passed. It could not distinguish "replaced successfully" from "both are empty". The guard did not break these tests — it revealed they were not testing anything.
Upstream context: ClickHouse#23727, open since 2021, classified the old behaviour as silent data loss. The previous behaviour is still available via
allow_replace_partition_from_empty_source = 1.Changes
1.
concurrent replace partitions— off-by-one (13 failures)for parts in range(1, number_of_parts)withnumber_of_parts = random.randrange(1, 50)runs zero inserts when the draw is1, so ~2% of partitions were created with no parts at all. Measured in the failing run: 96 of 99 partitions created, and every partition that failed had zero inserts logged.2.
engineswith aSummingMergeTreesource (8 failures)SummingMergeTreedrops rows whose summing columns are all zero. The shared insert only fillspandi, which are the partition and sorting keys and therefore not summed, so every row was discarded and the source ended up completely empty. Verified on 26.6.2:INSERT (p, i)→ 0 rows,INSERT (p, i, Value)→ 3 rows.This became visible when #111 moved these tables from
ORDER BY tuple()toORDER BY i, which tookiout of the summing set. Same pattern as #129.3.
engineswith aCollapsingMergeTreesource (5 failures)The helper reused the partition column as the sign column (
sign="p"), which forced partitions to{-1, 1}and one partition per table. To keep signs valid, the population drewrandom.choice([-1, 1])as the partition value — so half the time the only partition was-1while the test replaces partition1. All 5 tables that drew-1failed; all 11 that drew1passed.Collapsing engines now take a dedicated
Signcolumn. This freespto hold the same 5 partitions as every other engine, and the sign varies per row, which preserves the intent of the original change without randomising the partition. As a side effect, collapsing becomes structurally possible: withsign="p", rows with opposite signs landed in different partitions and could never collapse.The other callers of these wrappers keep the partition column as the sign, and unifying the population initially broke them: the shared helper inserts
p = 1..N, sop = 2was rejected withCode: 117. Incorrect data: Sign = 2 (must be 1 or -1). When the sign is the partition column, the population now picks the partition value from[-1, 1], which makes the generated SQL byte-for-byte identical to the helper that was removed. Caught by a 26.3 run ofs3/export part.4.
partition typesanddata integrity— intentional empty-source tests (6 failures)These deliberately replace from a source with no parts (
ALTER TABLE ... DROP PARTITION 1, or a partition beyond the source's range). They now expect the refusal on 26.6+ and assert that the destination data is kept; older versions keep the previous expectation.The expectation is matched by the
DB::Exception: Source tableprefix. Matching onlyhas no parts in partitionwas not enough:query()skips its generic "output has exception" check only when the expected message itself containsException:, so the test still failed on the very exception it was asserting.For
data integritythis also resolves a contradiction. The requirement it declares says:The test asserted the opposite — that the destination was emptied. The new ClickHouse behaviour is what the requirement always asked for.
5. Swapped engine wrappers
partitioned_summing_merge_tree_tablecalledcreate_aggregating_merge_tree_tableand vice versa. Both engines were exercised, but under each other's names, so the reported engine in every combination involving them was wrong.6.
concurrent actions— the partition to replace was drawn blindlyThe scenario deliberately destroys partitions (
DROP,DETACH,MOVE) over 100 iterations, but drewpartition_to_replacefromrandom.randrange(1, 500)— the range the tables were created with, not the partitions that survived. Once a draw landed on a destroyed partition the guard fired, and the surroundingretries(timeout=60)could not recover: the draw sat outside the retry, so every attempt asked for the same partition, and that partition never comes back.This is accumulated state, not a race. 128 of the 147 guard hits in the run were on partition 12, which iteration 2 had dropped 195s earlier.
The partition now comes from
system.partsand is drawn inside the retry, so a partition removed by a concurrent action mid-attempt is simply reconsidered on the next attempt.This corrects an earlier assessment in this PR that
concurrent actionsneeded no change. That was based on a single job log which happened to be the one that passed —replace partition along other actionsfailed in 51 of the 53 results recorded in the CI database.Decisions
create_partitions_with_random_uint64takes an optionalextra_columns. With the default the generated SQL is byte-for-byte unchanged, so the ~41 other callers are unaffected.sign="p"/version="i"as defaults, so the s3 and parquet callers keep the schema they had; only the engines test passes a dedicated sign column. Keeping the default was not enough on its own — see the note in change 3.allow_replace_partition_from_empty_source = 1anywhere. It would have turned everything green while leaving 26 tests asserting a behaviour upstream fixed as a data-loss bug.concurrent_actions.pywere left alone on purpose.replace_partition_with_single_concurrent_actionhas the same flaw, but the feature only runsone_replace_partitionandreplace_partition_along_other_actions, so it is dead code.replace_partition_from_another_tabledraws blindly too, but from inside the action, so each retry re-draws and it self-heals — and none of the 147 guard hits name the destination table as the source, which is what those actions would use.Not covered
RQ.SRS-032.ClickHouse.Alter.Table.ReplacePartition.Concurrent.Manipulating.Partitions.Droprequires thatDROP PARTITIONwaits for aREPLACE PARTITIONon the same partition to finish. This suite cannot exercise it reliably: it needs two independent draws to pick the same partition (~1/500 per iteration) and to overlap in time. Covering it needs a dedicated test that forces the overlap. Tracked in #160.References