pgwire: only show TZ minutes offset if non-zero

Similar to cockroachdb#57265, this commit matches the PG behavior to only show the minutes offset if it is non-zero. Release note (bug fix): Minute timezone offsets are only displayed in the wire protocol if they are non-zero for TimestampTZ and TimeTZ values. Previously, they would always display. Release note (bug fix): Binary TimeTZ values were not being decoded correctly when being sent as a parameter in the wire protocol. This is now fixed.
rafiss · May 12, 2021 · 9b9407e · 9b9407e
1 parent e1bd9cb
commit 9b9407e
Show file tree

Hide file tree

Showing 5 changed files with 194 additions and 12 deletions.
diff --git a/pkg/cmd/generate-binary/main.go b/pkg/cmd/generate-binary/main.go
@@ -302,7 +302,6 @@ var inputs = map[string][]string{
 		"9004-10-19 10:23:54",
 	},
 
-	/* TODO(mjibson): fix these; there's a slight timezone display difference
 	"'%s'::timestamptz": {
 		"1999-01-08 04:05:06+00",
 		"1999-01-08 04:05:06+00:00",
@@ -317,7 +316,21 @@ var inputs = map[string][]string{
 		"4004-10-19 10:23:54",
 		"9004-10-19 10:23:54",
 	},
-	*/
+
+	"'%s'::timetz": {
+		"04:05:06+00",
+		"04:05:06+00:00",
+		"04:05:06+10",
+		"04:05:06+10:00",
+		"04:05:06+10:30",
+		"04:05:06",
+		"10:23:54",
+		"00:00:00",
+		"10:23:54",
+		"10:23:54 BC",
+		"10:23:54",
+		"10:23:54",
+	},
 
 	"'%s'::date": {
 		"1999-01-08",

diff --git a/pkg/sql/pgwire/pgwirebase/encoding.go b/pkg/sql/pgwire/pgwirebase/encoding.go
@@ -683,7 +683,7 @@ func DecodeDatum(
 				return nil, pgerror.Newf(pgcode.Syntax, "timetz requires 12 bytes for binary format")
 			}
 			timeOfDayMicros := int64(binary.BigEndian.Uint64(b))
-			offsetSecs := int32(binary.BigEndian.Uint32(b))
+			offsetSecs := int32(binary.BigEndian.Uint32(b[8:]))
 			return tree.NewDTimeTZFromOffset(timeofday.TimeOfDay(timeOfDayMicros), offsetSecs), nil
 		case oid.T_interval:
 			if len(b) < 16 {

diff --git a/pkg/sql/pgwire/testdata/encodings.json b/pkg/sql/pgwire/testdata/encodings.json
@@ -1700,6 +1700,174 @@
 		"TextAsBinary": [57, 48, 48, 52, 45, 49, 48, 45, 49, 57, 32, 49, 48, 58, 50, 51, 58, 53, 52],
 		"Binary": [3, 17, 83, 233, 31, 54, 66, 128]
 	},
+	{
+		"SQL": "'1999-01-08 04:05:06+00'::timestamptz",
+		"Oid": 1184,
+		"Text": "1999-01-08 04:05:06+00",
+		"TextAsBinary": [49, 57, 57, 57, 45, 48, 49, 45, 48, 56, 32, 48, 52, 58, 48, 53, 58, 48, 54, 43, 48, 48],
+		"Binary": [255, 255, 227, 225, 177, 91, 128, 128]
+	},
+	{
+		"SQL": "'1999-01-08 04:05:06+00:00'::timestamptz",
+		"Oid": 1184,
+		"Text": "1999-01-08 04:05:06+00",
+		"TextAsBinary": [49, 57, 57, 57, 45, 48, 49, 45, 48, 56, 32, 48, 52, 58, 48, 53, 58, 48, 54, 43, 48, 48],
+		"Binary": [255, 255, 227, 225, 177, 91, 128, 128]
+	},
+	{
+		"SQL": "'1999-01-08 04:05:06+10'::timestamptz",
+		"Oid": 1184,
+		"Text": "1999-01-07 18:05:06+00",
+		"TextAsBinary": [49, 57, 57, 57, 45, 48, 49, 45, 48, 55, 32, 49, 56, 58, 48, 53, 58, 48, 54, 43, 48, 48],
+		"Binary": [255, 255, 227, 217, 79, 151, 24, 128]
+	},
+	{
+		"SQL": "'1999-01-08 04:05:06+10:00'::timestamptz",
+		"Oid": 1184,
+		"Text": "1999-01-07 18:05:06+00",
+		"TextAsBinary": [49, 57, 57, 57, 45, 48, 49, 45, 48, 55, 32, 49, 56, 58, 48, 53, 58, 48, 54, 43, 48, 48],
+		"Binary": [255, 255, 227, 217, 79, 151, 24, 128]
+	},
+	{
+		"SQL": "'1999-01-08 04:05:06+10:30'::timestamptz",
+		"Oid": 1184,
+		"Text": "1999-01-07 17:35:06+00",
+		"TextAsBinary": [49, 57, 57, 57, 45, 48, 49, 45, 48, 55, 32, 49, 55, 58, 51, 53, 58, 48, 54, 43, 48, 48],
+		"Binary": [255, 255, 227, 216, 228, 77, 70, 128]
+	},
+	{
+		"SQL": "'1999-01-08 04:05:06'::timestamptz",
+		"Oid": 1184,
+		"Text": "1999-01-08 04:05:06+00",
+		"TextAsBinary": [49, 57, 57, 57, 45, 48, 49, 45, 48, 56, 32, 48, 52, 58, 48, 53, 58, 48, 54, 43, 48, 48],
+		"Binary": [255, 255, 227, 225, 177, 91, 128, 128]
+	},
+	{
+		"SQL": "'2004-10-19 10:23:54'::timestamptz",
+		"Oid": 1184,
+		"Text": "2004-10-19 10:23:54+00",
+		"TextAsBinary": [50, 48, 48, 52, 45, 49, 48, 45, 49, 57, 32, 49, 48, 58, 50, 51, 58, 53, 52, 43, 48, 48],
+		"Binary": [0, 0, 137, 201, 15, 13, 226, 128]
+	},
+	{
+		"SQL": "'0001-01-01 00:00:00'::timestamptz",
+		"Oid": 1184,
+		"Text": "0001-01-01 00:00:00+00",
+		"TextAsBinary": [48, 48, 48, 49, 45, 48, 49, 45, 48, 49, 32, 48, 48, 58, 48, 48, 58, 48, 48, 43, 48, 48],
+		"Binary": [255, 31, 226, 255, 197, 156, 96, 0]
+	},
+	{
+		"SQL": "'0004-10-19 10:23:54'::timestamptz",
+		"Oid": 1184,
+		"Text": "0004-10-19 10:23:54+00",
+		"TextAsBinary": [48, 48, 48, 52, 45, 49, 48, 45, 49, 57, 32, 49, 48, 58, 50, 51, 58, 53, 52, 43, 48, 48],
+		"Binary": [255, 32, 80, 6, 42, 191, 2, 128]
+	},
+	{
+		"SQL": "'0004-10-19 10:23:54 BC'::timestamptz",
+		"Oid": 1184,
+		"Text": "0004-10-19 10:23:54+00 BC",
+		"TextAsBinary": [48, 48, 48, 52, 45, 49, 48, 45, 49, 57, 32, 49, 48, 58, 50, 51, 58, 53, 52, 43, 48, 48, 32, 66, 67],
+		"Binary": [255, 31, 135, 24, 26, 133, 34, 128]
+	},
+	{
+		"SQL": "'4004-10-19 10:23:54'::timestamptz",
+		"Oid": 1184,
+		"Text": "4004-10-19 10:23:54+00",
+		"TextAsBinary": [52, 48, 48, 52, 45, 49, 48, 45, 49, 57, 32, 49, 48, 58, 50, 51, 58, 53, 52, 43, 48, 48],
+		"Binary": [0, 224, 195, 139, 243, 92, 194, 128]
+	},
+	{
+		"SQL": "'9004-10-19 10:23:54'::timestamptz",
+		"Oid": 1184,
+		"Text": "9004-10-19 10:23:54+00",
+		"TextAsBinary": [57, 48, 48, 52, 45, 49, 48, 45, 49, 57, 32, 49, 48, 58, 50, 51, 58, 53, 52, 43, 48, 48],
+		"Binary": [3, 17, 83, 233, 31, 54, 66, 128]
+	},
+	{
+		"SQL": "'04:05:06+00'::timetz",
+		"Oid": 1266,
+		"Text": "04:05:06+00",
+		"TextAsBinary": [48, 52, 58, 48, 53, 58, 48, 54, 43, 48, 48],
+		"Binary": [0, 0, 0, 3, 108, 139, 192, 128, 0, 0, 0, 0]
+	},
+	{
+		"SQL": "'04:05:06+00:00'::timetz",
+		"Oid": 1266,
+		"Text": "04:05:06+00",
+		"TextAsBinary": [48, 52, 58, 48, 53, 58, 48, 54, 43, 48, 48],
+		"Binary": [0, 0, 0, 3, 108, 139, 192, 128, 0, 0, 0, 0]
+	},
+	{
+		"SQL": "'04:05:06+10'::timetz",
+		"Oid": 1266,
+		"Text": "04:05:06+10",
+		"TextAsBinary": [48, 52, 58, 48, 53, 58, 48, 54, 43, 49, 48],
+		"Binary": [0, 0, 0, 3, 108, 139, 192, 128, 255, 255, 115, 96]
+	},
+	{
+		"SQL": "'04:05:06+10:00'::timetz",
+		"Oid": 1266,
+		"Text": "04:05:06+10",
+		"TextAsBinary": [48, 52, 58, 48, 53, 58, 48, 54, 43, 49, 48],
+		"Binary": [0, 0, 0, 3, 108, 139, 192, 128, 255, 255, 115, 96]
+	},
+	{
+		"SQL": "'04:05:06+10:30'::timetz",
+		"Oid": 1266,
+		"Text": "04:05:06+10:30",
+		"TextAsBinary": [48, 52, 58, 48, 53, 58, 48, 54, 43, 49, 48, 58, 51, 48],
+		"Binary": [0, 0, 0, 3, 108, 139, 192, 128, 255, 255, 108, 88]
+	},
+	{
+		"SQL": "'04:05:06'::timetz",
+		"Oid": 1266,
+		"Text": "04:05:06+00",
+		"TextAsBinary": [48, 52, 58, 48, 53, 58, 48, 54, 43, 48, 48],
+		"Binary": [0, 0, 0, 3, 108, 139, 192, 128, 0, 0, 0, 0]
+	},
+	{
+		"SQL": "'10:23:54'::timetz",
+		"Oid": 1266,
+		"Text": "10:23:54+00",
+		"TextAsBinary": [49, 48, 58, 50, 51, 58, 53, 52, 43, 48, 48],
+		"Binary": [0, 0, 0, 8, 183, 61, 130, 128, 0, 0, 0, 0]
+	},
+	{
+		"SQL": "'00:00:00'::timetz",
+		"Oid": 1266,
+		"Text": "00:00:00+00",
+		"TextAsBinary": [48, 48, 58, 48, 48, 58, 48, 48, 43, 48, 48],
+		"Binary": [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
+	},
+	{
+		"SQL": "'10:23:54'::timetz",
+		"Oid": 1266,
+		"Text": "10:23:54+00",
+		"TextAsBinary": [49, 48, 58, 50, 51, 58, 53, 52, 43, 48, 48],
+		"Binary": [0, 0, 0, 8, 183, 61, 130, 128, 0, 0, 0, 0]
+	},
+	{
+		"SQL": "'10:23:54 BC'::timetz",
+		"Oid": 1266,
+		"Text": "10:23:54+00",
+		"TextAsBinary": [49, 48, 58, 50, 51, 58, 53, 52, 43, 48, 48],
+		"Binary": [0, 0, 0, 8, 183, 61, 130, 128, 0, 0, 0, 0]
+	},
+	{
+		"SQL": "'10:23:54'::timetz",
+		"Oid": 1266,
+		"Text": "10:23:54+00",
+		"TextAsBinary": [49, 48, 58, 50, 51, 58, 53, 52, 43, 48, 48],
+		"Binary": [0, 0, 0, 8, 183, 61, 130, 128, 0, 0, 0, 0]
+	},
+	{
+		"SQL": "'10:23:54'::timetz",
+		"Oid": 1266,
+		"Text": "10:23:54+00",
+		"TextAsBinary": [49, 48, 58, 50, 51, 58, 53, 52, 43, 48, 48],
+		"Binary": [0, 0, 0, 8, 183, 61, 130, 128, 0, 0, 0, 0]
+	},
 	{
 		"SQL": "'{00000000-0000-0000-0000-000000000000}'::uuid[]",
 		"Oid": 2951,

diff --git a/pkg/sql/pgwire/testdata/pgtest/timezone b/pkg/sql/pgwire/testdata/pgtest/timezone
@@ -33,16 +33,15 @@ ReadyForQuery
 {"Type":"CommandComplete","CommandTag":"SELECT 1"}
 {"Type":"ReadyForQuery","TxStatus":"I"}
 
-# PostgreSQL does not display seconds offset here, but CockroachDB does.
-send crdb_only
+send
 Query {"String": "SELECT '1882-05-23T00:00:00'::\"timestamptz\""}
 ----
 
-until crdb_only ignore_data_type_sizes
+until ignore_data_type_sizes
 ReadyForQuery
 ----
 {"Type":"RowDescription","Fields":[{"Name":"timestamptz","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":1184,"DataTypeSize":0,"TypeModifier":-1,"Format":0}]}
-{"Type":"DataRow","Values":[{"text":"1882-05-23 00:00:00+00:00"}]}
+{"Type":"DataRow","Values":[{"text":"1882-05-23 00:00:00+00"}]}
 {"Type":"CommandComplete","CommandTag":"SELECT 1"}
 {"Type":"ReadyForQuery","TxStatus":"I"}
 

diff --git a/pkg/sql/pgwire/types.go b/pkg/sql/pgwire/types.go
@@ -533,10 +533,10 @@ func (b *writeBuffer) writeBinaryDatum(
 
 const (
 	pgTimeFormat              = "15:04:05.999999"
-	pgTimeTZFormat            = pgTimeFormat + "-07:00"
+	pgTimeTZFormat            = pgTimeFormat + "-07"
 	pgDateFormat              = "2006-01-02"
 	pgTimeStampFormatNoOffset = pgDateFormat + " " + pgTimeFormat
-	pgTimeStampFormat         = pgTimeStampFormatNoOffset + "-07:00"
+	pgTimeStampFormat         = pgTimeStampFormatNoOffset + "-07"
 	pgTime2400Format          = "24:00:00"
 )
 
@@ -554,11 +554,11 @@ func formatTime(t timeofday.TimeOfDay, tmp []byte) []byte {
 // formatTimeTZ formats t into a format lib/pq understands, appending to the
 // provided tmp buffer and reallocating if needed. The function will then return
 // the resulting buffer.
-// Note it does not understand the "second" component of the offset as lib/pq
-// cannot parse it.
 func formatTimeTZ(t timetz.TimeTZ, tmp []byte) []byte {
 	format := pgTimeTZFormat
 	if t.OffsetSecs%60 != 0 {
+		format += ":00:00"
+	} else if t.OffsetSecs%3600 != 0 {
 		format += ":00"
 	}
 	ret := t.ToTime().AppendFormat(tmp, format)
@@ -577,7 +577,9 @@ func formatTs(t time.Time, offset *time.Location, tmp []byte) (b []byte) {
 	var format string
 	if offset != nil {
 		format = pgTimeStampFormat
-		if _, offset := t.In(offset).Zone(); offset%60 != 0 {
+		if _, offsetSeconds := t.In(offset).Zone(); offsetSeconds%60 != 0 {
+			format += ":00:00"
+		} else if offsetSeconds%3600 != 0 {
 			format += ":00"
 		}
 	} else {