server,sql: add an interface for internal SQL sessions#95702
server,sql: add an interface for internal SQL sessions#95702craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
|
This is used in #95703; I've made it a separate PR because it probably deserves more focused review. |
andreimatei
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @knz)
pkg/sql/pgwire/hba_conf.go line 220 at r1 (raw file):
} // If not rule for ConnInternalLoopback connections has been specified, add
Rafa, does this look good to you?
You expressed doubt that the stanza above does the right thing, btw, given that it only deals with one entry but we used to have two by default. I'm not sure whether it should also deal with the 2nd or not; perhaps take a look.
knz
left a comment
There was a problem hiding this comment.
Yes, this generally works thanks.
I still don't understand your motivation for not using connManager like the other listeners. I find it generally beneficial and important to reuse existing patterns because it makes the code easier to learn and maintain by others.
The specific problem you've encountered when you tried it (something about observing shutdown? I'm not sure I fully understand) can probably be remediated by other ways.
Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status:
pkg/server/tenant.go line 748 at r1 (raw file):
// internal loopback interface. func (s *SQLServerWrapper) AcceptInternalClients(ctx context.Context) error { serveConn := func(ctx context.Context, conn net.Conn, status pgwire.PreServeStatus) error {
Bringing my comment over from #95703.
I don't see why this can't be served by the same connManager-based logic as the others.
pkg/sql/pgwire/hba_conf.go line 220 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Rafa, does this look good to you?
You expressed doubt that the stanza above does the right thing, btw, given that it only deals with one entry but we used to have two by default. I'm not sure whether it should also deal with the 2nd or not; perhaps take a look.
Yes this looks good now.
Coming back to your comment from #95703: I would be OK with a special case instead of an HBA rule, if you'd prefer this not be exposed in the configurable HBA rules. However, your code here is following pre-existing patterns so it also works for me.
pkg/sql/pgwire/hba_conf.go line 303 at r1 (raw file):
host all all all cert-password # built-in CockroachDB default local all all password # built-in CockroachDB default loopback all all all trust # built-in CockroachDB default
If you decide to keep this, you can move it to the first line since this is what your logic above does.
Also please align the columns.
bf81f46 to
7a670db
Compare
knz
left a comment
There was a problem hiding this comment.
👍 i'll have another look when you ask me to.
Reviewed 20 of 22 files at r2, all commit messages.
Reviewable status:
andreimatei
left a comment
There was a problem hiding this comment.
I still don't understand your motivation for not using connManager like the other listeners. I find it generally beneficial and important to reuse existing patterns because it makes the code easier to learn and maintain by others.
done
Reviewable status:
pkg/server/tenant.go line 748 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
Bringing my comment over from #95703.
I don't see why this can't be served by the same connManager-based logic as the others.
Done.
pkg/sql/pgwire/hba_conf.go line 220 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
Yes this looks good now.
Coming back to your comment from #95703: I would be OK with a special case instead of an HBA rule, if you'd prefer this not be exposed in the configurable HBA rules. However, your code here is following pre-existing patterns so it also works for me.
left it as is
pkg/sql/pgwire/hba_conf.go line 303 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
If you decide to keep this, you can move it to the first line since this is what your logic above does.
Also please align the columns.
Done.
knz
left a comment
There was a problem hiding this comment.
LGTM modulo nits
Reviewed 2 of 22 files at r2.
Reviewable status:
pkg/server/initial_sql.go line 33 at r2 (raw file):
ctx context.Context, startSingleNode bool, adminUser, adminPassword string, ) error { log.Infof(ctx, "!!! RunInitialSQL")
nit
pkg/server/server.go line 1982 at r2 (raw file):
// internal loopback interface. func (s *Server) AcceptInternalClients(ctx context.Context) error { log.Infof(ctx, "!!! AcceptInternalClients 1")
nit here and below
7a670db to
29e28c8
Compare
andreimatei
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/server/server.go line 1982 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
nit here and below
done
pkg/server/initial_sql.go line 33 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
nit
done
This patch adds a way for a CRDB node to open pgwire connections to itself. This is implemented through the use of net.Conn's implemented by in-memory pipes. I want to use this in particular in order to embed the Obs Service into CRBD. The Obs Service is a library that wants to be given a handle to the backend database in the form of a pgx connection pool. Now such a connection pool can be created based on this new "network" interface, without any network actually being involved. Beyond that, I think this is a generally useful feature that we should have had for a long time - frequently it's useful internally to have a SQL session against the cluster; so far we've been doing that with the InternalExecutor, except that guy doesn't actually support sessions (it mostly supports single statements). Release note: None Epic: None
29e28c8 to
f5f5d9a
Compare
|
bors r+ |
|
Build succeeded: |
This patch adds a way for a CRDB node to open pgwire connections to itself. This is implemented through the use of net.Conn's implemented by in-memory pipes.
I want to use this in particular in order to embed the Obs Service into CRBD. The Obs Service is a library that wants to be given a handle to the backend database in the form of a pgx connection pool. Now such a connection pool can be created based on this new "network" interface, without any network actually being involved. Beyond that, I think this is a generally useful feature that we should have had for a long time - frequently it's useful internally to have a SQL session against the cluster; so far we've been doing that with the InternalExecutor, except that guy doesn't actually support sessions (it mostly supports single statements).
Release note: None
Epic: None