Merge cockroachdb#28945

28945: sql: fix the reporting of types in information_schema.columns r=knz a=knz

First commits from cockroachdb#28944 and priors.
Forked off cockroachdb#28690.
Fixes cockroachdb#27601.
Largely addresses the concerns that led to issue cockroachdb#26925.

Prior to this patch, CockroachDB incorrectly placed the "input syntax"
of each SQL type in the column `data_type` of
`information_schema.columns`.

The input syntax is the one reported in SHOW COLUMNS, SHOW CREATE
TABLE and other places, and is suitable to reproduce the exact type of
at able.

In contrast, `information_schema.columns.data_type` is constrained by
compatibility with third party tools and PostgreSQL clients.  It must
report the name of the type like PostgreSQL does, which in turn is
constrained by the SQL standard. A text column must be reported as
"text" not "string"; a decimal column as "numeric" not "decimal", a
float8 column as "double precision" not "float8", and so on.

By reporting the wrong string in that column CockroachDB is confusing
ORMs, which subsequently decide that the current on-disk type is not
the one expected by the app and then initiate a schema change (ALTER
COLUMN SET TYPE).

This patch corrects this incompatibility by introducing logic that
produces the proper information schema names for column types. This is
expected to reduce ORM complaints about insufficient support for ALTER
COLUMN SET TYPE (but will be tested/evaluated separately).

Release note (bug fix): CockroachDB now populates the `data_type`
column of `information_schema.columns` like PostgreSQL for
compatibility with 3rd party tools and ORMs.

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>

pkg/cli/dump.go

