sql/pgwire: send placeholder BackendKeyData #60281

rafiss · 2021-02-09T22:37:12Z

Some tools expect this message to be returned at connection time and
will not connect without it. CockroachDB does not support pgwire
cancellation, but we can still send a placeholder value here, and
continue ignoring cancellation requests like we already do.

Added a small test to make sure nothing broke.

Release note (sql change): When a connection is established, CockroachDB
will now return a placeholder BackendKeyData message in the response.
This is for compatibility with some tools, but using the BackendKeyData
to cancel a query will still have no effect, just the same as before.

cockroach-teamcity · 2021-02-09T22:37:19Z

This change is

rafiss · 2021-02-09T23:39:19Z

@asubiotto @jordanlewis i've requested your review since you may have more context from #13009

jordanlewis

Can you add some pgwire data driven tests for this too? I'm actually surprised we didn't see any failures in those, since we're changing what we're sending - what explains that?

What ORM/driver does this change target, again? Do we have tests for it?

Overall I think this looks good. Have you already gone spelunking for why we didn't merge a placeholder BackendKeyData originally? I vaguely recall @asubiotto sending a PR for this, but it obviously never got merged and I wonder why that's the case.

rafiss · 2021-02-10T02:41:00Z

Can you add some pgwire data driven tests for this too? I'm actually surprised we didn't see any failures in those, since we're changing what we're sending - what explains that?

i think it's because this message is during the connection handshake, and those tests only include assertions after the connection has been established. i'll look into enhancing those tests.

What ORM/driver does this change target, again? Do we have tests for it?

the tool mentioned in #13191 (which is the issue that prompted my attention here) is the py-postgresql driver for python3. the tool mentioned in #13009 (which was Alfonso's earlier PR) is pgpool. we don't have tests for these tools. i'm not sure if we should, since our approach has only been to test tools that we want to document support for, but i'm open to hearing an argument in favor of testing now.

Overall I think this looks good. Have you already gone spelunking for why we didn't merge a placeholder BackendKeyData originally? I vaguely recall @asubiotto sending a PR for this, but it obviously never got merged and I wonder why that's the case.

Alfonso's comment (#13009 (comment)) describes why. at that time, it seems that we were waiting until there was telemetry and unimplemented errors for when we received a CancelRequest message. we now do have this in place.

jordanlewis · 2021-02-10T02:57:41Z

Ok, sounds good. We need to have at least some test that this functionality is actually usable by clients - that they receive the key data without larger issues. I suppose all of the ORM tests will check this automatically too.

LGTM if you can check via the pgwire datadriven tests (or really any pgwire test that actually inspects the bytes) that we're sending the cancel data message - would be a shame if we had a regression here and never noticed.

asubiotto

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

pkg/sql/pgwire/conn.go, line 310 at r1 (raw file):

		// normally used by clients to send a CancelRequest message:
		// https://www.postgresql.org/docs/9.6/static/protocol-flow.html#AEN112861
		// CockroachDB currently ignores all CancelRequests.

Is this actually ignored or do we return some unimplemented error? If the former, would it make sense/be a lot of work to return an error? OK to do this work separately.

pkg/sql/pgwire/conn_test.go, line 1525 at r1 (raw file):

}

// TestReadTimeoutConn asserts that a readTimeoutConn performs reads normally

nit: name

pkg/sql/pgwire/conn_test.go, line 1545 at r1 (raw file):

	ctx, cancel := context.WithCancel(context.Background())

	// Delay execution for just a bit until db.QueryContext has begun.

Hmm, this looks like it's going to be flaky. You could use one of the executor testing knob callbacks (Before{Prepare,Execute}) in StartServer to only cancel the context once it receives the select pg_sleep statement on the server side.

asubiotto · 2021-02-10T11:21:59Z

Thanks for this change BTW! Good to close such an early issue.

rafiss

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

pkg/sql/pgwire/conn.go, line 310 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Is this actually ignored or do we return some unimplemented error? If the former, would it make sense/be a lot of work to return an error? OK to do this work separately.

we don't return an error, since it is actually quite common for clients to call already. we do have telemetry on it:

cockroach/pkg/sql/pgwire/server.go

