pgwire: split more code into pre-serve and serve logic #92576

knz · 2022-11-28T15:25:20Z

Parent PRs:

This PR splits the pre-connection logic (tenant-independent) into its own code sections.

Epic: CRDB-14537

cockroach-teamcity · 2022-11-28T15:25:33Z

This change is

rafiss

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

pkg/sql/pgwire/server.go line 987 at r3 (raw file):

	// Replace RemoteAddr if specified by client and we trust the client.
	if cp.clientProvidedRemoteAddr != "" {
		if !trustClientProvidedRemoteAddr {

since this trustClientProvidedRemoteAddr comes from an env var, i don't see why the client-provided address logic is tenant-specfic

pkg/sql/pgwire/server.go line 258 at r4 (raw file):

	// tenantConnMonitor is the pool where the memory usage for the
	// initial connection overhead is accounted for.
	tenantConnMonitor *mon.BytesMonitor

should this be called tenantSpecificConnMonitor? it seems like that tenantSpecificXYZ convention was used elsewhere

knz

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

pkg/sql/pgwire/server.go line 987 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

since this trustClientProvidedRemoteAddr comes from an env var, i don't see why the client-provided address logic is tenant-specfic

You are very right. I'm moving it there in #92579.

pkg/sql/pgwire/server.go line 258 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

should this be called tenantSpecificConnMonitor? it seems like that tenantSpecificXYZ convention was used elsewhere

Done.

rafiss

Reviewed 1 of 5 files at r5, 4 of 4 files at r8, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz)

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

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).

Release note: None

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

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>

knz self-assigned this Nov 28, 2022

knz changed the title ~~pgwire: lift readVersion + TLS handshake into the pre-serve handler~~ pgwire: simple code movement - no functionality change Nov 28, 2022

cockroachdb deleted a comment from cockroach-teamcity Nov 28, 2022

knz added the A-multitenancy Related to multi-tenancy label Nov 28, 2022

knz marked this pull request as ready for review November 28, 2022 15:42

knz requested a review from a team November 28, 2022 15:42

knz requested a review from a team as a code owner November 28, 2022 15:42

knz changed the title ~~pgwire: simple code movement - no functionality change~~ pgwire: split more code into pre-serve and serve logic Nov 28, 2022

knz requested a review from rafiss November 28, 2022 16:57

knz force-pushed the 20221116-shared-sql-2 branch from c8c2281 to fd47111 Compare November 28, 2022 18:26

knz force-pushed the 20221116-shared-sql-3 branch from 3b3795f to 9492997 Compare November 28, 2022 18:26

knz force-pushed the 20221116-shared-sql-2 branch from fd47111 to ec14b68 Compare November 29, 2022 14:58

knz requested a review from a team November 29, 2022 14:58

knz force-pushed the 20221116-shared-sql-3 branch from 9492997 to 521774a Compare November 29, 2022 14:58

dhartunian removed the request for review from a team November 29, 2022 16:48

rafiss requested changes Nov 29, 2022

View reviewed changes

knz force-pushed the 20221116-shared-sql-2 branch from ec14b68 to 37ecd71 Compare December 5, 2022 19:56

knz force-pushed the 20221116-shared-sql-3 branch 2 times, most recently from 0402377 to 57e2215 Compare December 5, 2022 20:24

knz force-pushed the 20221116-shared-sql-2 branch from 37ecd71 to 575685f Compare December 6, 2022 12:50

knz force-pushed the 20221116-shared-sql-3 branch from 57e2215 to f6d7823 Compare December 6, 2022 12:50

knz commented Dec 6, 2022

View reviewed changes

knz force-pushed the 20221116-shared-sql-3 branch from f6d7823 to 78cace3 Compare December 6, 2022 12:59

knz requested a review from rafiss December 6, 2022 12:59

rafiss approved these changes Dec 7, 2022

View reviewed changes

knz force-pushed the 20221116-shared-sql-3 branch from 811a0bd to e366fc4 Compare January 8, 2023 18:30

knz force-pushed the 20221116-shared-sql-base branch from 6824eee to 0b3c67b Compare January 8, 2023 19:42

knz requested a review from a team as a January 8, 2023 19:42

knz force-pushed the 20221116-shared-sql-3 branch from e366fc4 to 42ea2a3 Compare January 8, 2023 19:43

knz force-pushed the 20221116-shared-sql-base branch from 0b3c67b to 7752208 Compare January 8, 2023 19:51

knz requested review from a team, herkolategan and srosenberg and removed request for a team January 8, 2023 19:51

knz force-pushed the 20221116-shared-sql-3 branch from 42ea2a3 to dcbea65 Compare January 8, 2023 19:51

knz added 6 commits January 8, 2023 23:13

cli/demo: deflake some interactive tests

a139a83

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

pgwire: unexport ServerMetrics, rename to tenantSpecificMetrics

cdcbeda

Release note: None

pgwire: fix typo in metric descriptions

cd68aa2

Release note: None

pgwire: simplify writeErr/sendErr

7e86a50

Release note: None

knz force-pushed the 20221116-shared-sql-base branch from 7752208 to 1814c21 Compare January 8, 2023 22:17

knz added 4 commits January 8, 2023 23:17

pgwire: lift readVersion + TLS handshake into the pre-serve handler

e42293f

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).

pgwire: make handleCancel unaware of conn/buf

54f789e

Release note: None

pgwire: split parseClientProvidedSessionParameters

7bd4484

We need the read/parse of the parameters to be tenant-independent, so the function cannot contain tenant-specific configuration. Release note: None

pgwire: split the pre-conn reserved memory account

c7dd014

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

knz force-pushed the 20221116-shared-sql-3 branch from dcbea65 to c7dd014 Compare January 8, 2023 22:17

knz changed the base branch from 20221116-shared-sql-base to master January 8, 2023 22:18

knz mentioned this pull request Jan 8, 2023

server,cli/demo: make the default tenant configurable #94901

Merged

9 tasks

craig bot merged commit c7dd014 into master Jan 9, 2023

craig bot deleted the 20221116-shared-sql-3 branch January 9, 2023 01:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pgwire: split more code into pre-serve and serve logic #92576

pgwire: split more code into pre-serve and serve logic #92576

knz commented Nov 28, 2022 •

edited

Loading

cockroach-teamcity commented Nov 28, 2022 •

edited by petermattis

Loading

rafiss left a comment

knz left a comment

rafiss left a comment

pgwire: split more code into pre-serve and serve logic #92576

pgwire: split more code into pre-serve and serve logic #92576

Conversation

knz commented Nov 28, 2022 • edited Loading

cockroach-teamcity commented Nov 28, 2022 • edited by petermattis Loading

rafiss left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

knz commented Nov 28, 2022 •

edited

Loading

cockroach-teamcity commented Nov 28, 2022 •

edited by petermattis

Loading