@@ -370,23 +370,43 @@ func extractArray(val interface{}) ([]string, error) {
 
 func getMetadataForTable(conn *sqlConn, md basicMetadata, ts string) (tableMetadata, error) {
 	// Fetch column types.
-	query := fmt.Sprintf(`
-		SELECT COLUMN_NAME, DATA_TYPE
+
+	makeQuery := func(colname string) string {
+		// This query is parameterized by the column name because of
+		// 2.0/2.1beta/2.1 trans-version compatibility requirements.  See
+		// below for details.
+		return fmt.Sprintf(`
+		SELECT COLUMN_NAME, %s
 		FROM %s.information_schema.columns
 		AS OF SYSTEM TIME %s
 		WHERE TABLE_CATALOG = $1
 			AND TABLE_SCHEMA = $2
 			AND TABLE_NAME = $3
 			AND GENERATION_EXPRESSION = ''
-		`, &md.name.CatalogName, lex.EscapeSQLString(ts))
-	rows, err := conn.Query(query+` AND IS_HIDDEN = 'NO'`,
+		`, colname, &md.name.CatalogName, lex.EscapeSQLString(ts))
+	}
+	rows, err := conn.Query(makeQuery("CRDB_SQL_TYPE")+` AND IS_HIDDEN = 'NO'`,
 		[]driver.Value{md.name.Catalog(), md.name.Schema(), md.name.Table()})
 	if err != nil {
+		// IS_HIDDEN was introduced in the first 2.1 beta. CRDB_SQL_TYPE
+		// some time after that.  To ensure `cockroach dump` works across
+		// versions we must try the previous forms if the first form
+		// fails.
+		//
+		// TODO(knz): Remove this fallback logic post-2.2.
+		if strings.Contains(err.Error(), "column \"crdb_sql_type\" does not exist") {
+			// Pre-2.1 CRDB_SQL_HIDDEN did not exist in
+			// information_schema.columns. When it does not exist,
+			// information_schema.columns.data_type contains a usable SQL
+			// type name instead. Use that.
+			rows, err = conn.Query(makeQuery("DATA_TYPE")+` AND IS_HIDDEN = 'NO'`,
+				[]driver.Value{md.name.Catalog(), md.name.Schema(), md.name.Table()})
+		}
 		if strings.Contains(err.Error(), "column \"is_hidden\" does not exist") {
 			// Pre-2.1 IS_HIDDEN did not exist in information_schema.columns.
 			// When it does not exist, information_schema.columns only returns
 			// non-hidden columns so we can still use that.
-			rows, err = conn.Query(query,
+			rows, err = conn.Query(makeQuery("DATA_TYPE"),
 				[]driver.Value{md.name.Catalog(), md.name.Schema(), md.name.Table()})
 		}
 		if err != nil {

pkg/sql/information_schema.go

@@ -254,8 +254,9 @@ CREATE TABLE information_schema.columns (
 	CHARACTER_SET_CATALOG    STRING,
 	CHARACTER_SET_SCHEMA     STRING,
 	CHARACTER_SET_NAME       STRING,
-	GENERATION_EXPRESSION    STRING,
-	IS_HIDDEN                STRING NOT NULL -- CockroachDB extension
+	GENERATION_EXPRESSION    STRING,          -- MySQL/CockroachDB extension.
+	IS_HIDDEN                STRING NOT NULL, -- CockroachDB extension for SHOW COLUMNS / dump.
+	CRDB_SQL_TYPE            STRING NOT NULL  -- CockroachDB extension for SHOW COLUMNS / dump.
 );
 `,
 	populate: func(ctx context.Context, p *planner, dbContext *DatabaseDescriptor, addRow func(...tree.Datum) error) error {
@@ -267,25 +268,26 @@ CREATE TABLE information_schema.columns (
 			return forEachColumnInTable(table, func(column *sqlbase.ColumnDescriptor) error {
 				visible++
 				return addRow(
-					dbNameStr,                                // table_catalog
-					scNameStr,                                // table_schema
-					tree.NewDString(table.Name),              // table_name
-					tree.NewDString(column.Name),             // column_name
-					tree.NewDInt(tree.DInt(visible)),         // ordinal_position, 1-indexed
-					dStringPtrOrNull(column.DefaultExpr),     // column_default
-					yesOrNoDatum(column.Nullable),            // is_nullable
-					tree.NewDString(column.Type.SQLString()), // data_type
-					characterMaximumLength(column.Type),      // character_maximum_length
-					characterOctetLength(column.Type),        // character_octet_length
-					numericPrecision(column.Type),            // numeric_precision
-					numericPrecisionRadix(column.Type),       // numeric_precision_radix
-					numericScale(column.Type),                // numeric_scale
-					datetimePrecision(column.Type),           // datetime_precision
-					tree.DNull,                               // character_set_catalog
-					tree.DNull,                               // character_set_schema
-					tree.DNull,                               // character_set_name
-					dStringPtrOrEmpty(column.ComputeExpr),    // generation_expression
-					yesOrNoDatum(column.Hidden),              // is_hidden
+					dbNameStr,                                                   // table_catalog
+					scNameStr,                                                   // table_schema
+					tree.NewDString(table.Name),                                 // table_name
+					tree.NewDString(column.Name),                                // column_name
+					tree.NewDInt(tree.DInt(visible)),                            // ordinal_position, 1-indexed
+					dStringPtrOrNull(column.DefaultExpr),                        // column_default
+					yesOrNoDatum(column.Nullable),                               // is_nullable
+					tree.NewDString(column.Type.InformationSchemaVisibleType()), // data_type
+					characterMaximumLength(column.Type),                         // character_maximum_length
+					characterOctetLength(column.Type),                           // character_octet_length
+					numericPrecision(column.Type),                               // numeric_precision
+					numericPrecisionRadix(column.Type),                          // numeric_precision_radix
+					numericScale(column.Type),                                   // numeric_scale
+					datetimePrecision(column.Type),                              // datetime_precision
+					tree.DNull,                                                  // character_set_catalog
+					tree.DNull,                                                  // character_set_schema
+					tree.DNull,                                                  // character_set_name
+					dStringPtrOrEmpty(column.ComputeExpr),                       // generation_expression
+					yesOrNoDatum(column.Hidden),                                 // is_hidden
+					tree.NewDString(column.Type.SQLString()),                    // crdb_sql_type
 				)
 			})
 		})
@@ -834,7 +836,7 @@ CREATE TABLE information_schema.sequences (
 					tree.NewDString(db.GetName()),    // catalog
 					tree.NewDString(scName),          // schema
 					tree.NewDString(table.GetName()), // name
-					tree.NewDString("INT"),           // type
+					tree.NewDString("integer"),       // type
 					tree.NewDInt(64),                 // numeric precision
 					tree.NewDInt(2),                  // numeric precision radix
 					tree.NewDInt(0),                  // numeric scale

pkg/sql/logictest/testdata/logic_test/information_schema

@@ -826,20 +826,20 @@ DROP TABLE nullability
 statement ok
 CREATE TABLE data_types (a INT, b FLOAT, c DECIMAL, d STRING, e BYTES, f TIMESTAMP, g TIMESTAMPTZ)
 
-query TTT colnames
-SELECT table_name, column_name, data_type
+query TTTT colnames
+SELECT table_name, column_name, data_type, crdb_sql_type
 FROM information_schema.columns
 WHERE table_schema = 'public' AND table_name = 'data_types'
 ----
-table_name  column_name  data_type
-data_types  a            INT
-data_types  b            FLOAT8
-data_types  c            DECIMAL
-data_types  d            STRING
-data_types  e            BYTES
-data_types  f            TIMESTAMP
-data_types  g            TIMESTAMP WITH TIME ZONE
-data_types  rowid        INT
+table_name  column_name  data_type                 crdb_sql_type
+data_types  a            integer                   INT
+data_types  b            double precision          FLOAT8
+data_types  c            numeric                   DECIMAL
+data_types  d            text                      STRING
+data_types  e            bytea                     BYTES
+data_types  f            timestamp                 TIMESTAMP
+data_types  g            timestamp with time zone  TIMESTAMP WITH TIME ZONE
+data_types  rowid        integer                   INT
 
 statement ok
 DROP TABLE data_types
@@ -1598,8 +1598,8 @@ query TTTTIIITTTTT colnames
 SELECT * FROM information_schema.sequences
 ----
 sequence_catalog  sequence_schema  sequence_name  data_type  numeric_precision  numeric_precision_radix  numeric_scale  start_value  minimum_value  maximum_value        increment  cycle_option
-test              public           test_seq       INT        64                 2                        0              1            1              9223372036854775807  1          NO
-test              public           test_seq_2     INT        64                 2                        0              15           5              1000                 -1         NO
+test              public           test_seq       integer    64                 2                        0              1            1              9223372036854775807  1          NO
+test              public           test_seq_2     integer    64                 2                        0              15           5              1000                 -1         NO
 
 statement ok
 CREATE DATABASE other_db

pkg/sql/logictest/testdata/planner_test/explain

@@ -209,7 +209,7 @@ sort                                       ·            ·
  └── render                                ·            ·
       └── group                            ·            ·
            │                               aggregate 0  column_name
-           │                               aggregate 1  data_type
+           │                               aggregate 1  crdb_sql_type
            │                               aggregate 2  is_nullable
            │                               aggregate 3  column_default
            │                               aggregate 4  generation_expression
@@ -224,7 +224,7 @@ sort                                       ·            ·
                      ├── render            ·            ·
                      │    └── filter       ·            ·
                      │         └── values  ·            ·
-                     │                     size         19 columns, 910 rows
+                     │                     size         20 columns, 911 rows
                      └── render            ·            ·
                           └── filter       ·            ·
                                └── values  ·            ·

pkg/sql/opt/exec/execbuilder/testdata/explain

@@ -202,7 +202,7 @@ sort                                       ·            ·
  └── render                                ·            ·
       └── group                            ·            ·
            │                               aggregate 0  column_name
-           │                               aggregate 1  data_type
+           │                               aggregate 1  crdb_sql_type
            │                               aggregate 2  is_nullable
            │                               aggregate 3  column_default
            │                               aggregate 4  generation_expression
@@ -217,7 +217,7 @@ sort                                       ·            ·
                      ├── render            ·            ·
                      │    └── filter       ·            ·
                      │         └── values  ·            ·
-                     │                     size         19 columns, 910 rows
+                     │                     size         20 columns, 911 rows
                      └── render            ·            ·
                           └── filter       ·            ·
                                └── values  ·            ·

pkg/sql/show_columns.go

@@ -28,25 +28,25 @@ func (p *planner) ShowColumns(ctx context.Context, n *tree.ShowColumns) (planNod
 	const getColumnsQuery = `
 SELECT
   column_name AS column_name,
-  data_type AS data_type,
+  crdb_sql_type AS data_type,
   is_nullable::BOOL,
   column_default,
   generation_expression,
   IF(inames[1] IS NULL, ARRAY[]:::STRING[], inames) AS indices,
   is_hidden::BOOL
 FROM
-  (SELECT column_name, data_type, is_nullable, column_default, generation_expression, ordinal_position, is_hidden,
+  (SELECT column_name, crdb_sql_type, is_nullable, column_default, generation_expression, ordinal_position, is_hidden,
           array_agg(index_name) AS inames
      FROM
-         (SELECT column_name, data_type, is_nullable, column_default, generation_expression, ordinal_position, is_hidden
+         (SELECT column_name, crdb_sql_type, is_nullable, column_default, generation_expression, ordinal_position, is_hidden
             FROM %[4]s.information_schema.columns
            WHERE (length(%[1]s)=0 OR table_catalog=%[1]s) AND table_schema=%[5]s AND table_name=%[2]s)
          LEFT OUTER JOIN
          (SELECT column_name, index_name
             FROM %[4]s.information_schema.statistics
            WHERE (length(%[1]s)=0 OR table_catalog=%[1]s) AND table_schema=%[5]s AND table_name=%[2]s)
          USING(column_name)
-    GROUP BY column_name, data_type, is_nullable, column_default, generation_expression, ordinal_position, is_hidden
+    GROUP BY column_name, crdb_sql_type, is_nullable, column_default, generation_expression, ordinal_position, is_hidden
    )
 ORDER BY ordinal_position`
 	return p.showTableDetails(ctx, "SHOW COLUMNS", n.Table, getColumnsQuery)

pkg/sql/sqlbase/column_type_properties.go

@@ -16,6 +16,7 @@ package sqlbase
 
 import (
 	"fmt"
+	"strings"
 	"unicode/utf8"
 
 	"github.com/pkg/errors"
@@ -196,12 +197,7 @@ var aliasToVisibleTypeMap = map[string]ColumnType_VisibleType{
 // Is is used in error messages and also to produce the output
 // of SHOW CREATE.
 //
-// TODO(knz): This is currently also used for
-// information_schema.columns and produces invalid/incompatible values
-// in that context. Either the standard names of the strings produced
-// by SQLString() here must become those expected by users of
-// information_schema.columns, or another function must be provided
-// for the information schema instead.
+// See also InformationSchemaVisibleType() below.
 func (c *ColumnType) SQLString() string {
 	switch c.SemanticType {
 	case ColumnType_STRING:
@@ -252,6 +248,63 @@ func (c *ColumnType) SQLString() string {
 	return c.SemanticType.String()
 }
 
+// InformationSchemaVisibleType returns the string suitable to
+// populate the data_type column of information_schema.columns.
+//
+// This is different from SQLString() in that it must report SQL
+// standard names that are compatible with PostgreSQL client
+// expectations.
+func (c *ColumnType) InformationSchemaVisibleType() string {
+	switch c.SemanticType {
+	case ColumnType_BOOL:
+		return "boolean"
+
+	case ColumnType_INT:
+		// TODO(knz): This is not exactly correct. See #28690 for a
+		// followup. This needs to use the type width instead.
+		if c.VisibleType == ColumnType_NONE {
+			return "integer"
+		}
+		return c.VisibleType.String()
+
+	case ColumnType_STRING, ColumnType_COLLATEDSTRING:
+		// TODO(knz): this misses the distinction between text, varchar,
+		// char and "char".
+		return "text"
+
+	case ColumnType_FLOAT:
+		width, _ := c.FloatProperties()
+
+		switch width {
+		case 64:
+			return "double precision"
+		case 32:
+			return "real"
+		default:
+			panic(fmt.Sprintf("programming error: unknown float width: %d", width))
+		}
+
+	case ColumnType_DECIMAL:
+		return "numeric"
+	case ColumnType_TIMESTAMPTZ:
+		return "timestamp with time zone"
+	case ColumnType_BYTES:
+		return "bytea"
+	case ColumnType_JSON:
+		return "jsonb"
+	case ColumnType_NULL:
+		return "unknown"
+	case ColumnType_TUPLE:
+		return "record"
+	case ColumnType_ARRAY:
+		return "ARRAY"
+	}
+
+	// The name of the remaining semantic type constants are suitable
+	// for the data_type column in information_schema.columns.
+	return strings.ToLower(c.SemanticType.String())
+}
+
 // MaxCharacterLength returns the declared maximum length of
 // characters if the ColumnType is a character or bit string data
 // type. Returns false if the data type is not a character or bit
@@ -303,20 +356,8 @@ func (c *ColumnType) NumericPrecision() (int32, bool) {
 	case ColumnType_INT:
 		return 64, true
 	case ColumnType_FLOAT:
-		switch c.VisibleType {
-		case ColumnType_REAL:
-			return 24, true
-		default:
-			// NONE now means double precision.
-			// Pre-2.1 there were 3 cases:
-			// - VisibleType = DOUBLE PRECISION, Width = 0 -> now clearly FLOAT8
-			// - VisibleType = NONE, Width = 0 -> now clearly FLOAT8
-			// - VisibleType = NONE, Width > 0 -> we need to derive the precision.
-			if c.Precision >= 1 && c.Precision <= 24 {
-				return 24, true
-			}
-			return 53, true
-		}
+		_, prec := c.FloatProperties()
+		return prec, true
 	case ColumnType_DECIMAL:
 		if c.Precision > 0 {
 			return c.Precision, true
@@ -364,6 +405,24 @@ func (c *ColumnType) NumericScale() (int32, bool) {
 	return 0, false
 }
 
+// FloatProperties returns the width and precision for a FLOAT column type.
+func (c *ColumnType) FloatProperties() (int32, int32) {
+	switch c.VisibleType {
+	case ColumnType_REAL:
+		return 32, 24
+	default:
+		// NONE now means double precision.
+		// Pre-2.1 there were 3 cases:
+		// - VisibleType = DOUBLE PRECISION, Width = 0 -> now clearly FLOAT8
+		// - VisibleType = NONE, Width = 0 -> now clearly FLOAT8
+		// - VisibleType = NONE, Width > 0 -> we need to derive the precision.
+		if c.Precision >= 1 && c.Precision <= 24 {
+			return 32, 24
+		}
+		return 64, 53
+	}
+}
+
 // datumTypeToColumnSemanticType converts a types.T to a SemanticType.
 //
 // This is mainly used by DatumTypeToColumnType() above; it is also

pkg/sql/sqlbase/structured.proto

@@ -100,8 +100,13 @@ import "gogoproto/gogo.proto";
 message ColumnType {
   option (gogoproto.equal) = true;
 
-  // These mirror the types supported by the sql/parser. See
-  // sql/parser/col_types.go.
+  // These mirror the types supported by sql/coltypes.
+  //
+  // Note: when adding constants to this list or renaming constants,
+  // verify with PostgreSQL what the type name should be in
+  // information_schema.columns.data_type, and modify
+  // (*ColumnType).InformationSchemaVisibleType() accordingly.
+  //
   enum SemanticType {
     BOOL = 0;
     INT = 1;        // INT(width)