Lines 672 to 678 in cd39473

    
           func handleCancel(conn net.Conn) error { 
        
           	// Since we don't support this, close the door in the client's 
        
           	// face. Make a note of that use in telemetry. 
        
           	telemetry.Inc(sqltelemetry.CancelRequestCounter) 
        
           	_ = conn.Close() 
        
           	return nil 
        
           }

the data here shows that tools frequently invoke this, so it could potentially have far-reaching effects to change this to be an error: #41335 (comment)

pkg/sql/pgwire/conn_test.go, line 1525 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

nit: name

fixed

pkg/sql/pgwire/conn_test.go, line 1545 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Hmm, this looks like it's going to be flaky. You could use one of the executor testing knob callbacks (Before{Prepare,Execute}) in StartServer to only cancel the context once it receives the select pg_sleep statement on the server side.

nice! thanks for this pointer. for some context, i just copied this test from the lib/pq test suite: https://github.com/lib/pq/blob/4604d39ddc9f62e2ca152114c73aec99b77a3468/go18_test.go#L116

using callbacks is way better though

rafiss

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

pkg/sql/pgwire/conn.go, line 310 at r1 (raw file):
the Postgres docs make CancelRequest seem like a "best-effort" thing so, i think it's dine to not have an error even though we don't do anything

A CancelRequest message will be ignored unless it contains the same key data (PID and secret key) passed to the frontend during connection start-up. If the request matches the PID and secret key for a currently executing backend, the processing of the current query is aborted. (In the existing implementation, this is done by sending a special signal to the backend process that is processing the query.)

The cancellation signal might or might not have any effect — for example, if it arrives after the backend has finished processing the query, then it will have no effect. If the cancellation is effective, it results in the current command being terminated early with an error message.

pkg/sql/pgwire/conn_test.go, line 1545 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nice! thanks for this pointer. for some context, i just copied this test from the lib/pq test suite: https://github.com/lib/pq/blob/4604d39ddc9f62e2ca152114c73aec99b77a3468/go18_test.go#L116

using callbacks is way better though

Done.

rafiss · 2021-02-10T15:37:00Z

this is ready for another look. i've added a test to pkg/testutils/pgtest/pgtest.go that shows the BackendKeyData being received.

asubiotto

Reviewed 3 of 3 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis and @rafiss)

pkg/sql/pgwire/conn_test.go, line 1557 at r2 (raw file):

	// Cancellation has no effect on ongoing query.
	if _, err := db.QueryContext(cancelCtx, "select pg_sleep(0.2)"); err != nil {

nit: ok to make this 0?

rafiss · 2021-02-10T17:41:45Z

it looks like i need to update sqlproxy to be aware of BackendKeyData. gonna clear this with the CC team.

rafiss

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

pkg/sql/pgwire/conn_test.go, line 1557 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

nit: ok to make this 0?

done

Some tools expect this message to be returned at connection time and will not connect without it. CockroachDB does not support pgwire cancellation, but we can still send a placeholder value here, and continue ignoring cancellation requests like we already do. Added a pgwire cancellation test to make sure nothing broke, as well as a pgwire-level test to make sure the BackendKeyData is sent to the client. Release note (sql change): When a connection is established, CockroachDB will now return a placeholder BackendKeyData message in the response. This is for compatibility with some tools, but using the BackendKeyData to cancel a query will still have no effect, just the same as before.

rafiss · 2021-02-10T17:53:00Z

@spaskob i see that this affects an area of sqlproxy that you worked on. can you please take a look at the changes? also, can you confirm that this update to sqlproxy will be able to be deployed before CockroachDB 21.1 is released?

spaskob · 2021-02-10T18:48:12Z

Thanks @rafiss for fixing the sqlproxy code. The change looks good to me. Do you mind running https://github.com/cockroachdb/cockroach/blob/master/pkg/ccl/sqlproxyccl/run_proxy.sh to make sure the proxy works e2e?

rafiss · 2021-02-10T19:52:12Z

@spaskob thanks! that test passes for me locally.

how often does sqlproxy get released? as long as it's released before CC uses CRDB 21.1 we should be good.

rafiss · 2021-02-10T20:45:18Z

thanks for the reviews all!

bors r=asubiotto,spaskob

spaskob

There's no release schedule I will pick up this commit after you merge and create a ticket to release it.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto and @jordanlewis)

spaskob

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto and @jordanlewis)

