sql/pgwire: send down parameter status updates #42376

otan · 2019-11-11T21:13:09Z

Resolves #40854.

Server parameters such as application_name and timezone require updates
over pgwire when they are changed. This was previously not done.

In this PR, we introduce a listener-esque object on sessionDataMutator
that pgwire will add itself onto to send updates on these changes.

Coincidentally, this means a few logic tests changes because lib/pq will
encode this into the string.

Release note (sql change): This PR introduces a new pgwire update that
sends ParameterStatus messages when certain server parameters are
changed for the given session over pgwire.

cockroach-teamcity · 2019-11-11T21:13:16Z

This change is

maddyblue

Does this make that JDBC test pass?

pkg/sql/pgwire/testdata/pgtest/connection_params

maddyblue · 2019-11-11T21:34:03Z

pkg/sql/exec_util.go

+	if m.onSessionDataChangeListeners == nil {
+		m.onSessionDataChangeListeners = make(map[string][]func(val interface{}))
+	}
+	if _, ok := m.onSessionDataChangeListeners[key]; !ok {


There's no need for this block.

maddyblue · 2019-11-11T21:34:45Z

pkg/sql/exec_util.go

-	applicationNameChanged func(newName string)
+	// onSessionDataChangeListeners stores all the observers to execute when
+	// session data is modified, keyed by the value to change on.
+	onSessionDataChangeListeners map[string][]func(val interface{})


Why does this need to be a slice? Can there ever by more than one per key?

maddyblue · 2019-11-11T22:26:18Z

pkg/sql/pgwire/conn.go

 		value := connHandler.GetStatusParam(ctx, param)
 		if err := c.sendStatusParam(param, value); err != nil {
 			return sql.ConnectionHandler{}, err
 		}
+		// pgwire also expects updates when these parameters change.
+		connHandler.RegisterOnSessionDataChange(param, func(val interface{}) {
+			convertVal := val.(string)


Can val be a string instead of interface{} to avoid the forced cast? Seems unsafe otherwise.

otan

Yep, the tests pass with this change.

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

pkg/sql/exec_util.go, line 1743 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Why does this need to be a slice? Can there ever by more than one per key?

Yes, I think the application_name one can be used more than once - once for stats and once for pgwire (the change for this is somewhere else in this PR).

pkg/sql/exec_util.go, line 1752 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

There's no need for this block.

Ooh.

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

Previously, mjibson (Matt Jibson) wrote…

Can val be a string instead of interface{} to avoid the forced cast? Seems unsafe otherwise.

Yes, I saw some Set params that were not strings, but they don't seem to need listeners (yet) so okay with doing this.

pkg/sql/pgwire/testdata/pgtest/connection_params, line 22 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Do we support word time zones (MDT)? If so I'd like to see another test with that.

Added a test.

otan

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

pkg/sql/pgwire/testdata/pgtest/connection_params, line 1 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

If this file differs from what postgres does it should document that with a comment.

Done.

maddyblue

Can you flesh out the comment in the commit message about exactly why the logic test results changed? Is it because lib/pq supports reading the timezone back from the connection and sets the location data of received timestamps from it?

maddyblue · 2019-11-11T22:52:34Z

pkg/sql/pgwire/testdata/pgtest/connection_params

+{"Type":"ReadyForQuery","TxStatus":"I"}
+
+# Change the time zone.
+# NOTE: the order is different than postgres, because we execute the statement


once at the top is enough

maddyblue · 2019-11-11T22:56:13Z

pkg/sql/pgwire/testdata/pgtest/connection_params

@@ -0,0 +1,55 @@
+# Change the application name.
+# NOTE: the order is different than postgres, because we execute the statement


Are you sure this is the reason? The commit message is more unsure. (I'm also unsure, I just don't want to have an incorrect statement here.)

otan

Can you flesh out the comment in the commit message about exactly why the logic test results changed? Is it because lib/pq supports reading the timezone back from the connection and sets the location data of received timestamps from it?
yep!

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

pkg/sql/pgwire/testdata/pgtest/connection_params, line 2 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Are you sure this is the reason? The commit message is more unsure. (I'm also unsure, I just don't want to have an incorrect statement here.)

I dug even further into this and realised I wasn't buffering. I got mislead earlier.

pkg/sql/pgwire/testdata/pgtest/connection_params, line 22 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

once at the top is enough

Done.

pkg/sql/vars.go

rafiss · 2019-11-12T19:23:13Z

Nice stuff! Could you take a look at pgjdbc_blacklist.go, then find the tests that are failing due to issue #40854, and update the blacklist for v20.1 if the tests pass now?

Server parameters such as `application_name` require updates over pgwire when they are charged. This was previously not done. This applies to time zone changes as well. In this PR, we introduce a listener-esque object on `sessionDataMutator` that pgwire will add itself onto to send updates on these changes. Coincidentally, this means a few logic tests changes because lib/pq will encode this into the string. Release note (sql change): This PR introduces a new pgwire update that sends ParameterStatus messages when certain server parameters are changed for the given session over pgwire.

otan · 2019-11-13T13:24:06Z

thanks!

bors r+

42376: sql/pgwire: send down parameter status updates r=otan a=otan Resolves #40854. Server parameters such as `application_name` and `timezone` require updates over pgwire when they are changed. This was previously not done. In this PR, we introduce a listener-esque object on `sessionDataMutator` that pgwire will add itself onto to send updates on these changes. Coincidentally, this means a few logic tests changes because lib/pq will encode this into the string. Release note (sql change): This PR introduces a new pgwire update that sends ParameterStatus messages when certain server parameters are changed for the given session over pgwire. Co-authored-by: Oliver Tan <otan@cockroachlabs.com>

craig · 2019-11-13T13:55:47Z

Build succeeded

GitHub CI (Cockroach)

andreimatei

@otan did you attempt to implement this through the sql.CommandResult interface, instead of creating a new communication mechanism between sql and pgwire? I'm thinking the sql.CommandResultcould carry these updates, which would make the control flow much easier to reason about.

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

otan · 2020-02-08T00:31:55Z

@andreimatei that works too, see #44883. couldn't get this to work for CommandResult, but did something similar. This will be a nice extension for the work needed for notices.

otan requested review from maddyblue, rafiss and a team November 11, 2019 21:13

maddyblue reviewed Nov 11, 2019

View reviewed changes

otan commented Nov 11, 2019

View reviewed changes

otan force-pushed the otan-application_name_fix branch from 98b1e7b to db63b33 Compare November 11, 2019 22:45

maddyblue reviewed Nov 11, 2019

View reviewed changes

otan mentioned this pull request Nov 12, 2019

pgwire: TimeZone ParameterStatus field incorrect with offsets #42404

Closed

otan force-pushed the otan-application_name_fix branch from db63b33 to e891f0a Compare November 12, 2019 14:37

otan requested a review from maddyblue November 12, 2019 14:38

otan commented Nov 12, 2019

View reviewed changes

otan force-pushed the otan-application_name_fix branch 2 times, most recently from 19ce597 to 00f99b3 Compare November 12, 2019 14:48

otan commented Nov 12, 2019

View reviewed changes

pkg/sql/vars.go Show resolved Hide resolved

otan force-pushed the otan-application_name_fix branch from 00f99b3 to 08c89c4 Compare November 12, 2019 19:26

maddyblue approved these changes Nov 13, 2019

View reviewed changes

craig bot merged commit 08c89c4 into cockroachdb:master Nov 13, 2019

otan mentioned this pull request Feb 7, 2020

sql,pgwire: missing ParameterStatus packets upon SET statements #44804

Closed

andreimatei reviewed Feb 7, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql/pgwire: send down parameter status updates #42376

sql/pgwire: send down parameter status updates #42376

otan commented Nov 11, 2019 •

edited

Loading

cockroach-teamcity commented Nov 11, 2019

maddyblue left a comment

maddyblue Nov 11, 2019

maddyblue Nov 11, 2019

maddyblue Nov 11, 2019

otan left a comment

otan left a comment

maddyblue left a comment

maddyblue Nov 11, 2019

maddyblue Nov 11, 2019

otan left a comment

rafiss commented Nov 12, 2019

otan commented Nov 13, 2019

craig bot commented Nov 13, 2019

andreimatei left a comment

otan commented Feb 8, 2020

		@@ -0,0 +1,55 @@
		# Change the application name.
		# NOTE: the order is different than postgres, because we execute the statement

sql/pgwire: send down parameter status updates #42376

sql/pgwire: send down parameter status updates #42376

Conversation

otan commented Nov 11, 2019 • edited Loading

cockroach-teamcity commented Nov 11, 2019

maddyblue left a comment

Choose a reason for hiding this comment

maddyblue Nov 11, 2019

Choose a reason for hiding this comment

maddyblue Nov 11, 2019

Choose a reason for hiding this comment

maddyblue Nov 11, 2019

Choose a reason for hiding this comment

otan left a comment

Choose a reason for hiding this comment

otan left a comment

Choose a reason for hiding this comment

maddyblue left a comment

Choose a reason for hiding this comment

maddyblue Nov 11, 2019

Choose a reason for hiding this comment

maddyblue Nov 11, 2019

Choose a reason for hiding this comment

otan left a comment

Choose a reason for hiding this comment

rafiss commented Nov 12, 2019

otan commented Nov 13, 2019

craig bot commented Nov 13, 2019

Build succeeded

andreimatei left a comment

Choose a reason for hiding this comment

otan commented Feb 8, 2020

otan commented Nov 11, 2019 •

edited

Loading