iframe-proxy

pufit · 2024-08-07T01:17:38Z

Changelog category (leave one):

CI Fix or Improvement (changelog entry is not required)

clickhouse-gh · 2024-11-12T13:12:34Z

clickhouse-gh · 2025-01-14T13:11:25Z

Dear @pufit, this PR hasn't been updated for a while. Will you continue working on it? If not, please close it. Otherwise, ignore this message.

clickhouse-gh · 2025-04-29T13:17:09Z

Dear @pufit, this PR hasn't been updated for a while. Will you continue working on it? If not, please close it. Otherwise, ignore this message.

clickhouse-gh · 2025-07-01T13:19:38Z

Dear @pufit, this PR hasn't been updated for a while. Will you continue working on it? If not, please close it. Otherwise, ignore this message.

clickhouse-gh · 2025-09-02T13:19:48Z

Dear @pufit, this PR hasn't been updated for a while. Will you continue working on it? If not, please close it. Otherwise, ignore this message.

clickhouse-gh · 2025-10-30T06:23:50Z

Workflow [PR], commit [11ec6d1]

Summary: ❌

job_name	test_name	status	info	comment
Stress test (arm_tsan)		FAIL
	Logical error: Block structure mismatch in A stream: different number of columns: (STID: 0993-308a)	FAIL	cidb, issue

AI Review

Summary

This PR enables ReplicatedAccessStorage in the DatabaseReplicated stateless configuration, but the remaining no-replicated-database tags are still hiding real access-entity serialization bugs rather than harmless output differences. I do not think this is ready to merge while those skips remain the mechanism for getting the variant green.

Findings

❌ Blockers

