Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pgwire: lift the PreServe() call to the server package #92578

Merged
merged 16 commits into from
Jan 9, 2023

Conversation

@knz knz self-assigned this Nov 28, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz marked this pull request as ready for review November 28, 2022 15:43
@knz knz requested review from a team November 28, 2022 15:43
@knz knz added the A-multitenancy Related to multi-tenancy label Nov 28, 2022
@knz knz requested a review from rafiss November 28, 2022 16:59
@knz knz requested a review from a team November 29, 2022 14:59
@dhartunian dhartunian removed the request for review from a team November 29, 2022 16:48
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)

@knz knz requested review from srosenberg and smg260 and removed request for a team January 8, 2023 19:51
knz added 15 commits January 8, 2023 23:13
Prior to this patch, we were unconditionally applying a port offset
for secondary tenant servers, even when the port allocation for the KV
node was randomized. This resulted in the demo cluster trying to
reuse arbitrary TCP ports which may be already in use elsewhere in the
system.

This patch fixes it by ensuring that tenant servers also use random
allocation when the KV node ports were randomly allocated in the first
place.

It also adds demo networking tests that verify this via the
`--sql-port` / `--http-port` flags.

Release note: None
Some of the tests were not waiting for the prompt to start sending
their input. This was causing the input to be lost in some cases.

Release note: None
This commit introduces `PreServeConnHandler`, which will
handle the preparation of a SQL connection prior to handing it off to
a specific tenant server.

Release note (ops changes): The count of network bytes sent
to report re-authentication errors to a SQL client is now
reported via the metric `sql.pre_serve.bytesout` (instead of
`sql.bytesout` previously). The count of pre-authentication errors
is now reported via the metric `sql.pre_serve.conn.failures` (instead
of `sql.conn.failures` previously).

Release note (ops changes): The count of new SQL connections
is now also reported on `sql.pre_serve.new_conns`.
Release note (ops change): The bytes read from SQL clients
prior to authentication are now reported via the metric
`sql.pre_serve.bytesin` (instead of `sql.bytesin` previously).
We need the read/parse of the parameters to be tenant-independent,
so the function cannot contain tenant-specific configuration.

Release note: None
This commit extracts the memory reservation into
a tenant-independent memory pool.

Release note: None
This demonstrates that the logic is tenant-independent
and makes it ready to move into the PreServe() function.

Release note: None
@knz knz changed the base branch from 20221116-shared-sql-4 to master January 8, 2023 22:19
craig bot pushed a commit that referenced this pull request Jan 9, 2023
92580: server,pgwire: use a single SQL listener for multiple tenants r=rafiss a=knz

Parent PRs:
- [x] #84608
- [x] #91739
- [x] #91744
- [x] #92574
- [x] #92575
- [x] #92576
- [x] #92577
- [x] #92578
- [x] #92579
- [ ] #94901

Fixes #84585.
Informs  #94310.
Epic: CRDB-14537

This commit implements tenant routing using a single SQL network listener.

The tenant name can be specified:

- via the client status param `options`, e.g. `options=-ccrdb:tenant=hello`
- via the database name, e.g. `crdb:tenant-hello.defaultdb`.

Release note (backward-incompatible change): If a SQL database is created with a name that starts with `crdb:tenant-` (e.g. `CREATE DATABASE "crdb:tenant-foo"`, clients will no longer be able to connect to it directly.

Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
@craig craig bot merged commit 7eb7663 into master Jan 9, 2023
@craig craig bot deleted the 20221116-shared-sql-5 branch January 9, 2023 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants