Skip to content

CH local: Treat localhost:port as a remote database#26736

Merged
akuzm merged 2 commits intoClickHouse:masterfrom
Algunenano:ch_local_remote_localhost
Jul 27, 2021
Merged

CH local: Treat localhost:port as a remote database#26736
akuzm merged 2 commits intoClickHouse:masterfrom
Algunenano:ch_local_remote_localhost

Conversation

@Algunenano
Copy link
Copy Markdown
Member

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Backward Incompatible Change

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Under clickhouse-local, always treat local addresses with a port as remote.

Detailed description / Documentation draft:

Before this change localhost:default_port would be treated as a self-connection, while now it will try to connect to a remote database. Connections to local addresses without port will still be considered local.

For example:
remote('localhost', 'system.tables' -> Self
remote('127.0.0.1', 'system.tables') -> Self
remote('127.0.0.1:9000', 'system.tables') -> External

Note that this only affects clickhouse-local.

I'm considering it a backward incompatible change since there was a test checking the old behaviour.

Closes #26712

@robot-clickhouse robot-clickhouse added the pr-backward-incompatible Pull request with backwards incompatible changes label Jul 23, 2021
@akuzm
Copy link
Copy Markdown
Contributor

akuzm commented Jul 23, 2021

I think we already have logic for this -- all localhost IPs except .1 are treated as remote.

clickhouse-local -q "SELECT * FROM remote('127.0.0.2', 'system.parts') LIMIT 3;"

Is this sufficient?

@Algunenano
Copy link
Copy Markdown
Member Author

Is this sufficient?

What do you think @davidmanzanares? One issue it doesn't cover would be using localhost:9000 or myinternalhost.domain:9000.

@davidmanzanares
Copy link
Copy Markdown

Oh, I didn't know about that.

Yes, for me it is sufficient. Although, personally, I think @Algunenano 's behavior is less error prone.

@Algunenano
Copy link
Copy Markdown
Member Author

Added a commit to ignore a ASAN warning (not an error, it's just saying that it can't handle some things) which is already ignored in other places like stress tests.

@akuzm
Copy link
Copy Markdown
Contributor

akuzm commented Jul 27, 2021

The tests are flaky/broken in master.

@akuzm akuzm merged commit ece7d00 into ClickHouse:master Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backward-incompatible Pull request with backwards incompatible changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

clickhouse-local remote() fails with localhost on default port

4 participants