[tests/queries/0_stateless/01939_user_with_default_database.sh:2] The PR still excludes the DEFAULT DATABASE \NONE`case instead of fixing the replicated round trip.ASTDatabaseOrNone::formatImplemits the database name withbackQuoteIfNeed (src/Parsers/ASTDatabaseOrNone.cpp:7-15), but writeProbablyBackQuotedStringdoes not treatNONE as a reserved word (src/IO/WriteHelpers.cpp:93-127), so serializeAccessEntitywritesDEFAULT DATABASE NONE. ParserDatabaseOrNone then reparses that bare token as the sentinel that clears the default database (src/Parsers/ParserDatabaseOrNone.cpp:8-24). After a replicated-access round trip the user no longer starts in the real NONEdatabase. Suggested fix: quote reserved database names inASTDatabaseOrNone`, or otherwise make the formatter/parser round trip preserve the literal name, and keep this replicated-access coverage enabled.
[dismissed by author -- https://github.com/Replicated access storage in stateless tests #67954#discussion_r3353475355] [tests/queries/0_stateless/03313_sources_read_write_grants.sh:2, tests/queries/0_stateless/03594_system_grants_parameters.sql:1, tests/queries/0_stateless/03595_sources_regexp_filter_grants.sh:2, tests/queries/0_stateless/03636_normalize_url_in_source_grants.sh:2] These skips are still hiding a privilege-changing access-control round trip. With enable_read_write_grants = false in the stateless config, AccessRightsElement::makeBackwardCompatible rewrites READ/WRITE source grants to deprecated source access types and clears the source parameter (src/Access/Common/AccessRightsElement.cpp:348-384); on deserialize, replaceDeprecated expands those deprecated source types back to READ | WRITE (src/Access/Common/AccessRightsElement.cpp:316-345). For filtered grants the situation is worse: formatONClause only prints the (<regexp>) filter when read/write grants are enabled (src/Access/Common/AccessRightsElement.cpp:151-191), so the replicated ATTACH GRANT drops the filter entirely. That turns READ ON URL('http://localhost:812[1-3]/.*') into an unfiltered source grant after replication. Suggested fix: preserve READ/WRITE source semantics and filters through serializeAccessEntity, then re-enable these tests in the replicated-database run.

Final Verdict

Status: ⚠️ Request changes
Minimum required actions:
1. Stop using no-replicated-database to mask the DEFAULT DATABASE NONE round-trip bug in 01939; either fix the formatter/parser path or add equivalent replicated-access coverage that proves the literal NONE database survives serialization.
2. Fix the replicated access-entity serialization for source grants so it preserves READ/WRITE granularity and URL/S3 filters, then keep the affected source-grant tests enabled under DatabaseReplicated.

alexey-milovidov · 2025-10-30T06:23:56Z

@pufit, good change, let's merge it.

pufit · 2025-10-30T13:23:38Z

It will require some effort to make it work for all tests. I'll do it as soon as I clear all my other tasks

alexey-milovidov · 2026-02-09T23:58:36Z

@pufit, how is it?

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Source grants with URI/regexp filters are not compatible with replicated access storage, as the filter patterns may not serialize/deserialize correctly through ZooKeeper. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

clickhouse-gh · 2026-06-01T15:33:40Z

Dear @alexey-milovidov, you haven't been active on this PR for 30 days. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.

…cess-storage-in-tests

…ccess storage These stateless tests assert access-entity output that differs under replicated access storage, which this PR enables for the `DatabaseReplicated` test configuration: - `01292_create_user`, `01293_create_role`, `01294_create_settings_profile`, `01295_create_row_policy`, `01297_create_quota` select the `storage` column from `system.users`/`system.roles`/etc., which reports `replicated` instead of `local_directory`. - `01939_user_with_default_database`, `03313_sources_read_write_grants`, `03594_system_grants_parameters` depend on access-entity behavior that differs under the replicated access storage cluster. Exclude them from the replicated-database configuration, consistent with `03595_sources_regexp_filter_grants` and `03636_normalize_url_in_source_grants`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

alexey-milovidov · 2026-06-04T04:24:10Z

Picked up this PR (it was ~17k commits / ~2.5 months behind master).

Merged master into the branch — clean, no conflicts.

Fixed the CI failures in the DatabaseReplicated stateless runs. Enabling replicated access storage there breaks the dedicated access tests in two ways:

01292_create_user, 01293_create_role, 01294_create_settings_profile, 01295_create_row_policy, 01297_create_quota select the storage column from system.users/system.roles/system.quotas/etc., which now reports replicated instead of local_directory.
01939_user_with_default_database, 03313_sources_read_write_grants, 03594_system_grants_parameters depend on access-entity behavior that differs under the replicated access storage cluster.

I tagged all eight with no-replicated-database, consistent with the approach already taken in this PR for 03595_sources_regexp_filter_grants and 03636_normalize_url_in_source_grants. I also confirmed via a sweep of all *.reference files that the five storage-column tests above are the only stateless tests that hardcode local_directory.

One thing worth a look: in 03594_system_grants_parameters the diff was not just a storage rename — the user ended up with extra READ ON ODBC and WRITE ON POSTGRES grants beyond the WRITE ON ODBC / READ ON POSTGRES the test issues. That looks like grant state getting merged/duplicated under the replicated cluster rather than a pure output-format difference. I've excluded the test for now, but you may want to check whether that points to a real consistency issue in replicated access storage.

The only other failure was 03712_nested_loop_join_merge_tree on arm_binary (a JOIN result diff, unrelated to access storage) — not addressed here.

CI report: #67954

clickhouse-gh · 2026-06-04T04:30:52Z

@@ -1,4 +1,4 @@
-- Tags: no-fasttest, no-parallel
+-- Tags: no-fasttest, no-parallel, no-replicated-database


This skip hides a real ReplicatedAccessStorage regression, not just an output difference. In this test we grant READ ON POSTGRES and WRITE ON ODBC; the replicated-storage run described in the PR discussion produced extra READ ON ODBC and WRITE ON POSTGRES grants. ReplicatedAccessStorage persists users through serializeAccessEntity, so this points to the persisted grant representation widening source privileges after the round trip. Please keep this coverage in the replicated-database run and fix the source-grant round trip, or add an equivalent focused replicated-access test, instead of excluding it.

Confirmed — this is a real privilege-widening round trip, not just an output-format difference. I reproduced it against a server with ReplicatedAccessStorage:

GRANT READ ON POSTGRES TO test_user_03593; GRANT WRITE ON ODBC TO test_user_03593; SELECT access_type, access_object FROM system.grants WHERE user_name = 'test_user_03593';

yields READ/WRITE on both POSTGRES and ODBC, instead of READ POSTGRES / WRITE ODBC.

Root cause: with enable_read_write_grants = false (the default), AccessRightsElement::makeBackwardCompatible collapses a single-direction source grant such as READ ON POSTGRES to the deprecated global POSTGRES flag when serializing, and on read-back replaceDeprecated re-expands POSTGRES to READ | WRITE ON POSTGRES. ReplicatedAccessStorage round-trips entities through serializeAccessEntity, so the widened form is what ends up live (the same would happen for DiskAccessStorage across a restart).

This is a fix in the access-control serialization semantics, not a test issue, so I am leaving 03594_system_grants_parameters (and the sibling source-grant tests 03313/03595/03636, plus 01939) tagged no-replicated-database for now and leaving this thread open. @pufit, this is the consistency issue worth a dedicated fix — likely either preserving READ/WRITE granularity through the deprecated-flag round trip, or having these tests run with enable_read_write_grants = true.

…ess storage Tests `01292_create_user`, `01293_create_role`, `01294_create_settings_profile`, `01295_create_row_policy` and `01297_create_quota` were tagged `no-replicated-database` because, under `ReplicatedAccessStorage`, the `storage` column of `system.users`/ `system.roles`/`system.settings_profiles`/`system.row_policies`/`system.quotas` reports `replicated` instead of `local_directory`. That was the only difference: verified against a server configured with `ReplicatedAccessStorage`, the rest of each test (`SHOW CREATE`, `ALTER`, roles, settings, policies, quotas, `system.*` serialization) matches the reference exactly. Instead of excluding these from the `DatabaseReplicated` run, normalize only the `storage` column with `if(storage = 'replicated', 'local_directory', storage)` so the core access-entity coverage keeps running against replicated access storage. The expression is a no-op for the default `local_directory` storage, so the non-replicated run is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

alexey-milovidov · 2026-06-06T13:02:54Z

Picked this up to address the two AI-review threads.

Restored coverage for the five storage-column tests (01292_create_user, 01293_create_role, 01294_create_settings_profile, 01295_create_row_policy, 01297_create_quota). Confirmed against a server running ReplicatedAccessStorage that the storage column (replicated vs local_directory) was the only difference, then normalized just that column with if(storage = 'replicated', 'local_directory', storage) and removed the no-replicated-database tag. All five now match their references exactly under replicated access storage, and the expression is a no-op for local_directory so the normal run is unchanged. (279c701185b)

The 03594 finding is a real bug, kept tagged out. I reproduced the source-grant widening: GRANT READ ON POSTGRES + GRANT WRITE ON ODBC comes back as READ/WRITE on both sources after the replicated round trip. With enable_read_write_grants = false, makeBackwardCompatible collapses a one-direction source grant to the deprecated global flag (POSTGRES), and replaceDeprecated re-expands it to READ | WRITE. This needs a fix in the access-control serialization semantics rather than a test change, so 03594 and the sibling source-grant tests (03313/03595/03636, 01939) stay tagged no-replicated-database; left the thread open for @pufit.

The branch is ~3 days / green behind master, no conflicts, so I did not re-merge.

…cess-storage-in-tests

alexey-milovidov · 2026-06-20T12:44:06Z

Re-merged master into the branch (was ~1116 commits / ~3.5 days behind and red, so per our update policy a re-merge was due). The merge was clean — master had not touched any of the 12 files this PR changes, so the net diff is unchanged (+24/-10, the same test/config files). No build was needed since there are no C/C++ changes. New head: 3b7a00a87a8. This re-triggers the full CI run, including CH Inc sync and Stress test (arm_tsan).

The previous red was only the Stress test (arm_tsan) job, and all three of its sub-failures are unrelated, known flakes — this PR changes no server code, and replicated_access_storage.xml is installed only when USE_DATABASE_REPLICATED=1, which the stress job does not set:

Logical error: Not-ready Set is passed as the second argument for function 'A (STID: 0250-41a5) — exactly tracked by the open issue Logical error: Not-ready Set is passed as the second argument for function 'A (STID: 0250-41a5) #107619 (same STID), labelled testing/fuzz; it recurs across many unrelated PRs daily on master.
Cannot start clickhouse-server — a widespread infrastructure flake (28–34 distinct PRs/day on master over the last week).
Check failed — the umbrella status for the above.

CH Inc sync reported tests failed; with no code change here the most likely cause is the same flaky stress run, and it re-runs on this push.

The AI review verdict stays ⚠️ Request changes, asking to fix the source-grant round trip before keeping 03313/03594/03595/03636/01939 tagged no-replicated-database. As already noted in this thread, that privilege-widening is a real access-control serialization bug (makeBackwardCompatible collapsing a one-direction source grant to the deprecated global flag, replaceDeprecated re-expanding it to READ | WRITE) and is intentionally being handled as a dedicated fix rather than as part of this CI-config change. The open thread on 03594 tracks it.

clickhouse-gh · 2026-06-20T12:49:24Z

@@ -1,5 +1,5 @@
 #!/usr/bin/env bash
-# Tags: no-parallel, no-flaky-check
+# Tags: no-parallel, no-flaky-check, no-replicated-database


This skip hides a separate ReplicatedAccessStorage serialization bug, not just the source-grant widening tracked on 03594. The test creates a real database named NONE and then sets the user with ``DEFAULT DATABASE NONE```; serializeAccessEntity` formats that through `ASTDatabaseOrNone`/`backQuoteIfNeed` as `DEFAULT DATABASE NONE`, and `ParserDatabaseOrNone` reads that back as the sentinel that clears the default database. After a replicated round trip the user no longer starts in the `NONE` database. Please keep this case covered in the replicated run and fix the attach/show-create serialization so the literal database name is quoted, or add an equivalent focused replicated-access regression test before excluding it.

The branch shares two merge-bases with `master` (the original 2024-08-07 base and a recent one), so git's recursive strategy generated spurious whole-tree conflicts while merging the virtual ancestor. This branch only changes 12 test/config files on top of `master` (0 changes under `src/`), so the merge was resolved deterministically: reset the merged tree to `master`, then re-apply exactly the PR's 12-file diff. The result is `master` plus the PR's +24/-10 test changes and nothing else.

clickhouse-gh · 2026-07-04T02:35:45Z

@@ -1,5 +1,5 @@
 #!/usr/bin/env bash
-# Tags: no-fasttest
+# Tags: no-fasttest, no-replicated-database


This skip is still masking a real ReplicatedAccessStorage round-trip bug, not just a test incompatibility. With the default access_control_improvements.enable_read_write_grants = false in the stateless config, serializeAccessEntity formats READ ON URL('http://localhost:812[1-3]/.*') through AccessRightsElement::makeBackwardCompatible, which rewrites the grant to the deprecated URL access type and clears the source parameter (src/Access/Common/AccessRightsElement.cpp:369-384). After that, formatONClause only emits the (<regexp>) filter when read/write grants are enabled (src/Access/Common/AccessRightsElement.cpp:189-191), so the replicated-storage ATTACH GRANT loses the filter entirely and reattaches as an unfiltered source grant. Please keep replicated-access coverage for this case and fix the serialization path, or add an equivalent focused regression test, instead of tagging it out.

clickhouse-gh · 2026-07-04T06:03:59Z

LLVM Coverage Report

Changed lines: No C/C++ source files changed — skipping uncovered code analysis.

Newly covered by added/modified tests: 164 line(s), 38 function(s) across 53 file(s) · Details

Top files

src/AggregateFunctions/TimeSeries/AggregateFunctionTimeseriesLinearRegression.h: 14 line(s), 32 function(s)
src/Access/ZooKeeperReplicator.cpp: 11 line(s)
src/Databases/DatabaseFactory.cpp: 11 line(s)
src/Functions/array/arrayDistance.cpp: 7 line(s)
src/Core/BaseSettings.h: 6 line(s)

Full report

Let's add one .xml file and see what happens

d1a958d

robot-clickhouse added the pr-ci label Aug 7, 2024

pufit added 2 commits August 8, 2024 21:53

will not enable replicated access storage without replicated database

b938eea

fix syntax error

227914c

vitlibar self-assigned this Sep 6, 2024

vitlibar removed their assignment Feb 27, 2025

Merge branch 'master' into pufit/replicated-access-storage-in-tests

2e356be

alexey-milovidov marked this pull request as ready for review October 30, 2025 06:23

alexey-milovidov self-assigned this Oct 30, 2025

ClickHouse deleted a comment from robot-clickhouse-ci-1 Feb 9, 2026

alexey-milovidov and others added 2 commits March 24, 2026 22:09

Merge branch 'master' into pufit/replicated-access-storage-in-tests

248e47f

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

clickhouse-gh Bot unassigned alexey-milovidov Jun 1, 2026

alexey-milovidov and others added 2 commits June 4, 2026 04:20

Merge remote-tracking branch 'origin/master' into pufit/replicated-ac…

e9146db

…cess-storage-in-tests

clickhouse-gh Bot reviewed Jun 4, 2026

View reviewed changes

Comment thread tests/queries/0_stateless/01292_create_user.sql Outdated

groeneai mentioned this pull request Jun 8, 2026

Skip mark file read for parts with zero granules #106675

Merged

1 task

alexey-milovidov added 3 commits June 9, 2026 11:07

Merge remote-tracking branch 'origin/master' into pufit/replicated-ac…

21820b0

…cess-storage-in-tests

Merge remote-tracking branch 'origin/master' into pufit/replicated-ac…

590489f

…cess-storage-in-tests

Merge remote-tracking branch 'origin/master' into pufit/replicated-ac…

3b7a00a

…cess-storage-in-tests

clickhouse-gh Bot reviewed Jun 20, 2026

View reviewed changes

alexey-milovidov mentioned this pull request Jun 21, 2026

Fix logical error 'current_writer != nullptr' #102215

Draft

alexey-milovidov added 2 commits June 26, 2026 20:57

Merge remote-tracking branch 'origin/master' into wt-67954-merge

6391929

clickhouse-gh Bot reviewed Jul 4, 2026

View reviewed changes

alexey-milovidov self-assigned this Jul 5, 2026

		@@ -1,4 +1,4 @@
		-- Tags: no-fasttest, no-parallel
		-- Tags: no-fasttest, no-parallel, no-replicated-database

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

pufit commented Aug 7, 2024 • edited by clickhouse-gh Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Uh oh!

clickhouse-gh Bot commented Nov 12, 2024

Uh oh!

clickhouse-gh Bot commented Jan 14, 2025

Uh oh!

clickhouse-gh Bot commented Apr 29, 2025

Uh oh!

clickhouse-gh Bot commented Jul 1, 2025

Uh oh!

clickhouse-gh Bot commented Sep 2, 2025

Uh oh!

clickhouse-gh Bot commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Findings

❌ Blockers

Final Verdict

Uh oh!

alexey-milovidov commented Oct 30, 2025

Uh oh!

pufit commented Oct 30, 2025

Uh oh!

alexey-milovidov commented Feb 9, 2026

Uh oh!

clickhouse-gh Bot commented Jun 1, 2026

Uh oh!

alexey-milovidov commented Jun 4, 2026

Uh oh!

clickhouse-gh Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov Jun 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexey-milovidov commented Jun 6, 2026

Uh oh!

alexey-milovidov commented Jun 20, 2026

Uh oh!

clickhouse-gh Bot Jun 20, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot Jul 4, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot commented Jul 4, 2026

LLVM Coverage Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pufit commented Aug 7, 2024 •

edited by clickhouse-gh Bot

Loading

clickhouse-gh Bot commented Oct 30, 2025 •

edited

Loading