sql,sqlbase: fix the introspection for pre-2.1 BIT columns

Prior to CockroachDB 2.1, a BIT column was defined in column
descriptors using column type INT and a non-canonical type width.

When such a pre-2.1 column descriptor is loaded in a post-2.1
cockroachdb cluster (eg during an upgrade) the BIT column is
"upgraded" to INT, which is value-wise compatible.

However the introspection logic was not updated accordingly, and these
pre-2.1 BIT columns would show up in `information_schema.columns`
using mangled values.

Release note (bug fix): The value of
`information_schema.columns.character_maximum_column` is set to NULL
for all integer types, for compatibility with PostgreSQL.

Release note (bug fix): The values reported in
`information_schema.columns` for integer columns created prior to
CockroachDB v2.1 as BIT (back when CockroachDB mis-defined BIT as a
special-case integer) are now fixed and consistent with other integer
types.

pkg/sql/colencoding/key_encoding.go

@@ -310,10 +310,10 @@ func UnmarshalColumnValueToCol(
 			vec.Int16()[idx] = int16(v)
 		case 32:
 			vec.Int32()[idx] = int32(v)
-		case 0, 64:
-			vec.Int64()[idx] = v
 		default:
-			return errors.Errorf("invalid int width: %d", typ.Width)
+			// Pre-2.1 BIT was using INT encoding with arbitrary sizes.
+			// We map these to 64-bit INT now. See #34161.
+			vec.Int64()[idx] = v
 		}
 	case sqlbase.ColumnType_FLOAT:
 		var v float64

pkg/sql/colencoding/value_encoding.go

@@ -89,10 +89,10 @@ func decodeUntaggedDatumToCol(
 			vec.Int16()[idx] = int16(i)
 		case 32:
 			vec.Int32()[idx] = int32(i)
-		case 0, 64:
-			vec.Int64()[idx] = i
 		default:
-			return buf, errors.Errorf("unknown integer width %d", t.Width)
+			// Pre-2.1 BIT was using INT encoding with arbitrary sizes.
+			// We map these to 64-bit INT now. See #34161.
+			vec.Int64()[idx] = i
 		}
 	default:
 		return buf, errors.Errorf("couldn't decode type %s", t)

pkg/sql/crdb_internal_test.go

@@ -21,11 +21,16 @@ import (
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/gossip"
+	"github.com/cockroachdb/cockroach/pkg/internal/client"
 	"github.com/cockroachdb/cockroach/pkg/server/status/statuspb"
 	"github.com/cockroachdb/cockroach/pkg/sql"
+	"github.com/cockroachdb/cockroach/pkg/sql/parser"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
 	"github.com/cockroachdb/cockroach/pkg/sql/tests"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/jackc/pgx/pgtype"
 )
 
 func TestGossipAlertsTable(t *testing.T) {
@@ -61,3 +66,142 @@ func TestGossipAlertsTable(t *testing.T) {
 		t.Fatalf("got:\n%s\nexpected:\n%s", a, e)
 	}
 }
