Skip to content

fix(clickhouse): probe views with SELECT * so a view broken by ALTER is dropped - #23

Merged
alexey-milovidov merged 1 commit into
ClickHouse:mainfrom
groeneai:fix-clickhouse-view-probe-reads-all-columns
Nov 1, 2026
Merged

alexey-milovidov merged 1 commit into
ClickHouse:mainfrom
groeneai:fix-clickhouse-view-probe-reads-all-columns

Conversation

@groeneai

Copy link
Copy Markdown

The 2026-10-13 master nightly (SQLancer (arm_release), reproducer) reported TLPWhere / Code: 47 UNKNOWN_IDENTIFIER on SELECT * FROM v7, t0. The generator created v7 AS SELECT * FROM t1 and then ran ALTER TABLE t1 DROP COLUMN IF EXISTS c2. A plain view keeps the columns it was created with (ClickHouse/ClickHouse#19087), so the view can no longer be read. RENAME COLUMN breaks it the same way.

SQLProviderAdapter.checkViewsAreValid already runs after database generation and drops views it cannot read, but it probes with SELECT 1 FROM <view> LIMIT 1. ClickHouse prunes the view's unused columns for that query, so the probe passes on the broken SELECT * view and the view stays in the schema. This PR makes the probe query overridable (the default stays the same for the other providers) and uses SELECT * FROM <view> LIMIT 0 for ClickHouse, which resolves every column without reading data. A view that fails is dropped through the existing logged DROP VIEW path, which also frees its slot under the 3-view cap. That is option (b) from #22 (comment). ClickHouseSchema.fromConnection does not need to change.

Checked: mvn verify -DskipTests=true passes. I ran against master 26.10 (debug build, with the .claude/clickhouse-config/ overrides as in the nightly), 6 threads, 8 min per run. For the A/B runs I added a local generator bias that is not in this PR: every database gets 2-3 SELECT * views and 1-3 DROP/RENAME COLUMN alters. Those runs used --oracle TLPWhere --num-queries 30:

databases TLPWhere Code 47 findings
before 452 58
with this PR 321 0

Without the bias, the 100 curated oracles (excluding TextIndexDirectRead, as the nightly does) ran with this PR over 191 databases with 0 reproducers. A healthy SELECT * view still passes the probe after ADD COLUMN, MODIFY COLUMN, COMMENT COLUMN, ADD INDEX and ADD PROJECTION on its table.

Not changed here: in the biased runs, a mutation generated after the ALTER can still read the broken view during database generation, before the check runs (DELETE ... WHERE c IN (SELECT ... FROM v<n>), 2 occurrences). That shape has not appeared in CI in the last 90 days.

🤖 Generated with Claude Code

…is dropped

A plain ClickHouse view keeps the column list it was created with. When the
generator creates `v<n> AS SELECT * FROM t<k>` and then drops or renames a
column of `t<k>`, every read of the view that needs its columns fails with
`UNKNOWN_IDENTIFIER` (ClickHouse/ClickHouse#19087: intended). The 2026-10-13
master nightly reported exactly that through `TLPWhere` on
`SELECT * FROM v7, t0`.

`SQLProviderAdapter.checkViewsAreValid` already runs after database
generation and drops any view it cannot read, but its probe is
`SELECT 1 FROM <view> LIMIT 1`. ClickHouse prunes the unused columns of the
view for that query, so the probe passes and the broken view stays in the
schema for every oracle.

Make the probe query overridable, keeping the default for the other
providers, and use `SELECT * FROM <view> LIMIT 0` for ClickHouse. It resolves
every column of the view without reading data. A view that fails it is
dropped through the existing logged `DROP VIEW` path, which also frees its
slot under the 3-view cap and refreshes the schema.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Author

@fm4v I found another false positive from this provider. It is separate from this PR. It came from the 2026-10-19 master nightly, SQLancer (arm_release) (report, reproducer log):

SELECT toString(tuple(* APPLY(toString))) FROM t0 -> Code: 32 ... Attempt to read after eof: while executing '_CAST(c2, 'Decimal(3, 1)')' (while reading from part .../746276976_14_14_0/)

The generator ran ALTER TABLE t0 MODIFY COLUMN c2 Decimal(3, 1) on a String column that holds '', so the conversion mutation can never finish. This is the MaterializedCte case from #18. #18 makes such a table not stable for repeated reads. But ClickHouseColumnTransformerOracle.pickEligibleTable() picks from getDatabaseTables() with its own isEligibleTable filter (L204-L230), so the check never applies. It is the only finding of this kind since #18 merged: 1 in 18 nightly job runs.

The fix is one line: build pickEligibleTable() from getTablesStableForRepeatedReads(). That also excludes merging engines, which this oracle needs too, because it reads the table twice. Five other oracles also read columns of a table picked from getDatabaseTables() with no mutation check: AggregateStateRoundtrip, DictGetVsJoin, RemoteLocalEquivalence, StatsToggle and WindowEquivalence. None of them has fired yet.

May I send this as a separate PR? Should it change (a) ColumnTransformer only, or (b) also skip tables with an unfinished mutation in those five?

@groeneai

Copy link
Copy Markdown
Author

@fm4v one more provider-side false positive, also separate from this PR: CERT on the 2026-10-22 master nightly, SQLancer (arm_release) (report, reproducer log): 10000 > 9994 for Q1 SELECT t0.c0, t0.c1, t0.c2 FROM t0 WHERE t0.c0 and Q2 ... WHERE ((t0.c0) AND (pow(-2091530733,t0.c0))) AND ((- (t0.c2))).

It has the same one-row shape as the 08-25 finding that #13 could not reproduce (and the 09-25 one). The two estimates were read on two different part layouts:

  • ensureLargeEnough inserted 9993 rows right before Q1, and that insert schedules a merge.
  • Before the merge, t0 has five small parts plus the fill part. Statistics part pruning (auto basic statistics, on by default) drops the four parts whose c0 is only 0, so Q1 estimates 9994.
  • The diagnostics show a single level-1 part all_1_7_1_6 afterwards, so one merge of all six parts happened after the fill, and Q2 was read after it. Nothing on that part can be pruned, so Q2 estimates 10000.

On the same master build (31d10e5ef277) with merges stopped, both queries estimate 9994 on the six parts, and both estimate 10000 after OPTIMIZE TABLE t0 FINAL. Neither layout has Q2 > Q1, so this is not a ClickHouse bug.

The fix I would send: when card2 > card1, check() reads both estimates again, Q2 first and Q1 second, and reports only if Q2 is still larger. A real estimator bug holds on any fixed layout, so it still fires. One method, no server-side change.

May I send this as a PR, together with the ColumnTransformer one from my comment above?

@groeneai

Copy link
Copy Markdown
Author

One more provider-side false positive from the same 2026-10-22 nightly, also separate from this PR: TLPWhere on SQLancer (arm_asan_ubsan), result sets 44 vs 43 (report, reproducer log).

The server is fine here: t1 was being mutated in the background between the oracle's two reads.

  • DELETE FROM t1 WHERE ((c1)==((SELECT c0 FROM t0 WHERE c0 >= 389006002 ORDER BY c0 LIMIT 1))) fails (String/Int32, no supertype) while t0 holds 1024965776. With mutations_sync = 2 the client gets UNFINISHED, an expected error, but mutation_12 stays queued and blocks the later ALTER TABLE t1 DELETE WHERE (- (c0)).
  • The last statement, ALTER TABLE t0 DELETE WHERE ((c0)%(227697357)), empties that subquery. The next background retry then succeeds (c1 = NULL matches nothing), which unblocks mutation_13, and that deletes 8 of the 14 rows while the oracle reads.
  • I replayed the log on master 31d10e5ef277 (the nightly's commit) with mutations_sync = 2. With SYSTEM STOP MERGES t1, both queries return 44 on 5 reads (parts_to_do = 10). After SYSTEM START MERGES, both return 12. One unfrozen replay returned 44 vs 43.

Why #18 misses it: TLPWhere and NoREC do apply isStableForRepeatedReads(), through ClickHouseExpressionGenerator.setTablesAndColumns from #16. This corrects my 2026-10-13 comment on #22, which said they bypass it. But hasUnfinishedMutations is only refreshed when the schema is re-read. ClickHouseMutationGenerator returns its statement with couldAffectSchema = false (L118), and generateAndTestDatabase goes straight from generateDatabase to the oracle loop (L51-L60). So here the flag was last read right after CREATE TABLE t1, before any mutation.

The fix I would send is one line: globalState.updateSchema() at the end of ClickHouseProvider.generateDatabase, so the flag matches the state the oracles read.

May I send it in the same PR as the CERT and ColumnTransformer fixes above? Or would you rather handle it yourself?

@groeneai

Copy link
Copy Markdown
Author

One more provider-side false positive, also separate from this PR, from the 2026-10-28 master nightly, SQLancer (arm_release) (report, reproducer log):

CREATE TABLE t2 (c0 DateTime, c1 Bool, c2 Float64 EPHEMERAL) ENGINE = SummingMergeTree(c2) ORDER BY c0 -> Code: 16 ... Column c2 listed in columns to sum does not exist in table declaration. (NO_SUCH_COLUMN_IN_TABLE)

The server is right. Engine parameter columns are checked against the stored columns (MergingParams::check, getAllPhysical()), and EPHEMERAL and ALIAS columns are not stored. ClickHouseTableGenerator.start() calls pickEngine and renderEngineArgs on the dummy columns, while ClickHouseColumnBuilder.createColumn decides on its own whether a column becomes EPHEMERAL or ALIAS. On master 0525e0c764d1 (the nightly's commit), an unstored parameter column gives:

  • SummingMergeTree(c): the error above
  • CollapsingMergeTree(sign): Sign column ... does not exist in table declaration
  • VersionedCollapsingMergeTree(sign, ver): There is no physical column ... in table
  • ReplacingMergeTree(ver): Version column ..., which is an expected error, so the table is just not created

The first three are not expected errors for CREATE TABLE. The 73 unknown oracle / Code 16 findings of 2026-09-07..13 had the same shape (for example CollapsingMergeTree(c1) with c1 ALIAS c0); their messages were unreadable before #17, so even the Version column case was reported then. Since #17 it is 1 finding in 29 nightly job runs.

The fix I would send: in start(), render the column definitions first, then pick the engine and its parameter columns only from columns rendered without EPHEMERAL/ALIAS. Adding the three messages to the expected errors would also stop the red, but it would drop those tables.

May I add it to the same PR as the three fixes above?

@groeneai

Copy link
Copy Markdown
Author

One more provider-side false positive, also separate from this PR: LimitRanking on the 2026-10-31 master nightly, SQLancer (arm_asan_ubsan) (report, reproducer log). The 2026-10-01 checkNegativeLimitByReversal finding on arm_release (log) is the same case.

LIMIT -5 and the reversed LIMIT 5 differ only in (18446744072471263932,0,0) vs (18446744072471263932,-0,0). t0 has both rows: c1 Float32 is -0.0 in one insert and the default 0 in another. -0 = 0, so the two rows tie on the whole ORDER BY right at the LIMIT boundary and either query may keep either one, but toString prints them differently. The oracle assumes the order is total over the projected strings (L70-L72, L107-L154).

I replayed t0 on master 6983f5a16d92 (the nightly's commit) and ran the pair 108 times (max_threads 1 to 16, max_block_size 1 to 65409, three insert orders). 58 runs differ, always in this row only, and none after mapping -0 to 0, so LIMIT -n itself is correct.

The fix I would send (one file): add the projection as the last sort key in both negative checks (ASC on the ascending side, DESC on the reversed one) and run those queries with SETTINGS optimize_redundant_functions_in_order_by = 0. The setting is needed: the analyzer drops a sort key that is a function of earlier keys (pass), and without it 47 of 108 runs still differ. With it, 0 of 108 differ for LIMIT -n and 0 of 288 for LIMIT -n BY k. Float columns stay in the oracle.

May I send this as a separate PR?

@alexey-milovidov alexey-milovidov self-assigned this Nov 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants