pgwire: pad char values with spaces if length is less than column width

Previously, when returning row values of a column which has type char,
the value wouldn't be padded with spaces on the right if its length
was less than the column width.

This is in contrast to Postgres, which pads values with spaces on the
right to match the column width. This change fixes this by checking
the Oid and width of the value being written, and padding it
appropriately if required.

Fixes cockroachdb#49639

Release note (bug fix): Previously, when streaming values from a column
declared of type char(n), the length of the value could be <= n. After
this change, all values streamed have length exactly n by padding
spaces to the right if necessary.

pkg/cmd/generate-binary/main.go

@@ -272,6 +272,11 @@ var inputs = map[string][]string{
 		"9223372036854775807",
 	},
 
+	"'%s'::char(8)": {
+		"hello",
+		"hello123",
+	},
+
 	"'%s'::timestamp": {
 		"1999-01-08 04:05:06+00",
 		"1999-01-08 04:05:06+00:00",

pkg/sql/pgwire/command_result.go

@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
+	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/errors"
 	"github.com/lib/pq/oid"
 )
@@ -75,9 +76,9 @@ type commandResult struct {
 	// to have an entry for every column.
 	formatCodes []pgwirebase.FormatCode
 
-	// oids is a map from result column index to its Oid, similar to formatCodes
-	// (except oids must always be set).
-	oids []oid.Oid
+	// types is a map from result column index to its type T, similar to formatCodes
+	// (except types must always be set).
+	types []*types.T
 
 	// bufferingDisabled is conditionally set during planning of certain
 	// statements.
@@ -178,7 +179,7 @@ func (r *commandResult) AddRow(ctx context.Context, row tree.Datums) error {
 	}
 	r.rowsAffected++
 
-	r.conn.bufferRow(ctx, row, r.formatCodes, r.conv, r.oids)
+	r.conn.bufferRow(ctx, row, r.formatCodes, r.conv, r.types)
 	var err error
 	if r.bufferingDisabled {
 		err = r.conn.Flush(r.pos)
@@ -219,9 +220,9 @@ func (r *commandResult) SetColumns(ctx context.Context, cols sqlbase.ResultColum
 	if r.descOpt == sql.NeedRowDesc {
 		_ /* err */ = r.conn.writeRowDescription(ctx, cols, r.formatCodes, &r.conn.writerState.buf)
 	}
-	r.oids = make([]oid.Oid, len(cols))
+	r.types = make([]*types.T, len(cols))
 	for i, col := range cols {
-		r.oids[i] = col.Typ.Oid()
+		r.types[i] = col.Typ
 	}
 }
 

pkg/sql/pgwire/conn.go

@@ -1111,7 +1111,7 @@ func (c *conn) bufferRow(
 	row tree.Datums,
 	formatCodes []pgwirebase.FormatCode,
 	conv sessiondata.DataConversionConfig,
-	oids []oid.Oid,
+	types []*types.T,
 ) {
 	c.msgBuilder.initMsg(pgwirebase.ServerMsgDataRow)
 	c.msgBuilder.putInt16(int16(len(row)))
@@ -1122,9 +1122,9 @@ func (c *conn) bufferRow(
 		}
 		switch fmtCode {
 		case pgwirebase.FormatText:
-			c.msgBuilder.writeTextDatum(ctx, col, conv)
+			c.msgBuilder.writeTextDatum(ctx, col, conv, types[i])
 		case pgwirebase.FormatBinary:
-			c.msgBuilder.writeBinaryDatum(ctx, col, conv.Location, oids[i])
+			c.msgBuilder.writeBinaryDatum(ctx, col, conv.Location, types[i])
 		default:
 			c.msgBuilder.setError(errors.Errorf("unsupported format code %s", fmtCode))
 		}

pkg/sql/pgwire/conn_test.go

@@ -364,8 +364,8 @@ func sendResult(
 	for _, row := range rows {
 		c.msgBuilder.initMsg(pgwirebase.ServerMsgDataRow)
 		c.msgBuilder.putInt16(int16(len(row)))
-		for _, col := range row {
-			c.msgBuilder.writeTextDatum(ctx, col, defaultConv)
+		for i, col := range row {
+			c.msgBuilder.writeTextDatum(ctx, col, defaultConv, cols[i].Typ)
 		}
 
 		if err := c.msgBuilder.finishMsg(c.conn); err != nil {

pkg/sql/pgwire/encoding_test.go

@@ -37,6 +37,7 @@ type encodingTest struct {
 	SQL          string
 	Datum        tree.Datum
 	Oid          oid.Oid
+	T            *types.T
 	Text         string
 	TextAsBinary []byte
 	Binary       []byte
@@ -84,6 +85,21 @@ func readEncodingTests(t testing.TB) []*encodingTest {
 			t.Fatal(err)
 		}
 		tc.Datum = d
+
+		// Annotate with the type.
+		tc.T = types.OidToType[tc.Oid]
+		if tc.T == nil {
+			t.Fatalf("Unknown Oid %d not found in the OidToType map", tc.Oid)
+		}
+
+		// Populate specific type information based on OID and the specific test
+		//case.
+		switch tc.T.Oid() {
+		case oid.T_bpchar:
+			// The width of a bpchar type is fixed and equal to the length of the
+			// Text string returned by postgres.
+			tc.T.InternalType.Width = int32(len(tc.Text))
+		}
 	}
 
 	return tests
@@ -126,7 +142,7 @@ func TestEncodings(t *testing.T) {
 
 				buf.reset()
 				buf.textFormatter.Buffer.Reset()
-				buf.writeTextDatum(ctx, d, conv)
+				buf.writeTextDatum(ctx, d, conv, tc.T)
 				if buf.err != nil {
 					t.Fatal(buf.err)
 				}
@@ -140,7 +156,7 @@ func TestEncodings(t *testing.T) {
 			for _, tc := range tests {
 				d := tc.Datum
 				buf.reset()
-				buf.writeBinaryDatum(ctx, d, time.UTC, tc.Oid)
+				buf.writeBinaryDatum(ctx, d, time.UTC, tc.T)
 				if buf.err != nil {
 					t.Fatal(buf.err)
 				}
@@ -249,13 +265,13 @@ func BenchmarkEncodings(b *testing.B) {
 				for i := 0; i < b.N; i++ {
 					buf.reset()
 					buf.textFormatter.Buffer.Reset()
-					buf.writeTextDatum(ctx, d, conv)
+					buf.writeTextDatum(ctx, d, conv, tc.T)
 				}
 			})
 			b.Run("binary", func(b *testing.B) {
 				for i := 0; i < b.N; i++ {
 					buf.reset()
-					buf.writeBinaryDatum(ctx, d, time.UTC, tc.Oid)
+					buf.writeBinaryDatum(ctx, d, time.UTC, tc.T)
 				}
 			})
 		})
@@ -267,7 +283,7 @@ func TestEncodingErrorCounts(t *testing.T) {
 
 	buf := newWriteBuffer(metric.NewCounter(metric.Metadata{}))
 	d, _ := tree.ParseDDecimal("Inf")
-	buf.writeBinaryDatum(context.Background(), d, nil, d.ResolvedType().Oid())
+	buf.writeBinaryDatum(context.Background(), d, nil, d.ResolvedType())
 	if count := telemetry.GetRawFeatureCounts()["pgwire.#32489.binary_decimal_infinity"]; count != 1 {
 		t.Fatalf("expected 1 encoding error, got %d", count)
 	}

pkg/sql/pgwire/pgwirebase/encoding.go

@@ -17,6 +17,7 @@ import (
 	"io"
 	"math"
 	"strconv"
+	"strings"
 	"time"
 	"unicode/utf8"
 	"unsafe"
@@ -671,11 +672,18 @@ func DecodeOidDatum(
 
 	// Types with identical text/binary handling.
 	switch id {
-	case oid.T_text, oid.T_varchar, oid.T_bpchar:
+	case oid.T_text, oid.T_varchar:
 		if err := validateStringBytes(b); err != nil {
 			return nil, err
 		}
 		return tree.NewDString(string(b)), nil
+	case oid.T_bpchar:
+		if err := validateStringBytes(b); err != nil {
+			return nil, err
+		}
+		// Trim the trailing spaces
+		sv := strings.TrimRight(string(b), " ")
+		return tree.NewDString(sv), nil
 	case oid.T_name:
 		if err := validateStringBytes(b); err != nil {
 			return nil, err

pkg/sql/pgwire/testdata/encodings.json

@@ -41,6 +41,20 @@
 		"TextAsBinary": [123, 125],
 		"Binary": [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 23]
 	},
+	{
+		"SQL": "'hello'::char(8)",
+		"Oid": 1042,
+		"Text": "hello   ",
+		"TextAsBinary": [104, 101, 108, 108, 111, 32, 32, 32],
+		"Binary": [104, 101, 108, 108, 111, 32, 32, 32]
+	},
+	{
+		"SQL": "'hello123'::char(8)",
+		"Oid": 1042,
+		"Text": "hello123",
+		"TextAsBinary": [104, 101, 108, 108, 111, 49, 50, 51],
+		"Binary": [104, 101, 108, 108, 111, 49, 50, 51]
+	},
 	{
 		"SQL": "'1999-01-08'::date",
 		"Oid": 1082,

pkg/sql/pgwire/testdata/pgtest/row_description

@@ -171,16 +171,11 @@ RowDescription
 # The following discrepancy is a bug.
 # See: https://github.com/cockroachdb/cockroach/issues/49639
 
-until noncrdb_only
+until
 DataRow
 ----
 {"Type":"DataRow","Values":[{"text":"hello   "}]}
 
-until crdb_only
-DataRow
-----
-{"Type":"DataRow","Values":[{"text":"hello"}]}
-
 until
 ReadyForQuery
 ----
@@ -217,16 +212,11 @@ RowDescription
 # The following discrepancy is the first bug above.
 # See: https://github.com/cockroachdb/cockroach/issues/49639
 
-until noncrdb_only
+until
 DataRow
 ----
 {"Type":"DataRow","Values":[{"text":"hello   "}]}
 
-until crdb_only
-DataRow
-----
-{"Type":"DataRow","Values":[{"text":"hello"}]}
-
 until
 ReadyForQuery
 ----

pkg/sql/pgwire/types.go

@@ -13,6 +13,7 @@ package pgwire
 import (
 	"context"
 	"encoding/binary"
+	"fmt"
 	"math"
 	"math/big"
 	"net"
@@ -64,8 +65,22 @@ func pgTypeForParserType(t *types.T) pgType {
 	}
 }
 
+// resolveBlankPaddedChar pads the given string with spaces if blank padding is
+// required or returns the string unmodified otherwise.
+func resolveBlankPaddedChar(s string, t *types.T) string {
+	if t.Oid() == oid.T_bpchar {
+		// Pad spaces on the right of the string to make it of length specified in
+		// the type t.
+		return fmt.Sprintf("%-*v", t.Width(), s)
+	}
+	return s
+}
+
+// writeTextDatum writes d to the buffer. Type t must be specified for types
+// that have various width encodings and therefore need padding (chars).
+// It is ignored (and can be nil) for types which do not need padding.
 func (b *writeBuffer) writeTextDatum(
-	ctx context.Context, d tree.Datum, conv sessiondata.DataConversionConfig,
+	ctx context.Context, d tree.Datum, conv sessiondata.DataConversionConfig, t *types.T,
 ) {
 	if log.V(2) {
 		log.Infof(ctx, "pgwire writing TEXT datum of type: %T, %#v", d, d)
@@ -116,7 +131,7 @@ func (b *writeBuffer) writeTextDatum(
 		b.writeLengthPrefixedString(v.IPAddr.String())
 
 	case *tree.DString:
-		b.writeLengthPrefixedString(string(*v))
+		b.writeLengthPrefixedString(resolveBlankPaddedChar(string(*v), t))
 
 	case *tree.DCollatedString:
 		b.writeLengthPrefixedString(v.Contents)
@@ -187,11 +202,11 @@ func (b *writeBuffer) writeTextDatum(
 	}
 }
 
-// writeBinaryDatum writes d to the buffer. Oid must be specified for types
-// that have various width encodings. It is ignored (and can be 0) for types
-// with a 1:1 datum:oid mapping.
+// writeBinaryDatum writes d to the buffer. Type t must be specified for types
+// that have various width encodings (floats, ints, chars). It is ignored
+// (and can be nil) for types with a 1:1 datum:type mapping.
 func (b *writeBuffer) writeBinaryDatum(
-	ctx context.Context, d tree.Datum, sessionLoc *time.Location, Oid oid.Oid,
+	ctx context.Context, d tree.Datum, sessionLoc *time.Location, t *types.T,
 ) {
 	if log.V(2) {
 		log.Infof(ctx, "pgwire writing BINARY datum of type: %T, %#v", d, d)
@@ -242,7 +257,7 @@ func (b *writeBuffer) writeBinaryDatum(
 		}
 
 	case *tree.DInt:
-		switch Oid {
+		switch t.Oid() {
 		case oid.T_int2:
 			b.putInt32(2)
 			b.putInt16(int16(*v))
@@ -253,19 +268,19 @@ func (b *writeBuffer) writeBinaryDatum(
 			b.putInt32(8)
 			b.putInt64(int64(*v))
 		default:
-			b.setError(errors.Errorf("unsupported int oid: %v", Oid))
+			b.setError(errors.Errorf("unsupported int oid: %v", t.Oid()))
 		}
 
 	case *tree.DFloat:
-		switch Oid {
+		switch t.Oid() {
 		case oid.T_float4:
 			b.putInt32(4)
 			b.putInt32(int32(math.Float32bits(float32(*v))))
 		case oid.T_float8:
 			b.putInt32(8)
 			b.putInt64(int64(math.Float64bits(float64(*v))))
 		default:
-			b.setError(errors.Errorf("unsupported float oid: %v", Oid))
+			b.setError(errors.Errorf("unsupported float oid: %v", t.Oid()))
 		}
 
 	case *tree.DDecimal:
@@ -398,7 +413,7 @@ func (b *writeBuffer) writeBinaryDatum(
 		b.writeLengthPrefixedString(v.LogicalRep)
 
 	case *tree.DString:
-		b.writeLengthPrefixedString(string(*v))
+		b.writeLengthPrefixedString(resolveBlankPaddedChar(string(*v), t))
 
 	case *tree.DCollatedString:
 		b.writeLengthPrefixedString(v.Contents)
@@ -438,7 +453,7 @@ func (b *writeBuffer) writeBinaryDatum(
 		for _, elem := range v.D {
 			oid := elem.ResolvedType().Oid()
 			subWriter.putInt32(int32(oid))
-			subWriter.writeBinaryDatum(ctx, elem, sessionLoc, oid)
+			subWriter.writeBinaryDatum(ctx, elem, sessionLoc, elem.ResolvedType())
 		}
 		b.writeLengthPrefixedBuffer(&subWriter.wrapped)
 
@@ -476,7 +491,7 @@ func (b *writeBuffer) writeBinaryDatum(
 			// Lower bound, we only support a lower bound of 1.
 			subWriter.putInt32(1)
 			for _, elem := range v.Array {
-				subWriter.writeBinaryDatum(ctx, elem, sessionLoc, oid)
+				subWriter.writeBinaryDatum(ctx, elem, sessionLoc, v.ParamTyp)
 			}
 		}
 		b.writeLengthPrefixedBuffer(&subWriter.wrapped)