+
+// TestOldBitColumnMetadata checks that a pre-2.1 BIT columns
+// shows up properly in metadata post-2.1.
+func TestOldBitColumnMetadata(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	// The descriptor changes made must have an immediate effect
+	// so disable leases on tables.
+	defer sql.TestDisableTableLeases()()
+
+	params, _ := tests.CreateTestServerParams()
+	s, sqlDB, kvDB := serverutils.StartServer(t, params)
+	defer s.Stopper().Stop(context.TODO())
+
+	if _, err := sqlDB.Exec(`
+CREATE DATABASE t;
+CREATE TABLE t.test (k INT);
+`); err != nil {
+		t.Fatal(err)
+	}
+
+	// We now want to create a pre-2.1 table descriptor with an
+	// old-style bit column. We're going to edit the table descriptor
+	// manually, without going through SQL.
+	tableDesc := sqlbase.GetTableDescriptor(kvDB, "t", "test")
+	for i := range tableDesc.Columns {
+		if tableDesc.Columns[i].Name == "k" {
+			tableDesc.Columns[i].Type.VisibleType = 4 // Pre-2.1 BIT.
+			tableDesc.Columns[i].Type.Width = 12      // Arbitrary non-std INT size.
+			break
+		}
+	}
+	// To make this test future-proof we must ensure that there isn't
+	// any logic in an unrelated place which will prevent the table from
+	// being committed. To verify this, we add another column and check
+	// it appears in introspection afterwards.
+	//
+	// We also avoid the regular schema change logic entirely, because
+	// this may be equipped with code to "fix" the old-style BIT column
+	// we defined above.
+	alterCmd, err := parser.ParseOne("ALTER TABLE t ADD COLUMN z INT")
+	if err != nil {
+		t.Fatal(err)
+	}
+	colDef := alterCmd.AST.(*tree.AlterTable).Cmds[0].(*tree.AlterTableAddColumn).ColumnDef
+	col, _, _, err := sqlbase.MakeColumnDefDescs(colDef, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	col.ID = tableDesc.NextColumnID
+	tableDesc.NextColumnID++
+	tableDesc.Families[0].ColumnNames = append(tableDesc.Families[0].ColumnNames, col.Name)
+	tableDesc.Families[0].ColumnIDs = append(tableDesc.Families[0].ColumnIDs, col.ID)
+	tableDesc.Columns = append(tableDesc.Columns, *col)
+
+	// Write the modified descriptor.
+	if err := kvDB.Txn(context.TODO(), func(ctx context.Context, txn *client.Txn) error {
+		if err := txn.SetSystemConfigTrigger(); err != nil {
+			return err
+		}
+		return txn.Put(ctx, sqlbase.MakeDescMetadataKey(tableDesc.ID), sqlbase.WrapDescriptor(tableDesc))
+	}); err != nil {
+		t.Fatal(err)
+	}
+
+	// Read the column metadata from information_schema.
+	rows, err := sqlDB.Query(`
+SELECT column_name, character_maximum_length, numeric_precision, numeric_precision_radix, crdb_sql_type
+  FROM t.information_schema.columns
+ WHERE table_catalog = 't' AND table_schema = 'public' AND table_name = 'test'
+   AND column_name != 'rowid'`)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer rows.Close()
+
+	expected := 0
+	for rows.Next() {
+		var colName string
+		var charMaxLength, numPrec, numPrecRadix pgtype.Int8
+		var sqlType string
+		if err := rows.Scan(&colName, &charMaxLength, &numPrec, &numPrecRadix, &sqlType); err != nil {
+			t.Fatal(err)
+		}
+		switch colName {
+		case "k":
+			if charMaxLength.Status != pgtype.Null {
+				t.Fatalf("x character_maximum_length: expected null, got %d", charMaxLength.Int)
+			}
+			if numPrec.Int != 64 {
+				t.Fatalf("x numeric_precision: expected 64, got %v", numPrec.Get())
+			}
+			if numPrecRadix.Int != 2 {
+				t.Fatalf("x numeric_precision_radix: expected 64, got %v", numPrecRadix.Get())
+			}
+			if sqlType != "INT8" {
+				t.Fatalf("x crdb_sql_type: expected INT8, got %q", sqlType)
+			}
+			expected |= 2
+		case "z":
+			// This is just a canary to verify that the manually-modified
+			// table descriptor is visible to introspection.
+			expected |= 1
+		default:
+			t.Fatalf("unexpected col: %q", colName)
+		}
+	}
+	if expected != 3 {
+		t.Fatal("did not find both expected rows")
+	}
+
+	// Now test the workaround: using ALTER to "upgrade" the type fully to INT.
+	if _, err := sqlDB.Exec(`ALTER TABLE t.test ALTER COLUMN k SET DATA TYPE INT8`); err != nil {
+		t.Fatal(err)
+	}
+
+	// And verify that this has re-set the fields.
+	tableDesc = sqlbase.GetTableDescriptor(kvDB, "t", "test")
+	found := false
+	for i := range tableDesc.Columns {
+		col := tableDesc.Columns[i]
+		if col.Name == "k" {
+			// TODO(knz): post-2.2, we're removing the visible types for
+			// integer types so the expectation will be just 0 here.
+			if col.Type.VisibleType != 0 && col.Type.VisibleType != sqlbase.ColumnType_BIGINT {
+				t.Errorf("unexpected visible type: got %s, expected NONE or BIGINT", col.Type.VisibleType.String())
+			}
+			if col.Type.Width != 64 {
+				t.Errorf("unexpected width: got %d, expected 64", col.Type.Width)
+			}
+			found = true
+			break
+		}
+	}
+	if !found {
+		t.Fatal("column disappeared")
+	}
+
+}

pkg/sql/distsqlplan/physical_plan.go

@@ -903,17 +903,22 @@ func MergeResultTypes(left, right []sqlbase.ColumnType) ([]sqlbase.ColumnType, e
 // type alias, which doesn't effect the merging of values. There is
 // also special handling for "naked" int types of no defined size.
 func equivalentTypes(c, other *sqlbase.ColumnType) bool {
-	// Convert a "naked" INT type to INT8. The 64-bit value is for
-	// historical compatibility before all INTs were assigned an
-	// explicit width at parse time.
+	// Convert pre-2.1 and pre-2.2 INTs to INT8.
 	lhs := *c
-	if lhs.SemanticType == sqlbase.ColumnType_INT && lhs.Width == 0 {
-		lhs.Width = 64
+	if lhs.SemanticType == sqlbase.ColumnType_INT {
+		// Pre-2.2 INT without size was assigned width 0.
+		// Pre-2.1 BIT was assigned arbitrary width, and is mapped to INT8 post-2.1. See #34161.
+		if lhs.Width != 64 && lhs.Width != 32 && lhs.Width != 16 {
+			lhs.Width = 64
+		}
 	}
 
 	rhs := *other
-	if rhs.SemanticType == sqlbase.ColumnType_INT && rhs.Width == 0 {
-		rhs.Width = 64
+	if rhs.SemanticType == sqlbase.ColumnType_INT {
+		// See above.
+		if rhs.Width != 64 && rhs.Width != 32 && rhs.Width != 16 {
+			rhs.Width = 64
+		}
 	}
 	rhs.VisibleType = c.VisibleType
 	return lhs.Equal(rhs)

pkg/sql/distsqlrun/materializer.go

@@ -155,7 +155,7 @@ func (m *materializer) Next() (sqlbase.EncDatumRow, *ProducerMetadata) {
 					m.row[outIdx].Datum = m.da.NewDInt(tree.DInt(col.Int16()[rowIdx]))
 				case 32:
 					m.row[outIdx].Datum = m.da.NewDInt(tree.DInt(col.Int32()[rowIdx]))
-				case 0, 64:
+				default:
 					m.row[outIdx].Datum = m.da.NewDInt(tree.DInt(col.Int64()[rowIdx]))
 				}
 			case sqlbase.ColumnType_FLOAT:

pkg/sql/logictest/testdata/logic_test/information_schema

@@ -907,9 +907,9 @@ FROM information_schema.columns
 WHERE table_schema = 'public' AND table_name = 'char_len'
 ----
 table_name  column_name  character_maximum_length  character_octet_length
-char_len    a            64                        NULL
-char_len    b            16                        NULL
-char_len    c            32                        NULL
+char_len    a            NULL                      NULL
+char_len    b            NULL                      NULL
+char_len    c            NULL                      NULL
 char_len    d            NULL                      NULL
 char_len    e            12                        48
 char_len    dc           1                         4

pkg/sql/sqlbase/column_type_properties.go

@@ -252,7 +252,13 @@ func (c *ColumnType) SQLString() string {
 		}
 		return typName
 	case ColumnType_INT:
-		if name, ok := coltypes.IntegerTypeNames[int(c.Width)]; ok {
+		// Pre-2.1 BIT was using column type INT with arbitrary width We
+		// map this to INT now. See #34161.
+		width := c.Width
+		if width != 0 && width != 64 && width != 32 && width != 16 {
+			width = 64
+		}
+		if name, ok := coltypes.IntegerTypeNames[int(width)]; ok {
 			return name
 		}
 	case ColumnType_STRING, ColumnType_COLLATEDSTRING:
@@ -383,7 +389,7 @@ func (c *ColumnType) InformationSchemaVisibleType() string {
 // expectations.
 func (c *ColumnType) MaxCharacterLength() (int32, bool) {
 	switch c.SemanticType {
-	case ColumnType_INT, ColumnType_STRING, ColumnType_COLLATEDSTRING, ColumnType_BIT:
+	case ColumnType_STRING, ColumnType_COLLATEDSTRING, ColumnType_BIT:
 		if c.Width > 0 {
 			return c.Width, true
 		}
@@ -422,7 +428,9 @@ func (c *ColumnType) NumericPrecision() (int32, bool) {
 	switch c.SemanticType {
 	case ColumnType_INT:
 		width := c.Width
-		if width == 0 {
+		// Pre-2.1 BIT was using column type INT with arbitrary
+		// widths. Clamp them to fixed/known widths. See #34161.
+		if width != 64 && width != 32 && width != 16 {
 			width = 64
 		}
 		return width, true