ClickHouse / ClickHouse/ClickHouse

s3_max_connections and the S3 disk max_connections key have had no effect since 21.4

Open
#118,564 2 comments 0 reactions 1 assignee Claimed by @alexey-milovidov View on GitHub
comp-object-storage external
Dominant language
C++
Stars
49.9k
Forks
9k
Avg merge
21h 32m
Merged PRs (30d)
515

Description

### Company or project name

Duale AI

### Describe the unexpected behaviour

`s3_max_connections` — documented as "The maximum number of connections per server" ([`Settings.cpp#L625-L627`](https://github.com/ClickHouse/ClickHouse/blob/05f500731250ae3c47a6a718356ba03a769526fc/src/Core/Settings.cpp#L625-L627)) — and the S3 disk's `max_connections` key ([`S3AuthSettings.cpp#L20`](https://github.com/ClickHouse/ClickHouse/blob/05f500731250ae3c47a6a718356ba03a769526fc/src/IO/S3AuthSettings.cpp#L20)) have had no effect since 21.4. Both are still accepted, and `system.settings` still echoes the first one back.

Both write `Aws::Client::ClientConfiguration::maxConnections`, which nothing in ClickHouse reads:

- [`diskSettings.cpp#L171`](https://github.com/ClickHouse/ClickHouse/blob/05f500731250ae3c47a6a718356ba03a769526fc/src/Disks/DiskObjectStorage/ObjectStorages/S3/diskSettings.cpp#L171) — from the disk key
- [`BackupIO_S3.cpp#L222`](https://github.com/ClickHouse/ClickHouse/blob/05f500731250ae3c47a6a718356ba03a769526fc/src/Backups/BackupIO_S3.cpp#L222) and [`GlueCatalog.cpp#L166`](https://github.com/ClickHouse/ClickHouse/blob/05f500731250ae3c47a6a718356ba03a769526fc/src/Databases/DataLake/GlueCatalog.cpp#L166) — from `s3_max_connections`
- [`Credentials.cpp#L1112`](https://github.com/ClickHouse/ClickHouse/blob/05f500731250ae3c47a6a718356ba03a769526fc/src/IO/S3/Credentials.cpp#L1112) — hardcoded `2`

Those four writes are every occurrence of `maxConnections` under `src/` on master. The field matters only to the AWS SDK's own HTTP clients, and ClickHouse builds none of them: `ClientFactory` sets `initAndCleanupCurl = false` and installs `PocoHTTPClientFactory` ([`Client.cpp#L1242-L1243`](https://github.com/ClickHouse/ClickHouse/blob/05f500731250ae3c47a6a718356ba03a769526fc/src/IO/S3/Client.cpp#L1242-L1243)) before `Aws::InitAPI`, so every client — including the SDK's internal ones — is a `PocoHTTPClient`, which draws its session from the global `HTTPConnectionPools` group and never consults `maxConnections`.

### Cause: an unfinished revert

- [#13405](https://github.com/ClickHouse/ClickHouse/pull/13405) (21.2) made it work — `PocoHTTPClient::max_connections(clientConfiguration.maxConnections)`, passed to `makePooledHTTPSession`. Changelog: "Use a connection pool for S3 connections, controlled by the `s3_max_connections` settings."
- [#21737](https://github.com/ClickHouse/ClickHouse/pull/21737) ("Reverted S3 connection pools", 2021-03-16) removed the member, returned to `makeHTTPSession`, and deleted the disk key's docs from `mergetree.md`.

The revert stopped there. The setting, its description, the disk key, and the two assignments that existed then (`registerDiskS3.cpp`, plus the hardcoded `2`) all survived, and two more assignments have been added since.

### Why it still matters

Nothing bounds concurrent S3 sessions per endpoint or per disk. The rate knobs next door do work — `s3_max_get_rps` and `s3_max_put_rps` reach `HTTPRequestThrottler` — so an operator can throttle the request rate but not the session count. What remains are the global group limits ([`ServerSettings.cpp#L1212-L1215`](https://github.com/ClickHouse/ClickHouse/blob/05f500731250ae3c47a6a718356ba03a769526fc/src/Core/ServerSettings.cpp#L1212-L1215): `disk_connections_soft_limit` 5000 … `hard_limit` 200000) and the I/O pools, whose defaults sum to 578 threads that can each hold a session.

That matters where a provider caps sessions. Hetzner Object Storage documents ["Up to 256 active parallel (TCP) sessions per source IP"](https://docs.hetzner.com/storage/object-storage/overview/). `s3_max_connections` is the name an operator reaches for; the first warning ClickHouse emits is at 8000.

### Which ClickHouse versions are affected?

21.4 through master. Reading `src/IO/S3/PocoHTTPClient.cpp` per tag: `v21.3.2.5-lts` still has the reader, `v21.4.3.21-stable` does not, and neither do v21.8, v22.3, v22.8, v23.3, v23.8, v24.3, v24.8, v25.3, v26.3 LTS or master. `v21.3.20.1-lts` lacks it too, so the revert was backported into 21.3.

### How to reproduce

Source-reading report: the field has no reader, so no workload behaves differently. `grep -rn maxConnections src/` shows it.

Runtime check, which I have not run — set `1` on an S3 disk, restart, run concurrent reads, then `SELECT metric, value FROM system.metrics WHERE metric LIKE 'DiskConnections%'`. If the key worked, `DiskConnectionsTotal` would stay at 1.

### Expected behavior

Pick one, rather than the present state:

1. **Finish the feature** — cap live sessions per endpoint or per disk in `HTTPConnectionPools`, driven by these two names.
2. **Finish the revert** — drop the four assignments and the dead `extern` in `S3TablesCatalog.cpp#L42`, deprecate both names, and document `disk_connections_*` / `storage_connections_*` as the only connection bounds.

Either way, a setting with no reader should stop advertising "the maximum number of connections per server".

### Related issues and pull requests

Caused by: https://github.com/ClickHouse/ClickHouse/pull/21737
Related: https://github.com/ClickHouse/ClickHouse/pull/13405
Related: https://github.com/ClickHouse/ClickHouse/issues/118563

The last is the same subsystem and compounds this one: `PocoHTTPClient` has no multiplexing, so one in-flight request costs one socket.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.