Merge #31637 #31725 #31731

31637: pgwire: add telemetry for fetch limits r=knz a=knz

Requested by @awoods187 

The JDBC driver and perhaps others commonly try to use the "fetch
limit" parameter, which is yet unsupported in
CockroachDB (#4035). This patch adds telemetry to gauge demand.

Release note (sql change): attempts by client apps to use the
unsupported "fetch limit" parameter (e.g. via JDBC) will now be
captured in telemetry if statistics reporting is enabled, to gauge
support for this feature.

31725: sql/parser: re-allow FAMILY, MINVALUE, MAXVALUE, NOTHING and INDEX in table names r=knz a=knz

Fixes #31589.

CockroachDB introduced non-standard extensions to its SQL dialect very
early in its history, before concerns of compatibility with existing
PostgreSQL clients became a priority. When these features were added,
new keywords were liberally marked as "reserved", so as to "make the
grammar work", and without noticing / care for the fact that this
change would make existing valid SQL queries/clients encounter new
errors.

An example of this:

1. let's make "column families" a thing

2. the syntax `create table(..., constraint xxx family(a,b,c))` is not
   good enough (although this would not require reserved keywords), we
   really want also `create table (..., family (a,b,c))` to be
   possible.

3. oh, the grammar won't let us because "family" is a possible column
   name? No matter! let's mark "FAMILY" as a reserved name for
   column/function names.

   - No concern for the fact that "family" is a  perfectly valid
	 English name for things that people want to make an attribute of
	 in inventory / classification tables.

   - No concern for the fact that reserved column/function names are
	 also reserved for table names.

4. (much later) Clients complaining about the fact they can't call
   their columns or tables `family` without quoting.

Ditto "INDEX", "MINVALUE", "MAXVALUE", and perhaps others.

Moral of the story: DO NOT MAKE NEW RESERVED KEYWORDS UNLESS YOU'RE
VERY VERY VERY SURE THAT THERE IS NO LEGITIMATE USE FOR THEM IN CLIENT
APPS EVER.

(An example perhaps: the word "NOTHING" was also marked as reserved,
but it's much more unlikely this word will ever be used for something
useful.)

This patch restores the use of FAMILY, INDEX, NOTHING, MINVALUE and
MAXVALUE in table and function names, by introducing an awkward dance
in the grammar of keyword non-terminals and database object names.

They remain reserved as colum names because of the non-standard
CockroachDB extensions.

Release note (sql change): It is now again possible to use the
keywords FAMILY, MINVALUE, MAXVALUE, INDEX or NOTHING as table names,
for compatibility with PostgreSQL.

31731: sql/parser: unreserve INDEX and NOTHING from the RHS of SET statements r=knz a=knz

First commit from #31725.

The SET statement in the pg dialect is special because it
auto-converts identifiers on its RHS to symbolic values or strings. In
particular it is meant to support a diversity of special keywords as
pseudo-values.

This patch ensures that INDEX and NOTHING are accepted on the RHS.

Release note (sql change): the names "index" and "nothing" are again
accepted in the right-hand-side of the assignment in SET statements,
for compatibility with PostgreSQL.


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

docs/generated/sql/bnf/stmt_block.bnf

@@ -808,8 +808,8 @@ kv_option_list ::=
 
 complex_table_pattern ::=
 	complex_db_object_name
-	| name '.' unrestricted_name '.' '*'
-	| name '.' '*'
+	| db_object_name_component '.' unrestricted_name '.' '*'
+	| db_object_name_component '.' '*'
 	| '*'
 
 table_pattern ::=
@@ -1050,7 +1050,7 @@ to_or_eq ::=
 
 var_value ::=
 	a_expr
-	| 'ON'
+	| extra_var_value
 
 opt_on_targets_roles ::=
 	'ON' targets_roles
@@ -1093,11 +1093,11 @@ set_clause ::=
 	| multiple_set_clause
 
 simple_db_object_name ::=
-	name
+	db_object_name_component
 
 complex_db_object_name ::=
-	name '.' unrestricted_name
-	| name '.' unrestricted_name '.' unrestricted_name
+	db_object_name_component '.' unrestricted_name
+	| db_object_name_component '.' unrestricted_name '.' unrestricted_name
 
 non_reserved_word ::=
 	'identifier'
@@ -1111,6 +1111,11 @@ kv_option ::=
 	| 'SCONST' '=' string_or_placeholder
 	| 'SCONST'
 
+db_object_name_component ::=
+	name
+	| cockroachdb_extra_type_func_name_keyword
+	| cockroachdb_extra_reserved_keyword
+
 unrestricted_name ::=
 	'identifier'
 	| unreserved_keyword
@@ -1414,6 +1419,10 @@ offset_clause ::=
 generic_set ::=
 	var_name to_or_eq var_list
 
+extra_var_value ::=
+	'ON'
+	| cockroachdb_extra_reserved_keyword
+
 targets_roles ::=
 	'ROLE' name_list
 	| targets
@@ -1430,7 +1439,6 @@ multiple_set_clause ::=
 type_func_name_keyword ::=
 	'COLLATION'
 	| 'CROSS'
-	| 'FAMILY'
 	| 'FULL'
 	| 'INNER'
 	| 'ILIKE'
@@ -1439,14 +1447,22 @@ type_func_name_keyword ::=
 	| 'JOIN'
 	| 'LEFT'
 	| 'LIKE'
-	| 'MAXVALUE'
-	| 'MINVALUE'
 	| 'NATURAL'
 	| 'NOTNULL'
 	| 'OUTER'
 	| 'OVERLAPS'
 	| 'RIGHT'
 	| 'SIMILAR'
+	| cockroachdb_extra_type_func_name_keyword
+
+cockroachdb_extra_type_func_name_keyword ::=
+	'FAMILY'
+	| 'MAXVALUE'
+	| 'MINVALUE'
+
+cockroachdb_extra_reserved_keyword ::=
+	'INDEX'
+	| 'NOTHING'
 
 reserved_keyword ::=
 	'ALL'
@@ -1490,7 +1506,6 @@ reserved_keyword ::=
 	| 'GROUP'
 	| 'HAVING'
 	| 'IN'
-	| 'INDEX'
 	| 'INITIALLY'
 	| 'INTERSECT'
 	| 'INTO'
@@ -1500,7 +1515,6 @@ reserved_keyword ::=
 	| 'LOCALTIME'
 	| 'LOCALTIMESTAMP'
 	| 'NOT'
-	| 'NOTHING'
 	| 'NULL'
 	| 'OFFSET'
 	| 'ON'
@@ -1529,6 +1543,7 @@ reserved_keyword ::=
 	| 'WHERE'
 	| 'WINDOW'
 	| 'WITH'
+	| cockroachdb_extra_reserved_keyword
 
 transaction_iso_level ::=
 	'ISOLATION' 'LEVEL' iso_level
@@ -1649,9 +1664,9 @@ interval ::=
 
 column_path_with_star ::=
 	column_path
-	| name '.' unrestricted_name '.' unrestricted_name '.' '*'
-	| name '.' unrestricted_name '.' '*'
-	| name '.' '*'
+	| db_object_name_component '.' unrestricted_name '.' unrestricted_name '.' '*'
+	| db_object_name_component '.' unrestricted_name '.' '*'
+	| db_object_name_component '.' '*'
 
 func_expr ::=
 	func_application filter_clause over_clause
@@ -2048,9 +2063,9 @@ interval_qualifier ::=
 	| 'MINUTE' 'TO' interval_second
 
 prefixed_column_path ::=
-	name '.' unrestricted_name
-	| name '.' unrestricted_name '.' unrestricted_name
-	| name '.' unrestricted_name '.' unrestricted_name '.' unrestricted_name
+	db_object_name_component '.' unrestricted_name
+	| db_object_name_component '.' unrestricted_name '.' unrestricted_name
+	| db_object_name_component '.' unrestricted_name '.' unrestricted_name '.' unrestricted_name
 
 func_name ::=
 	type_function_name

pkg/server/telemetry/features.go

@@ -18,6 +18,8 @@ import (
 	"fmt"
 	"sync/atomic"
 
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 )
 
@@ -123,3 +125,36 @@ func GetAndResetFeatureCounts(quantize bool) map[string]int32 {
 	}
 	return m
 }
