iframe-proxy

bharatnc · 2026-07-03T03:14:53Z

Fix S3 settings priority so a URL-scoped <s3> endpoint block overrides the top-level <s3> defaults.

The <s3> section has two layers: top-level defaults, and URL-scoped endpoint blocks that are more specific and should win. This is done at table creation (fromAST).

<s3>
    <!-- top-level defaults: apply to all S3 access -->
    <use_environment_credentials>false</use_environment_credentials>

    <!-- URL-scoped endpoint block: applies only to matching URLs, more specific -->
    <my_endpoint>
        <endpoint>http://localhost:11111/test/my-bucket</endpoint>
        <use_environment_credentials>true</use_environment_credentials>
    </my_endpoint>
</s3>

After #100975 was merged, S3ObjectStorage::applyNewSettings applied the config_prefix settings last. For disks that is correct, since config_prefix is the disk's own <storage_configuration> section, which is more specific than <s3>. But the same path also runs for tables not using a <storage_configuration> (S3, S3Queue, etc.), where config_prefix is the top-level <s3> itself, so the generic defaults ended up overriding the endpoint block. For example, this turned use_environment_credentials off for specific endpoints on the object-storage refresh, resulting in ACCESS_DENIED errors on flush.

After this fix:

Disks keep the current behavior where a disk's own config wins.
Engine tables restore the behavior where the endpoint-specific config wins over the top-level <s3> defaults, matching how the same settings are already resolved at table creation.

The disk and query/profile priority changes from #100975 are left untouched.

Changelog category (leave one):

Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fix S3 settings priority so a URL-scoped <s3> endpoint block takes precedence over the top-level <s3> defaults.

clickhouse-gh · 2026-07-03T03:15:30Z

Workflow [PR], commit [03676ca]

Summary: ❌

Performance Comparison: Performance dashboard

job_name	test_name	status	info	comment
Finish Workflow		FAIL
	python3 ./ci/jobs/scripts/workflow_hooks/new_tests_check.py	FAIL

AI Review

Summary

This PR fixes the endpoint-vs-top-level precedence bug in S3ObjectStorage::applyNewSettings for engine-backed object storages and adds a regression test for that path. The disk-side behavior looks correct, but the non-disk branch still does not restore query/profile S3 request settings on top of the endpoint block, so the priority contract for ordinary S3 table reads remains incomplete.

Findings

⚠️ Majors

[src/Disks/DiskObjectStorage/ObjectStorages/S3/S3ObjectStorage.cpp:718] StorageObjectStorage::update calls applyNewSettings on each non-table-function query, so the new non-disk branch is also the normal S3 table read path. It now restores global <s3> < endpoint precedence, but it still omits the final request_settings.updateFromSettings(...) that fromAST uses to keep query/profile settings above endpoint config. A query such as SELECT ... SETTINGS s3_max_single_read_retries = 1 against an S3 table will still be overwritten by a matching <s3><endpoint> block before readObject uses s3_settings->request_settings. Suggested fix: after apply_endpoint_settings() in the non-disk branch, re-apply context->getSettingsRef() to modified_settings->request_settings with the same validation flag as fromAST, and add a regression test that exercises a read-side setting on an S3 table.

Tests

⚠️ The new stateless test proves the endpoint-vs-top-level write path with max_single_part_upload_size, but it does not cover the read-side query/profile precedence that applyNewSettings is responsible for on ordinary S3 table queries. A focused SELECT ... SETTINGS s3_max_single_read_retries = ... or throttling-based regression would close this gap.

Final Verdict

Status: ⚠️ Request changes
Minimum required actions: re-apply query/profile S3 request settings after endpoint settings in the non-disk applyNewSettings path, and add a regression test for a read-side S3 table query that would fail without that re-application.

clickhouse-gh · 2026-07-03T03:31:59Z

+        /// top-level `<s3>` section, which is less specific than the URL-scoped endpoint settings.
+        /// Apply it first so the endpoint settings win, matching the resolution order used at table
+        /// creation in `S3StorageParsedArguments::fromAST` (global `<s3>` first, endpoint on top).
+        apply_config_settings();


StorageObjectStorage::update calls applyNewSettings on every non-table-function query, so this branch is the live S3 table path, not only SYSTEM RELOAD CONFIG. The new ordering fixes global <s3> vs endpoint precedence, but it still skips the final request_settings.updateFromSettings(...) that fromAST does in src/Storages/ObjectStorage/S3/Configuration.cpp:667-671. Because S3SettingsByEndpoint::loadFromConfig seeds each endpoint entry from S3RequestSettings(config, settings, "s3") and then applies the endpoint block on top, a query-level read setting like s3_max_single_read_retries or s3_max_get_rps is still overwritten here by the matching <s3><endpoint> config, and readObject later consumes s3_settings->request_settings directly. Can we re-apply context->getSettingsRef() after apply_endpoint_settings() in the non-disk case as well, so the priority order for ordinary S3 table reads remains global <s3> < endpoint block < query/profile`?

clickhouse-gh · 2026-07-03T06:47:45Z

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 15/15 (100.00%) · Uncovered code

Full report · Diff report

fix s3 endpoint configuration priorities

03676ca

clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Jul 3, 2026

bharatnc mentioned this pull request Jul 3, 2026

Revert "Fix S3 settings configuration priorities" #107552

Closed

clickhouse-gh Bot reviewed Jul 3, 2026

View reviewed changes

bharatnc requested a review from azat July 3, 2026 13:43

Sunbelt Computer Software

PL/B Language Development and Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix s3 endpoint specific configuration priorities#109251

fix s3 endpoint specific configuration priorities#109251
bharatnc wants to merge 1 commit into
masterfrom
ncb/fix-s3-setting-priority

bharatnc commented Jul 3, 2026

Uh oh!

clickhouse-gh Bot commented Jul 3, 2026 •

edited

Loading

Uh oh!

clickhouse-gh Bot Jul 3, 2026

Uh oh!

clickhouse-gh Bot commented Jul 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

bharatnc commented Jul 3, 2026

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Uh oh!

clickhouse-gh Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Findings

Tests

Final Verdict

Uh oh!

clickhouse-gh Bot Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot commented Jul 3, 2026

LLVM Coverage Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

clickhouse-gh Bot commented Jul 3, 2026 •

edited

Loading