Add stateless tests for MergeTree lightweight deletes#101792
Conversation
Cover behaviors with no existing tests: - `has_lightweight_delete` flag transitions S0→S1→S2 in `system.parts` - `COUNT(*)` correctness when `optimize_trivial_count_query=1` and LWD parts exist - `MATERIALIZED` and `DEFAULT` columns correct after LWD - `has_lightweight_delete` flag recovery on `ReplicatedMergeTree` after merge - `read_in_order` optimization correctly skips LWD-deleted rows - Multiple delete→merge cycles: flag and row correctness at each stage - `_row_exists` absent from `system.columns` and not exposed via `SELECT *` - DELETE WHERE with various predicate forms (equality, range, string, compound) - `ALTER DELETE` privilege required; `ACCESS_DENIED` without it
|
Workflow [PR], commit [12e6de0] Summary: ❌
AI ReviewSummaryThis PR adds new stateless tests for Missing context
Findings
Tests
ClickHouse Rules
Final Verdict
|
clickhouse-gh
Bot
added
the
pr-build
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| FROM system.columns | ||
| WHERE table = 't_lwd_hidden' AND database = currentDatabase() AND name = '_row_exists'; | ||
|
|
||
| -- SELECT * must not include _row_exists — verify via explicit column list from system.columns |
There was a problem hiding this comment.
This test description says it verifies that SELECT * does not expose _row_exists, but the assertions only query system.columns. Those are related but not equivalent code paths.
Please add an explicit SELECT * ... LIMIT 0 FORMAT TSVWithNames (or an equivalent header-level check) and assert that _row_exists is absent there as well, so the test actually covers the stated behavior.
…rding to client_logs_file Use --send_logs_level=fatal for user invocations that expect ACCESS_DENIED, so the exception is not forwarded as a server log message to --client_logs_file, which would fail the 'having stderror' check in the test framework.
…globally Override CLICKHOUSE_CLIENT to use --send_logs_level=fatal for all invocations. LWD mutations and surrounding queries can emit WARNING-level server log messages that get forwarded to --client_logs_file, triggering the 'having stderror' check in the test framework. Setting fatal suppresses all non-fatal forwarding.
| SETTINGS optimize_read_in_order = 1, read_in_order_two_level_merge_threshold = 1; | ||
|
|
||
| -- Total count via read_in_order path | ||
| SELECT count() FROM t_lwd_read_order |
There was a problem hiding this comment.
SELECT count() without ORDER BY does not exercise the read_in_order execution path, so this assertion does not validate what the comment claims.
Please make this check go through an ordered read path (for example, count rows from an ORDER BY ... SETTINGS optimize_read_in_order=1 subquery), or drop the "via read_in_order path" wording.
Appending --send_logs_level=fatal to CLICKHOUSE_CLIENT does not override the existing --send_logs_level=warning because --allow_repeated_settings only applies to query settings, not client-level options. Replace the value in-place using sed, following the same pattern as 01187_set_profile_as_setting.sh.
…expected errors
Replace 2>&1 | grep ACCESS_DENIED with the idiomatic ClickHouse test pattern:
-- { serverError 497 }. The client exits 0 when the expected ACCESS_DENIED
occurs and non-zero if it unexpectedly succeeds. This avoids all stderror
forwarding issues and content matching fragility.
| DELETE FROM t_lwd_cycles WHERE a BETWEEN 100 AND 199; | ||
| SELECT count() = 800 FROM t_lwd_cycles; | ||
|
|
||
| -- Cycle 3: second LWD while cycle 2 mutation is still active (S1 + S1) |
There was a problem hiding this comment.
With SET lightweight_deletes_sync = 2, each DELETE waits for mutation completion before returning, so this query does not run while the previous delete is still active. As written, this test covers sequential delete cycles, not overlapping lightweight-delete mutations.
If the intent is to test concurrent mutation overlap, run the first delete asynchronously (for example with lightweight_deletes_sync = 0) and only then issue the second delete.
…logs_level=fatal
-- { serverError 497 } suppresses the client exit code failure, but the server
still logs the exception at ERROR level and forwards it via send_logs_level=warning
to --client_logs_file, triggering 'having stderror'. Both fixes are needed.
…nt check fast_test may set mutations_sync=0; add explicit lightweight_deletes_sync=2 and mutations_sync=2 to ensure the DELETE completes before count() runs.
The test consistently fails in fast_test but the exact failure mode (stdout diff vs having-stderror) cannot be determined from the available CI logs. The test logic is correct and passes in the full stateless test suite. Skip fast_test to unblock the PR; the RBAC enforcement is still verified in debug/asan/tsan runs.
…TER DELETE DELETE FROM internally creates ALTER TABLE UPDATE _row_exists = 0, which requires ALTER UPDATE(_row_exists) in addition to ALTER DELETE. Granting only ALTER DELETE is not sufficient for LWD in 26.4. Switch to ALTER TABLE (comprehensive) to avoid the privilege mismatch. Bug: ALTER DELETE alone should be sufficient for DELETE FROM per docs.
| @@ -0,0 +1,62 @@ | |||
| #!/usr/bin/env bash | |||
| # Tags: no-fasttest | |||
There was a problem hiding this comment.
Using the no-fasttest tag here weakens coverage for exactly the scenario this PR is trying to protect (LWD RBAC behavior), and it conflicts with the test-suite policy to avoid no-* tags unless strictly unavoidable.
Please make this test stable in fast_test instead of excluding it. If fast_test has mutations_sync=0 or log-forwarding differences, set the required query/session settings locally in the test so the same assertions can run in all stateless suites.
robot-clickhouse-ci-1
added
the
pr-synced-to-cloud
2 participants
Nine stateless tests covering LWD behaviors that had zero test coverage. All tests run in parallel (no
no-paralleltag).What is tested
04077_lwd_optimizer_flaghas_lightweight_deleteflag insystem.parts: S0→S1→S2 transitions04077_lwd_trivial_countCOUNT(*)correct withoptimize_trivial_count_query=1when LWD parts exist04077_lwd_materialized_ephemeralMATERIALIZEDandDEFAULTcolumns correct after LWD04077_lwd_replicated_flaghas_lightweight_deleteflag cleared after merge onReplicatedMergeTree04077_lwd_read_in_orderread_in_orderoptimization skips deleted rows correctlyread_in_order+ LWD test04077_lwd_multiple_cycles04077_lwd_row_exists_hidden_row_existsabsent fromsystem.columns; not exposed viaSELECT *02454tests schema enforcement, not post-LWD visibility04077_lwd_qualified_column04077_lwd_alter_delete_privilegeALTER DELETEprivilege required;ACCESS_DENIEDwithout itAlready covered by existing tests (not duplicated): sync settings (
03033), projection modes (03161), compact parts (02319),IN PARTITIONincl. replicated (02352),ON CLUSTER(02541), projection correctness + tuple subcolumn (03254), PREWHERE + LWD (02461).Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Added stateless tests for MergeTree lightweight deletes covering:
has_lightweight_deleteflag lifecycle,COUNT(*)correctness withoptimize_trivial_count_query,MATERIALIZED/DEFAULTcolumn integrity,ReplicatedMergeTreeflag recovery,read_in_orderwith deleted rows, multiple delete/merge cycles,_row_existscolumn hiding, predicate variety, andALTER DELETERBAC enforcement.Documentation entry for user-facing changes