+
+// RecordError takes an error and increments the corresponding count
+// for its error code, and, if it is an unimplemented or internal
+// error, the count for that feature or the internal error's shortened
+// stack trace.
+func RecordError(err error) {
+	if err == nil {
+		return
+	}
+
+	if pgErr, ok := pgerror.GetPGCause(err); ok {
+		Count("errorcodes." + pgErr.Code)
+
+		if details := pgErr.InternalCommand; details != "" {
+			var prefix string
+			switch pgErr.Code {
+			case pgerror.CodeFeatureNotSupportedError:
+				prefix = "unimplemented."
+			case pgerror.CodeInternalError:
+				prefix = "internalerror."
+			default:
+				prefix = "othererror." + pgErr.Code + "."
+			}
+			Count(prefix + details)
+		}
+	} else {
+		typ := log.ErrorSource(err)
+		if typ == "" {
+			typ = "unknown"
+		}
+		Count("othererror." + typ)
+	}
+}

pkg/sql/conn_executor.go

@@ -303,39 +303,6 @@ func (s *Server) Start(ctx context.Context, stopper *stop.Stopper) {
 	s.PeriodicallyClearStmtStats(ctx, stopper)
 }
 
-// recordError takes an error and increments the corresponding count
-// for its error code, and, if it is an unimplemented or internal
-// error, the count for that feature or the internal error's shortened
-// stack trace.
-func (s *Server) recordError(err error) {
-	if err == nil {
-		return
-	}
-
-	if pgErr, ok := pgerror.GetPGCause(err); ok {
-		telemetry.Count("errorcodes." + pgErr.Code)
-
-		if details := pgErr.InternalCommand; details != "" {
-			var prefix string
-			switch pgErr.Code {
-			case pgerror.CodeFeatureNotSupportedError:
-				prefix = "unimplemented."
-			case pgerror.CodeInternalError:
-				prefix = "internalerror."
-			default:
-				prefix = "othererror." + pgErr.Code + "."
-			}
-			telemetry.Count(prefix + details)
-		}
-	} else {
-		typ := log.ErrorSource(err)
-		if typ == "" {
-			typ = "unknown"
-		}
-		telemetry.Count("othererror." + typ)
-	}
-}
-
 // ResetStatementStats resets the executor's collected statement statistics.
 func (s *Server) ResetStatementStats(ctx context.Context) {
 	s.sqlStats.resetStats(ctx)
@@ -1293,12 +1260,12 @@ func (ex *connExecutor) run(
 				ex.sessionEventf(ex.Ctx(), "execution error: %s", pe.errorCause())
 			}
 			if resErr == nil && ok {
-				ex.server.recordError(pe.errorCause())
+				telemetry.RecordError(pe.errorCause())
 				// Depending on whether the result has the error already or not, we have
 				// to call either Close or CloseWithErr.
 				res.CloseWithErr(pe.errorCause())
 			} else {
-				ex.server.recordError(resErr)
+				telemetry.RecordError(resErr)
 				res.Close(stateToTxnStatusIndicator(ex.machine.CurState()))
 			}
 		} else {

pkg/sql/conn_executor_exec.go

@@ -26,6 +26,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/internal/client"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/coltypes"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -1207,7 +1208,7 @@ func (ex *connExecutor) runShowSyntax(
 			commErr = res.AddRow(ctx, tree.Datums{tree.NewDString(field), tree.NewDString(msg)})
 			return nil
 		},
-		ex.server.recordError, /* reportErr */
+		telemetry.RecordError, /* reportErr */
 	); err != nil {
 		res.SetError(err)
 	}

