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

pgrepl: allow logging in with replication parameter #105400

Merged
merged 1 commit into from
Jul 4, 2023

Conversation

otan
Copy link
Contributor

@otan otan commented Jun 23, 2023

This commit adds the replication=database connection parameter, which enables certain replication commands to be run.
See: https://www.postgresql.org/docs/current/protocol-replication.html

We do this by making it a session variable instead of a parameter on the connection like PG does. This adds a hidden replication connection parameter to accomplish this.

Informs: #105130

Release note: None

@otan otan requested a review from rafiss June 23, 2023 01:09
@otan otan requested review from a team as code owners June 23, 2023 01:09
@otan otan requested a review from yuzefovich June 23, 2023 01:09
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich removed their request for review June 23, 2023 01:25
@otan otan force-pushed the replication_protobuf branch 5 times, most recently from a9a2928 to c6728de Compare June 23, 2023 05:46
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.

lgtm! just a small comment

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


pkg/sql/pgwire/pre_serve_options.go line 97 at r1 (raw file):

		case "replication":
			// See session variable comment for the reason behind the remapping.

nit: could you change to "behind the error remapping" -- took me a while to understand this comment.


pkg/sql/pgrepl/pgrepl_test.go line 35 at r1 (raw file):

func TestMain(m *testing.M) {
	securityassets.SetLoader(securitytest.EmbeddedAssets)
	serverutils.InitTestServerFactory(server.TestServerFactory)

nit: add the leaktest auto generate thingo?

Copy link
Contributor Author

@otan otan 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 @rafiss)


pkg/sql/pgwire/pre_serve_options.go line 97 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: could you change to "behind the error remapping" -- took me a while to understand this comment.

i've replaced it with an actual comment.

@otan
Copy link
Contributor Author

otan commented Jul 3, 2023

pkg/sql/pgwire/pre_serve_options.go line 97 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

i've replaced it with an actual comment.

sorry, i mean a full comment*

@otan otan force-pushed the replication_protobuf branch 2 times, most recently from d5edb4e to f1c861b Compare July 3, 2023 22:18
@otan
Copy link
Contributor Author

otan commented Jul 3, 2023

bors r=rafiss

@craig
Copy link
Contributor

craig bot commented Jul 3, 2023

Build failed:

This commit adds the `replication=database` connection parameter, which
enables certain replication commands to be run.
See: https://www.postgresql.org/docs/current/protocol-replication.html

We do this by making it a session variable instead of a parameter on the
connection like PG does. This adds a hidden `replication` connection
parameter to accomplish this.

Release note: None
@otan otan force-pushed the replication_protobuf branch from f1c861b to 569669c Compare July 3, 2023 23:34
@otan
Copy link
Contributor Author

otan commented Jul 3, 2023

bors r=rafiss

@craig
Copy link
Contributor

craig bot commented Jul 4, 2023

Build succeeded:

@craig craig bot merged commit 428dc9d into cockroachdb:master Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants