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.

Sign up for GitHub

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

Jump to bottom

pgwire: unexport ServerMetrics, rename to tenantSpecificMetrics #92574

Merged
merged 2 commits into from
Nov 29, 2022
Merged

pgwire: unexport ServerMetrics, rename to tenantSpecificMetrics #92574

merged 2 commits into from
Nov 29, 2022

Conversation

knz
Copy link
Contributor

@knz knz commented Nov 28, 2022 •
edited
Loading

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

This change is Reviewable

@knz knz added the A-multitenancy Related to multi-tenancy label Nov 28, 2022
@knz knz mentioned this pull request Nov 28, 2022
Merged
4 tasks
@knz knz marked this pull request as ready for review November 28, 2022 15:42
@knz knz requested a review from a team November 28, 2022 15:42
@knz knz requested a review from a team as a code owner November 28, 2022 15:42
rafiss
rafiss reviewed Nov 28, 2022
View reviewed changes
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.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)

pkg/sql/pgwire/server.go line 215 at r1 (raw file):

// Server implements the server side of the PostgreSQL wire protocol for one
// specific tenant (i.e. its configuration is specific to one tenant).

i don't follow why the "tenant specific" name is helpful here. it already seemed clear that this is a "per-connection object" which definitionally is finer-scoped than "tenant specific."

to take this PR's logic further, why not rename Server to TenantSpecificServer? (i don't think we should, just asking rhetorically)

@knz
Copy link
Contributor Author

knz commented Nov 28, 2022

it already seemed clear that this is a "per-connection object" which definitionally is finer-scoped than "tenant specific."

Nope, that is not how I want to frame it.
Connections start in a tenant-agnostic state, then get assigned to a specific tenant.

to take this PR's logic further, why not rename Server to TenantSpecificServer? (i don't think we should, just asking rhetorically)

Yes, once this PR and its subsequent PRs, up to and including #92579 are merged, this would make (rhetorically) sense.

This was referenced Nov 28, 2022
Merged
Merged
Merged
Merged
Merged
stevendanna
stevendanna approved these changes Nov 29, 2022
View reviewed changes
Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

LGTM

@knz
Copy link
Contributor Author

knz commented Nov 29, 2022

rafi, I will be willing to rename the various concepts in this code any time, for example in a followup PR.

bors r=stevendanna

@craig
Copy link
Contributor

craig bot commented Nov 29, 2022

Build failed:

@knz
Copy link
Contributor Author

knz commented Nov 29, 2022

unrelated flake

bors r=stevendanna

@craig
Copy link
Contributor

craig bot commented Nov 29, 2022

Canceled.

@knz
Copy link
Contributor Author

knz commented Nov 29, 2022

bors r=stevendanna

@craig
Copy link
Contributor

craig bot commented Nov 29, 2022

Build succeeded:

@craig craig bot merged commit b5cab45 into 20221116-shared-sql-base Nov 29, 2022
@craig craig bot deleted the 20221116-shared-sql-1 branch November 29, 2022 22:39
@knz knz mentioned this pull request Jan 8, 2023
Merged
9 tasks
craig bot pushed a commit that referenced this pull request Jan 9, 2023
@knz
Merge #92580
27ebfa8 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafiss rafiss rafiss approved these changes

@stevendanna stevendanna stevendanna approved these changes

Assignees

@knz knz

Labels
A-multitenancy Related to multi-tenancy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@knz @cockroach-teamcity @stevendanna @rafiss