pkg/sql/parser/all_keywords.awk

@@ -21,9 +21,9 @@ BEGIN {
       category = "C"
   } else if ($1 == "unreserved_keyword:") {
       category = "U"
-  } else if ($1 == "type_func_name_keyword:") {
+  } else if ($1 == "type_func_name_keyword:" || $1 == "cockroachdb_extra_type_func_name_keyword:") {
       category = "T"
-  } else if ($1 == "reserved_keyword:") {
+  } else if ($1 == "reserved_keyword:" || $1 == "cockroachdb_extra_reserved_keyword:") {
       category ="R"
   } else {
       print "unknown keyword type:", $1 >>"/dev/stderr"
@@ -36,7 +36,7 @@ BEGIN {
   keyword = 0
 }
 
-{
+/^ *\|? *[A-Z]/ {
   if (keyword && $NF != "") {
       printf("\"%s\": {%s, \"%s\"},\n", tolower($NF), $NF, category) | sort
   }

pkg/sql/parser/parse_test.go

@@ -1839,6 +1839,21 @@ func TestParse2(t *testing.T) {
 		{`ALTER TABLE a ALTER b DROP NOT NULL`, `ALTER TABLE a ALTER COLUMN b DROP NOT NULL`},
 		{`ALTER TABLE a ALTER b TYPE INT`, `ALTER TABLE a ALTER COLUMN b SET DATA TYPE INT`},
 		{`EXPLAIN ANALYZE SELECT 1`, `EXPLAIN ANALYZE (DISTSQL) SELECT 1`},
+
+		{`SET a = INDEX`, `SET a = "index"`},
+		{`SET a = NOTHING`, `SET a = "nothing"`},
+
+		// Regression for #31589
+		{`CREATE TABLE FAMILY (x INT)`,
+			`CREATE TABLE "family" (x INT)`},
+		{`CREATE TABLE INDEX (x INT)`,
+			`CREATE TABLE "index" (x INT)`},
+		{`CREATE TABLE NOTHING (x INT)`,
+			`CREATE TABLE "nothing" (x INT)`},
+		{`CREATE TABLE MINVALUE (x INT)`,
+			`CREATE TABLE "minvalue" (x INT)`},
+		{`CREATE TABLE MAXVALUE (x INT)`,
+			`CREATE TABLE "maxvalue" (x INT)`},
 	}
 	for _, d := range testData {
 		stmts, err := parser.Parse(d.sql)

pkg/sql/parser/reserved_keywords.awk

@@ -3,12 +3,12 @@
   next
 }
 
-/^type_func_name_keyword:/ {
+/^(cockroachdb_extra_)?type_func_name_keyword:/ {
   reserved_keyword = 1
   next
 }
 
-/^reserved_keyword:/ {
+/^(cockroachdb_extra_)?reserved_keyword:/ {
   reserved_keyword = 1
   next
 }