craig · 2021-02-10T22:22:01Z

Build succeeded:

GitHub CI (Cockroach)

rafiss force-pushed the dummy-backendkey branch from aa19cec to 71399b0 Compare February 9, 2021 23:35

rafiss requested review from jordanlewis, a team and asubiotto February 9, 2021 23:37

jordanlewis reviewed Feb 10, 2021

View reviewed changes

asubiotto suggested changes Feb 10, 2021

View reviewed changes

rafiss force-pushed the dummy-backendkey branch from 71399b0 to ab68d78 Compare February 10, 2021 15:12

rafiss commented Feb 10, 2021

View reviewed changes

rafiss force-pushed the dummy-backendkey branch from ab68d78 to 62c50a9 Compare February 10, 2021 15:36

rafiss requested review from asubiotto and jordanlewis February 10, 2021 15:36

rafiss force-pushed the dummy-backendkey branch from 62c50a9 to 24adfee Compare February 10, 2021 15:47

asubiotto approved these changes Feb 10, 2021

View reviewed changes

rafiss commented Feb 10, 2021

View reviewed changes

rafiss force-pushed the dummy-backendkey branch from 24adfee to c749a60 Compare February 10, 2021 17:51

rafiss requested a review from spaskob February 10, 2021 17:52

spaskob reviewed Feb 10, 2021

View reviewed changes

craig bot merged commit 5095a3c into cockroachdb:master Feb 10, 2021

rafiss deleted the dummy-backendkey branch February 11, 2021 01:44

jseldess mentioned this pull request Mar 23, 2021

sql/pgwire: send placeholder BackendKeyData cockroachdb/docs#10073

Closed

rausnitz mentioned this pull request Jun 3, 2022

Make backend key data optional vapor/postgres-nio#296

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql/pgwire: send placeholder BackendKeyData #60281

sql/pgwire: send placeholder BackendKeyData #60281

rafiss commented Feb 9, 2021 •

edited

Loading

cockroach-teamcity commented Feb 9, 2021

rafiss commented Feb 9, 2021

jordanlewis left a comment

rafiss commented Feb 10, 2021

jordanlewis commented Feb 10, 2021

asubiotto left a comment

asubiotto commented Feb 10, 2021

rafiss left a comment

rafiss left a comment

rafiss commented Feb 10, 2021

asubiotto left a comment

rafiss commented Feb 10, 2021

rafiss left a comment

rafiss commented Feb 10, 2021

spaskob commented Feb 10, 2021

rafiss commented Feb 10, 2021

rafiss commented Feb 10, 2021

spaskob left a comment

spaskob left a comment

craig bot commented Feb 10, 2021

	func handleCancel(conn net.Conn) error {
	// Since we don't support this, close the door in the client's
	// face. Make a note of that use in telemetry.
	telemetry.Inc(sqltelemetry.CancelRequestCounter)
	_ = conn.Close()
	return nil
	}

sql/pgwire: send placeholder BackendKeyData #60281

sql/pgwire: send placeholder BackendKeyData #60281

Conversation

rafiss commented Feb 9, 2021 • edited Loading

cockroach-teamcity commented Feb 9, 2021

rafiss commented Feb 9, 2021

jordanlewis left a comment

Choose a reason for hiding this comment

rafiss commented Feb 10, 2021

jordanlewis commented Feb 10, 2021

asubiotto left a comment

Choose a reason for hiding this comment

asubiotto commented Feb 10, 2021

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss commented Feb 10, 2021

asubiotto left a comment

Choose a reason for hiding this comment

rafiss commented Feb 10, 2021

rafiss left a comment

Choose a reason for hiding this comment

rafiss commented Feb 10, 2021

spaskob commented Feb 10, 2021

rafiss commented Feb 10, 2021

rafiss commented Feb 10, 2021

spaskob left a comment

Choose a reason for hiding this comment

spaskob left a comment

Choose a reason for hiding this comment

craig bot commented Feb 10, 2021

rafiss commented Feb 9, 2021 •

edited

Loading