Add system.masking_policies introspection in open-source builds#108030
Conversation
Masking policies are available only in ClickHouse Cloud. SHOW MASKING POLICIES rewrote the query into SELECT name FROM system.masking_policies, which does not exist in OSS builds, producing a confusing UNKNOWN_TABLE error. Match the SHOW CREATE MASKING POLICY and CREATE MASKING POLICY paths, which throw SUPPORT_IS_DISABLED. Closes: #101116 Related: #102785 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Workflow [PR], commit [d8039c9] Summary: ✅
AI ReviewSummaryThis PR adds Final VerdictStatus: ✅ Approve |
|
Instead, let's do it entirely differently. We should have a So the query will not throw an exception. |
Per review on the PR: rather than making SHOW MASKING POLICIES throw a cloud-only error, add the system.masking_policies table and introspection to open-source builds (using the existing SHOW MASKING POLICIES grant), so the query returns an empty result. Masking policies can still only be created, altered, dropped and applied in ClickHouse Cloud (the manipulation interpreters remain cloud-only and throw SUPPORT_IS_DISABLED), so in open-source builds the table is always empty. The new system table does not reference the cloud-only MaskingPolicy entity, so it links in open-source builds; Cloud populates the same table. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Following review, the new open-source `system.masking_policies` table exposed a column named `expression` (non-nullable `String`), but the existing public/documented schema and the `CREATE MASKING POLICY` parser call this column `update_assignments` (`Nullable(String)`). Because the goal is for ClickHouse Cloud to populate the same table, the open-source and Cloud schemas must match, so rename the column to `update_assignments` and make it `Nullable(String)`, matching the autogenerated documentation in `docs/en/operations/system-tables/masking_policies.md`. Update the stateless test `04405_masking_policies_introspection` to select the documented column. Also update that docs page: the table is now present (and always empty) in open-source builds, so the `CloudOnlyBadge` no longer applies. Remove it and describe the open-source behavior, while noting that masking policies can still only be created and applied in ClickHouse Cloud. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
139adb8 to
83834ed
Compare
`DROP MASKING POLICY` was routed through the generic `InterpreterDropAccessEntityQuery`, so in open-source builds it did not follow the same cloud-only contract as the other manipulation statements: `DROP MASKING POLICY IF EXISTS ...` silently no-op'd via `tryRemove`, while the non-`IF EXISTS` form reported a confusing `UNKNOWN_MASKING_POLICY` error from the always-empty open-source access storage, instead of `SUPPORT_IS_DISABLED`. Add an open-source guard in `InterpreterDropAccessEntityQuery::execute` that rejects `AccessEntityType::MASKING_POLICY` outright (including the `IF EXISTS` and `ON CLUSTER` forms), mirroring the existing inline guard in `InterpreterShowCreateAccessEntityQuery` and the dedicated `InterpreterCreateMaskingPolicyQuery` used by `CREATE`/`ALTER`. Now `CREATE`, `ALTER` and `DROP MASKING POLICY` all consistently report `SUPPORT_IS_DISABLED` in open-source builds, matching the documented contract. ClickHouse Cloud overrides these guards to provide the real implementation. Extend `04405_masking_policies_introspection` to assert the `ALTER` and both `DROP` forms throw `SUPPORT_IS_DISABLED`. Addresses AI review feedback on #108030 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Merging master added other tests with the 04405 prefix (`04405_geojson_output_properties`, `04405_qbit_int8_distance`, `04405_qbit_length`), so move this test to a free number. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Completes the move from the colliding 04405 prefix to 04489. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`SHOW MASKING POLICIES` is rewritten to `SELECT name FROM system.masking_policies`, which goes through `InterpreterSelectQuery`. With `select_from_system_db_requires_grant` enabled (the default in CI), a user granted only `SHOW MASKING POLICIES` had no implicit `SELECT` on the system table and hit `ACCESS_DENIED`, so the introspection path promised by the PR did not work for restricted users. Map `SHOW_MASKING_POLICIES` to an implicit `SELECT` grant on `system.masking_policies` in `ContextAccess`, mirroring the existing mappings for `system.users`, `system.roles`, `system.row_policies`, `system.settings_profiles`, and `system.quotas`. Add `04490_masking_policies_restricted_user`, which grants only `SHOW MASKING POLICIES` and verifies that both `SHOW MASKING POLICIES` and `SELECT count() FROM system.masking_policies` succeed without an explicit `SELECT` grant on the system table. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@groeneai, the only CI failure here is unrelated to this PR (which only adds the This is a recurring master-wide crash flake: the same |
|
@alexey-milovidov Confirmed: not caused by this PR. The exact backtrace ( This is the chronic MergeTree merge heap-corruption family (tracking issues #107487, #107488, #106370). The existing fix is your own #107451 plus follow-up #107457 (COW-safe
The instrumented twin (STID 2046-4f7a) shows the bug is single-threaded on I am not opening a separate speculative PR. The obvious guard (a use_count-aware mutate at the |
|
The only CI failure on the latest commit ( This PR only adds the It is a chronic flake. Over the last 90 days the CIDB shows it failing on unrelated PRs on different sanitizer builds:
The failure is a "return code" failure (the test process exits non-zero), not a deterministic reference mismatch: the captured output diverges in the All other checks are green (every build, the other stateless shards, all three flaky-check shards, stress, BuzzHouse, AST fuzzer, integration and performance jobs), there are no unresolved review threads, and the AI review verdict is Approve. @groeneai, could you investigate this |
|
Re-ran the only failing check ( I re-reviewed the diff this run and it is sound and complete: the new |
|
Confirmed ours, and it is not related to the masking-policies change. The two thrown from the distributed The third run you cited, #105712 ( 0 failures on master in 30d. We are tracking #108854 to verify across the affected distributed tests. |
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 41/42 (97.62%) · Uncovered code |
|
Resolved the
The sync PR is now |
|
📊 Cloud Performance Report 🔴 AI verdict: This PR only adds the system.masking_policies introspection table and its implicit SELECT grant (and rejects DROP MASKING POLICY in open-source builds) — it does not touch any query-execution code. The two ClickBench deltas (Q30 +18.75%, Q31 +16.05%) and the TPC-H Q16 -50% 'improvement' are off-path and have been downgraded to run-to-run variance. The three remaining TPC-H join regressions (Q4 +220%, Q7 +797%, Q8 +29%) are large enough to stand on the numbers alone, but since the diff cannot plausibly cause them, verify the benchmark build and environment before treating them as a real effect of this change. clickbenchFlagged queries (2 of 43)
q-value = BH-FDR adjusted p; smaller is stronger evidence. MIRAI flags a query when q < fdr_q (default 0.10) — the value the verdict is based on. tpch_adapted_1_official🔴 3 regressed · Flagged queries (4 of 22)
q-value = BH-FDR adjusted p; smaller is stronger evidence. MIRAI flags a query when q < fdr_q (default 0.10) — the value the verdict is based on. Debug info
|
|
Fixed the Root cause: the sync run went red across ~12 private stateless shards (plus the flaky and stress jobs) on The earlier reconciliation only added the test to Fix: added The other reds on the sync run were unrelated known flakes: |
|
The The four remaining reds are all chronic private/Cloud flakes, none touching the masking-policies code (this PR only adds the
I re-ran the four failed private jobs to clear the red. Public CI is fully green (0 failures), the AI review is ✅ Approve, and there are no unresolved review threads. |
SHOW MASKING POLICIESrewrote the query intoSELECT name FROM system.masking_policies, but that system table did not exist in open-source builds, so the statement failed with a confusingUNKNOWN_TABLEerror.Following review feedback, instead of making
SHOW MASKING POLICIESthrow a cloud-only error, this adds thesystem.masking_policiestable and introspection to open-source builds (reusing the existingSHOW MASKING POLICIESgrant), so the query returns an empty result. Masking policies can still only be created, altered, dropped and applied in ClickHouse Cloud — the manipulation interpreters remain cloud-only and continue to throwSUPPORT_IS_DISABLED. In open-source builds the table is therefore always empty (the new system table intentionally does not depend on the cloud-onlyMaskingPolicyentity implementation, so it links; ClickHouse Cloud populates the same table).system.masking_policiessystem table.SHOW MASKING POLICIESreturns an empty result instead of erroring.CREATE/ALTER/DROP MASKING POLICYremain cloud-only (SUPPORT_IS_DISABLED).Closes: #101116
Related: #102785
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Added the
system.masking_policiestable andSHOW MASKING POLICIESintrospection to open-source builds. Masking policies themselves remain a ClickHouse Cloud feature, so the table is empty in open-source builds, but introspection queries no longer fail with an error.Documentation entry for user-facing changes
Version info
26.7.1.383(included in26.7and later)