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

externalconn, streamingccl: support external connections as replication sources #96551

Merged
merged 1 commit into from
Feb 16, 2023
Merged

externalconn, streamingccl: support external connections as replication sources #96551

merged 1 commit into from
Feb 16, 2023

Conversation

HonoreDB
Copy link
Contributor

@HonoreDB HonoreDB commented Feb 4, 2023 •
edited
Loading

This PR adds support for creating external connections with the postgresql scheme and using them in a
CREATE TENANT ... FROM REPLICATION OF external://name statement. Among other benefits, this lets users validate their postgresql connection strings before creating the job, as creating the external connection will ping the source DB.

Release note (enterprise change): Added support for CREATE EXTERNAL CONNECTION ... AS "postgresql://" or "postgres://". These external connections may be specified as the source in streaming replication.

Informs #93629

@HonoreDB HonoreDB requested a review from a team as a code owner February 4, 2023 22:42
@HonoreDB HonoreDB requested review from benbardin and removed request for a team February 4, 2023 22:42
@cockroach-teamcity
Copy link
Member

This change is Reviewable

adityamaru
adityamaru suggested changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@adityamaru adityamaru left a comment

Choose a reason for hiding this comment

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

Big thank you for doing this! Overall lgtm barring one comment about the external connection type.

pkg/ccl/streamingccl/streamingest/external_connection.go
for _, scheme := range postgresSchemes() {
externalconn.RegisterConnectionDetailsFromURIFactory(
scheme,
connectionpb.ConnectionProvider_sql,
Copy link
Contributor

@adityamaru adityamaru Feb 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

heh, so I think we added this for experimental-sql but didn't end up using it because of this TODO

cockroach/pkg/ccl/changefeedccl/sink_external_connection.go

Line 112 in 2f81142

// TODO (zinger): Not including SinkSchemeExperimentalSQL for now because A: it's undocumented
. I don't know much about experimental sql sinks but does it makes sense to reuse this type or should we delete it and add a ConnectionProvider_postgres. On a similar note, I don't think this ExternalConnection object should be of type Storage:

cockroach/pkg/cloud/externalconn/connectionpb/connection.go

Line 24 in eb3e560

ConnectionProvider_webhookhttp, ConnectionProvider_webhookhttps, ConnectionProvider_gcpubsub:
. Should we add a new type?

@adityamaru adityamaru self-requested a review February 7, 2023 15:09
HonoreDB
HonoreDB commented Feb 7, 2023
View reviewed changes
Copy link
Contributor Author

@HonoreDB HonoreDB 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 @adityamaru and @benbardin)

pkg/ccl/streamingccl/streamingest/external_connection.go line 48 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

heh, so I think we added this for experimental-sql but didn't end up using it because of this TODO

cockroach/pkg/ccl/changefeedccl/sink_external_connection.go

Line 112 in 2f81142

// TODO (zinger): Not including SinkSchemeExperimentalSQL for now because A: it's undocumented
. I don't know much about experimental sql sinks but does it makes sense to reuse this type or should we delete it and add a ConnectionProvider_postgres. On a similar note, I don't think this ExternalConnection object should be of type Storage:

cockroach/pkg/cloud/externalconn/connectionpb/connection.go

Line 24 in eb3e560

ConnectionProvider_webhookhttp, ConnectionProvider_webhookhttps, ConnectionProvider_gcpubsub:
. Should we add a new type?

As it happens, experimental-sql also requires a postgres connection string and connects the same way, so I don't think it does any harm to reuse the sql type. For the connection type, sure, I can add a new one. Would "Source" make sense? Or "ForeignData" maybe, to borrow a postgres idiom?

@adityamaru
Copy link
Contributor

As it happens, experimental-sql also requires a postgres connection string and connects the same way, so I don't think it does any harm to reuse the sql type

SGTM

TIL about ForeignData, but it sounds good to me after reading https://www.postgresql.org/docs/current/ddl-foreign-data.html#:~:text=PostgreSQL%20implements%20portions%20of%20the,of%20constraint%20within%20the%20database.).

HonoreDB
HonoreDB commented Feb 10, 2023
View reviewed changes
Copy link
Contributor Author

@HonoreDB HonoreDB 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 @adityamaru and @benbardin)

pkg/ccl/streamingccl/streamingest/external_connection.go line 48 at r1 (raw file):

Previously, HonoreDB (Aaron Zinger) wrote…

As it happens, experimental-sql also requires a postgres connection string and connects the same way, so I don't think it does any harm to reuse the sql type. For the connection type, sure, I can add a new one. Would "Source" make sense? Or "ForeignData" maybe, to borrow a postgres idiom?

Done.

@HonoreDB
Copy link
Contributor Author

bors r=[adityamaru]

@craig
Copy link
Contributor

craig bot commented Feb 13, 2023

Build failed:

@HonoreDB
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 16, 2023

Build failed (retrying...):

@otan
Copy link
Contributor

otan commented Feb 16, 2023

bors r-

needs a dev generate bazel

@craig
Copy link
Contributor

craig bot commented Feb 16, 2023

Canceled.

@HonoreDB
externalconn, streamingccl: support external connections as replicati…
7fed328 
…on sources

This PR adds support for creating external connections with the
postgresql scheme and using them in a
`CREATE TENANT ... FROM REPLICATION OF external://name` statement.
Among other benefits, this lets users validate their postgresql
connection strings before creating the job, as creating the external
connection will ping the source DB.

Release note (enterprise change): Added support for `CREATE EXTERNAL CONNECTION ... AS "postgresql://"` or `"postgres://"`. These external connections may be specified as the source in streaming replication.
@HonoreDB
Copy link
Contributor Author

Sorry about that.

@HonoreDB
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 16, 2023

Build succeeded:

@craig craig bot merged commit f1958ad into cockroachdb:master Feb 16, 2023
@cockroach-teamcity cockroach-teamcity mentioned this pull request Feb 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adityamaru adityamaru adityamaru approved these changes

@benbardin benbardin Awaiting requested review from benbardin benbardin is a code owner automatically assigned from cockroachdb/disaster-recovery

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@HonoreDB @cockroach-teamcity @adityamaru @otan