From 08c89c46926a9668fec664741070d10d3fd33fed Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Mon, 11 Nov 2019 15:30:13 -0500 Subject: [PATCH] sql/pgwire: send down parameter status updates 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. --- pkg/cmd/roachtest/pgjdbc_blacklist.go | 3 - pkg/sql/conn_executor.go | 10 ++- pkg/sql/exec_util.go | 26 +++++-- .../testdata/logic_test/alter_column_type | 2 +- .../logictest/testdata/logic_test/datetime | 20 ++--- pkg/sql/logictest/testdata/logic_test/prepare | 2 +- .../logictest/testdata/logic_test/timestamp | 2 +- pkg/sql/pgwire/conn.go | 14 ++++ .../pgwire/testdata/pgtest/connection_params | 73 +++++++++++++++++++ pkg/sql/vars.go | 26 ++++--- pkg/util/timeutil/time_zone_util.go | 2 + 11 files changed, 146 insertions(+), 34 deletions(-) create mode 100644 pkg/sql/pgwire/testdata/pgtest/connection_params diff --git a/pkg/cmd/roachtest/pgjdbc_blacklist.go b/pkg/cmd/roachtest/pgjdbc_blacklist.go index 86f36ac9bc2a..3a51a3ff983c 100644 --- a/pkg/cmd/roachtest/pgjdbc_blacklist.go +++ b/pkg/cmd/roachtest/pgjdbc_blacklist.go @@ -1270,9 +1270,6 @@ var pgjdbcBlackList20_1 = blacklist{ "org.postgresql.test.jdbc4.BlobTest.testGetBinaryStreamWithBoundaries": "26725", "org.postgresql.test.jdbc4.BlobTest.testSetBlobWithStream": "26725", "org.postgresql.test.jdbc4.BlobTest.testSetBlobWithStreamAndLength": "26725", - "org.postgresql.test.jdbc4.ClientInfoTest.testExplicitSetAppNameNotificationIsParsed": "40854", - "org.postgresql.test.jdbc4.ClientInfoTest.testSetAppName": "40854", - "org.postgresql.test.jdbc4.ClientInfoTest.testSetAppNameProps": "40854", "org.postgresql.test.jdbc4.DatabaseMetaDataTest.testGetColumnsForAutoIncrement": "41870", "org.postgresql.test.jdbc4.DatabaseMetaDataTest.testGetFunctionsWithBlankPatterns": "41872", "org.postgresql.test.jdbc4.DatabaseMetaDataTest.testGetFunctionsWithSpecificTypes": "17511", diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index c36497827a92..433248c1bd6f 100644 --- a/pkg/sql/conn_executor.go +++ b/pkg/sql/conn_executor.go @@ -429,6 +429,12 @@ func (h ConnectionHandler) GetStatusParam(ctx context.Context, varName string) s return defVal } +// RegisterOnSessionDataChange adds a listener to execute when a change on the +// given key is made using the mutator object. +func (h ConnectionHandler) RegisterOnSessionDataChange(key string, f func(val string)) { + h.ex.dataMutator.RegisterOnSessionDataChange(key, f) +} + // ServeConn serves a client connection by reading commands from the stmtBuf // embedded in the ConnHandler. // @@ -543,10 +549,10 @@ func (s *Server) newConnExecutor( sdMutator.setCurTxnReadOnly = func(val bool) { ex.state.readOnly = val } - sdMutator.applicationNameChanged = func(newName string) { + sdMutator.RegisterOnSessionDataChange("application_name", func(newName string) { ex.appStats = ex.server.sqlStats.getStatsForApplication(newName) ex.applicationName.Store(newName) - } + }) // Initialize the session data from provided defaults. We need to do this early // because other initializations below use the configured values. if err := resetSessionVars(ctx, sdMutator); err != nil { diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 8cdfd809601f..ef65ecb8e8f5 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -1737,17 +1737,30 @@ type sessionDataMutator struct { settings *cluster.Settings // setCurTxnReadOnly is called when we execute SET transaction_read_only = ... setCurTxnReadOnly func(val bool) - // applicationNamedChanged, if set, is called when the "application name" - // variable is updated. - 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 string) +} + +// RegisterOnSessionDataChange adds a listener to execute when a change on the +// given key is made using the mutator object. +func (m *sessionDataMutator) RegisterOnSessionDataChange(key string, f func(val string)) { + if m.onSessionDataChangeListeners == nil { + m.onSessionDataChangeListeners = make(map[string][]func(val string)) + } + m.onSessionDataChangeListeners[key] = append(m.onSessionDataChangeListeners[key], f) +} + +func (m *sessionDataMutator) notifyOnDataChangeListeners(key string, val string) { + for _, f := range m.onSessionDataChangeListeners[key] { + f(val) + } } // SetApplicationName sets the application name. func (m *sessionDataMutator) SetApplicationName(appName string) { m.data.ApplicationName = appName - if m.applicationNameChanged != nil { - m.applicationNameChanged(appName) - } + m.notifyOnDataChangeListeners("application_name", appName) } func (m *sessionDataMutator) SetBytesEncodeFormat(val sessiondata.BytesEncodeFormat) { @@ -1816,6 +1829,7 @@ func (m *sessionDataMutator) SetSearchPath(val sessiondata.SearchPath) { func (m *sessionDataMutator) SetLocation(loc *time.Location) { m.data.DataConversion.Location = loc + m.notifyOnDataChangeListeners("TimeZone", sessionDataTimeZoneFormat(loc)) } func (m *sessionDataMutator) SetReadOnly(val bool) { diff --git a/pkg/sql/logictest/testdata/logic_test/alter_column_type b/pkg/sql/logictest/testdata/logic_test/alter_column_type index 7f0b10bbeeb4..944af452c5cc 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_column_type +++ b/pkg/sql/logictest/testdata/logic_test/alter_column_type @@ -32,7 +32,7 @@ rowid INT8 false unique_rowid() ยท { query TTTT SELECT * FROM t ---- -some string short 0000-01-01 20:16:27 +0000 UTC 2018-05-23 22:16:27.658082 +0200 +0200 +some string short 0000-01-01 20:16:27 +0000 UTC 2018-05-23 22:16:27.658082 +0200 CEST statement ok DROP TABLE t diff --git a/pkg/sql/logictest/testdata/logic_test/datetime b/pkg/sql/logictest/testdata/logic_test/datetime index ed01264f1ddc..774c78502bef 100644 --- a/pkg/sql/logictest/testdata/logic_test/datetime +++ b/pkg/sql/logictest/testdata/logic_test/datetime @@ -954,7 +954,7 @@ SET TIME ZONE 'America/New_York' query T SELECT '2015-08-25 05:45:45-01:00'::timestamp::timestamptz ---- -2015-08-25 05:45:45 -0400 -0400 +2015-08-25 05:45:45 -0400 EDT query T SELECT '2015-08-25 05:45:45-01:00'::timestamptz::timestamp @@ -1196,17 +1196,17 @@ SHOW TIME ZONE query T SELECT DATE '1999-01-01' + INTERVAL '4 minutes' ---- -1999-01-01 00:04:00 +0000 UTC +1999-01-01 00:04:00 +0000 +0000 query T SELECT INTERVAL '4 minutes' + DATE '1999-01-01' ---- -1999-01-01 00:04:00 +0000 UTC +1999-01-01 00:04:00 +0000 +0000 query T SELECT DATE '1999-01-01' - INTERVAL '4 minutes' ---- -1998-12-31 23:56:00 +0000 UTC +1998-12-31 23:56:00 +0000 +0000 query B SELECT DATE '1999-01-02' < TIMESTAMPTZ '1999-01-01' @@ -1356,12 +1356,12 @@ SELECT date_trunc('month', ts) AS date_trunc_month_created_at FROM "topics"; query T SELECT date_trunc('month', tstz) AS date_trunc_month_created_at FROM "topics"; ---- -2017-12-01 00:00:00 +0000 UTC +2017-12-01 00:00:00 +0000 +0000 query T SELECT date_trunc('month', "date") AS date_trunc_month_created_at FROM "topics"; ---- -2017-12-01 00:00:00 +0000 UTC +2017-12-01 00:00:00 +0000 +0000 # Test date_trunc works when timestamp zone changes. subtest regression_41663 @@ -1369,12 +1369,12 @@ subtest regression_41663 query T select date_trunc('day', '2011-01-01 22:30:00'::date); ---- -2011-01-01 00:00:00 +0000 UTC +2011-01-01 00:00:00 +0000 +0000 query T select date_trunc('day', '2011-01-01 22:30:00+01:00'::timestamptz); ---- -2011-01-01 00:00:00 +0000 UTC +2011-01-01 00:00:00 +0000 +0000 statement ok SET TIME ZONE 'Africa/Nairobi' @@ -1382,7 +1382,7 @@ SET TIME ZONE 'Africa/Nairobi' query T select date_trunc('day', '2011-01-01 22:30:00'::date) ---- -2011-01-01 00:00:00 +0300 +0300 +2011-01-01 00:00:00 +0300 EAT query T select date_trunc('day', '2011-01-02 01:30:00'::timestamp) @@ -1392,7 +1392,7 @@ select date_trunc('day', '2011-01-02 01:30:00'::timestamp) query T select date_trunc('day', '2011-01-01 22:30:00+01:00'::timestamptz) ---- -2011-01-02 00:00:00 +0300 +0300 +2011-01-02 00:00:00 +0300 EAT statement ok SET TIME ZONE -5 diff --git a/pkg/sql/logictest/testdata/logic_test/prepare b/pkg/sql/logictest/testdata/logic_test/prepare index b0e11ccc4f2e..a69724205290 100644 --- a/pkg/sql/logictest/testdata/logic_test/prepare +++ b/pkg/sql/logictest/testdata/logic_test/prepare @@ -680,7 +680,7 @@ SET TIME ZONE 'EST'; query T EXECUTE change_loc ---- -2000-01-01 18:05:10.123 -0500 -0500 +2000-01-01 18:05:10.123 -0500 EST statement ok SET TIME ZONE 'UTC'; diff --git a/pkg/sql/logictest/testdata/logic_test/timestamp b/pkg/sql/logictest/testdata/logic_test/timestamp index 1aac5dabc572..cf6a2cb66294 100644 --- a/pkg/sql/logictest/testdata/logic_test/timestamp +++ b/pkg/sql/logictest/testdata/logic_test/timestamp @@ -48,7 +48,7 @@ SET TIME ZONE 'PST8PDT' query TT SELECT TIMESTAMP '2001-02-16 20:38:40' AT TIME ZONE 'MST', timezone(TIMESTAMP '2001-02-16 20:38:40', 'MST') ---- -2001-02-16 19:38:40 -0800 -0800 2001-02-16 19:38:40 -0800 -0800 +2001-02-16 19:38:40 -0800 PST 2001-02-16 19:38:40 -0800 PST query TT SELECT TIMESTAMP WITH TIME ZONE '2001-02-16 20:38:40-05' AT TIME ZONE 'MST', timezone(TIMESTAMP WITH TIME ZONE '2001-02-16 20:38:40-05', 'MST') diff --git a/pkg/sql/pgwire/conn.go b/pkg/sql/pgwire/conn.go index 5d0225e1ea1c..e8a140597daa 100644 --- a/pkg/sql/pgwire/conn.go +++ b/pkg/sql/pgwire/conn.go @@ -593,6 +593,13 @@ func (c *conn) sendStatusParam(param, value string) error { return c.msgBuilder.finishMsg(c.conn) } +func (c *conn) bufferStatusParam(param, value string) error { + c.msgBuilder.initMsg(pgwirebase.ServerMsgParameterStatus) + c.msgBuilder.writeTerminatedString(param) + c.msgBuilder.writeTerminatedString(value) + return c.msgBuilder.finishMsg(&c.writerState.buf) +} + func (c *conn) sendInitialConnData( ctx context.Context, sqlServer *sql.Server, ) (sql.ConnectionHandler, error) { @@ -610,10 +617,17 @@ func (c *conn) sendInitialConnData( // defaults with client-provided values. // For details see: https://www.postgresql.org/docs/10/static/libpq-status.html for _, param := range statusReportParams { + param := param 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 string) { + if err := c.bufferStatusParam(param, val); err != nil { + panic(fmt.Sprintf("unexpected error when trying to send status param update: %s", err.Error())) + } + }) } // The two following status parameters have no equivalent session // variable. diff --git a/pkg/sql/pgwire/testdata/pgtest/connection_params b/pkg/sql/pgwire/testdata/pgtest/connection_params new file mode 100644 index 000000000000..c65f0dad05e7 --- /dev/null +++ b/pkg/sql/pgwire/testdata/pgtest/connection_params @@ -0,0 +1,73 @@ +# Change the application name. + +send +Parse {"Query": "SET application_name = 'pgtest'"} +Bind +Execute +Sync +---- + +until +ReadyForQuery +---- +{"Type":"ParseComplete"} +{"Type":"BindComplete"} +{"Type":"ParameterStatus","Name":"application_name","Value":"pgtest"} +{"Type":"CommandComplete","CommandTag":"SET"} +{"Type":"ReadyForQuery","TxStatus":"I"} + +# Change the time zone using an offset. +# TODO(#42404): postgres has a different output. This is what we have right now +# as code, but we need to dig into what/why we use this format in param status. +# postgres: {"Type":"ParameterStatus","Name":"TimeZone","Value":"\u003c+06\u003e-06"} + +send +Parse {"Query": "SET TIME ZONE +6"} +Bind +Execute +Sync +---- + +until +ReadyForQuery +---- +{"Type":"ParseComplete"} +{"Type":"BindComplete"} +{"Type":"ParameterStatus","Name":"TimeZone","Value":"6"} +{"Type":"CommandComplete","CommandTag":"SET"} +{"Type":"ReadyForQuery","TxStatus":"I"} + +send +Parse {"Query": "SET TIME ZONE -11.5"} +Bind +Execute +Sync +---- + +# postgres: {"Type":"ParameterStatus","Name":"TimeZone","Value":"\u003c-11:30\u003e+11:30"} +until +ReadyForQuery +---- +{"Type":"ParseComplete"} +{"Type":"BindComplete"} +{"Type":"ParameterStatus","Name":"TimeZone","Value":"-11.5"} +{"Type":"CommandComplete","CommandTag":"SET"} +{"Type":"ReadyForQuery","TxStatus":"I"} + +# Change the time zone using a real string. + +send +Parse {"Query": "SET TIME ZONE 'America/New_York'"} +Bind +Execute +Sync +---- + +until +ReadyForQuery +---- +{"Type":"ParseComplete"} +{"Type":"BindComplete"} +{"Type":"ParameterStatus","Name":"TimeZone","Value":"America/New_York"} +{"Type":"CommandComplete","CommandTag":"SET"} +{"Type":"ReadyForQuery","TxStatus":"I"} diff --git a/pkg/sql/vars.go b/pkg/sql/vars.go index f6167461a0f4..e54c7e84ef04 100644 --- a/pkg/sql/vars.go +++ b/pkg/sql/vars.go @@ -702,16 +702,7 @@ var varGen = map[string]sessionVar{ // See https://www.postgresql.org/docs/10/static/runtime-config-client.html#GUC-TIMEZONE `timezone`: { Get: func(evalCtx *extendedEvalContext) string { - // If the time zone is a "fixed offset" one, initialized from an offset - // and not a standard name, then we use a magic format in the Location's - // name. We attempt to parse that here and retrieve the original offset - // specified by the user. - locStr := evalCtx.SessionData.DataConversion.Location.String() - _, origRepr, parsed := timeutil.ParseFixedOffsetTimeZone(locStr) - if parsed { - return origRepr - } - return locStr + return sessionDataTimeZoneFormat(evalCtx.SessionData.DataConversion.Location) }, GetStringVal: timeZoneVarGetStringVal, Set: timeZoneVarSet, @@ -868,6 +859,21 @@ func displayPgBool(val bool) func(_ *settings.Values) string { var globalFalse = displayPgBool(false) +// sessionDataTimeZoneFormat returns the appropriate timezone format +// to output when the `timezone` is required output. +// If the time zone is a "fixed offset" one, initialized from an offset +// and not a standard name, then we use a magic format in the Location's +// name. We attempt to parse that here and retrieve the original offset +// specified by the user. +func sessionDataTimeZoneFormat(loc *time.Location) string { + locStr := loc.String() + _, origRepr, parsed := timeutil.ParseFixedOffsetTimeZone(locStr) + if parsed { + return origRepr + } + return locStr +} + func makeCompatBoolVar(varName string, displayValue, anyValAllowed bool) sessionVar { displayValStr := formatBoolAsPostgresSetting(displayValue) return sessionVar{ diff --git a/pkg/util/timeutil/time_zone_util.go b/pkg/util/timeutil/time_zone_util.go index 55dd4222577f..b426fa826401 100644 --- a/pkg/util/timeutil/time_zone_util.go +++ b/pkg/util/timeutil/time_zone_util.go @@ -45,6 +45,8 @@ func TimeZoneStringToLocation(location string) (*time.Location, error) { // // The strings produced by FixedOffsetTimeZoneToLocation look like // " ()". +// TODO(#42404): this is not the format given by the results in +// pgwire/testdata/connection_params. func ParseFixedOffsetTimeZone(location string) (offset int, origRepr string, success bool) { if !strings.HasPrefix(location, fixedOffsetPrefix) { return 0, "", false