Merge #65499 #73627 #73635 #73648 #73650

65499: kvserver: improve documentation for non-ready nodes r=aayushshah15 a=aayushshah15

This commit improves the comment over the `StorePool`'s
`isNodeReadyForRoutineReplicaTransfer` method by clarifying the distinction
between such non-ready nodes and "dead" nodes.

Release note: None

73627: util: fastintset fixes and improvements r=RaduBerinde a=RaduBerinde

#### util: fix FastIntSet.Next and ForEach with negative numbers

These methods assume that we only insert non-negative elements
(whereas the rest of the API allows that case). This commit fixes this
and improves the test to include negative elements.

Release note: None

#### util: remove FastIntSet.Shift

This method is not used anymore.

Release note: None

#### util: fix FastIntSet.Encode edge case

Encode has an edge case where we incorrectly encode a set that has the
large field set but which is empty. This commit fixes this and adds
tests for Encode/Decode.

Also, before this change Encode could choose either encoding for a
"small" set depending whether `large` was allocated. Although this
doesn't break the documented contract, it is not ideal to leak this
internal detail.

This change also cleans up the code a bit (the semantics of
`largeToSmall` are left over from an earlier implementation).

Release note: None

#### fastintset: reduce allocations from FastIntSet.Encode

The current implementation of Encode causes allocations: the temporary
array escapes to the heap because it is passed through an opaque
interface call.

This change fixes this by passing a `bytes.Buffer` specifically.

`Prepared/ExecBuild` benchmark results:
```
name                                                   old time/op    new time/op    delta
Phases/kv-read/Prepared/ExecBuild                        1.46µs ± 2%    1.40µs ± 2%   -4.04%  (p=0.000 n=10+10)
Phases/kv-read-const/Prepared/ExecBuild                  1.24µs ± 2%    1.14µs ± 2%   -8.03%  (p=0.000 n=10+10)
Phases/tpcc-new-order/Prepared/ExecBuild                 1.79µs ± 2%    1.69µs ± 2%   -5.67%  (p=0.000 n=10+9)
Phases/tpcc-delivery/Prepared/ExecBuild                  33.0µs ± 1%    33.7µs ± 5%   +2.19%  (p=0.001 n=10+10)
Phases/tpcc-stock-level/Prepared/ExecBuild                143µs ± 2%     141µs ± 2%     ~     (p=0.280 n=10+10)
Phases/many-columns-and-indexes-a/Prepared/ExecBuild      511µs ± 1%     515µs ± 2%     ~     (p=0.218 n=10+10)
Phases/many-columns-and-indexes-b/Prepared/ExecBuild      529µs ± 1%     543µs ± 4%   +2.77%  (p=0.004 n=10+10)
Phases/ored-preds-100/Prepared/ExecBuild                  116µs ± 1%     118µs ± 4%   +1.35%  (p=0.029 n=10+10)
Phases/ored-preds-using-params-100/Prepared/ExecBuild    1.41ms ± 2%    1.41ms ± 3%     ~     (p=0.971 n=10+10)
name                                                   old alloc/op   new alloc/op   delta
Phases/kv-read/Prepared/ExecBuild                          704B ± 0%      656B ± 0%   -6.82%  (p=0.000 n=10+10)
Phases/kv-read-const/Prepared/ExecBuild                    520B ± 0%      472B ± 0%   -9.23%  (p=0.000 n=10+10)
Phases/tpcc-new-order/Prepared/ExecBuild                   768B ± 0%      720B ± 0%   -6.25%  (p=0.000 n=10+10)
Phases/tpcc-delivery/Prepared/ExecBuild                  11.1kB ± 0%    11.1kB ± 0%   -0.39%  (p=0.000 n=10+6)
Phases/tpcc-stock-level/Prepared/ExecBuild               39.5kB ± 0%    39.5kB ± 0%   -0.12%  (p=0.000 n=10+10)
Phases/many-columns-and-indexes-a/Prepared/ExecBuild     5.02kB ± 0%    4.98kB ± 0%   -0.80%  (p=0.000 n=10+10)
Phases/many-columns-and-indexes-b/Prepared/ExecBuild     7.46kB ± 0%    7.42kB ± 0%   -0.53%  (p=0.000 n=10+10)
Phases/ored-preds-100/Prepared/ExecBuild                 29.6kB ± 0%    29.6kB ± 0%   -0.16%  (p=0.000 n=10+10)
Phases/ored-preds-using-params-100/Prepared/ExecBuild     765kB ± 0%     765kB ± 0%   -0.01%  (p=0.005 n=10+10)
name                                                   old allocs/op  new allocs/op  delta
Phases/kv-read/Prepared/ExecBuild                          11.0 ± 0%       9.0 ± 0%  -18.18%  (p=0.000 n=10+10)
Phases/kv-read-const/Prepared/ExecBuild                    8.00 ± 0%      6.00 ± 0%  -25.00%  (p=0.000 n=10+10)
Phases/tpcc-new-order/Prepared/ExecBuild                   11.0 ± 0%       9.0 ± 0%  -18.18%  (p=0.000 n=10+10)
Phases/tpcc-delivery/Prepared/ExecBuild                    75.0 ± 0%      73.0 ± 0%   -2.67%  (p=0.000 n=10+10)
Phases/tpcc-stock-level/Prepared/ExecBuild                  249 ± 0%       247 ± 0%   -0.80%  (p=0.000 n=10+10)
Phases/many-columns-and-indexes-a/Prepared/ExecBuild       34.0 ± 0%      32.0 ± 0%   -5.88%  (p=0.000 n=10+10)
Phases/many-columns-and-indexes-b/Prepared/ExecBuild       53.0 ± 0%      51.0 ± 0%   -3.77%  (p=0.000 n=10+10)
Phases/ored-preds-100/Prepared/ExecBuild                    415 ± 0%       413 ± 0%   -0.48%  (p=0.000 n=10+10)
Phases/ored-preds-using-params-100/Prepared/ExecBuild     3.29k ± 0%     3.29k ± 0%   -0.06%  (p=0.000 n=10+10)
```

Release note: None

#### fastintset: fix fast_int_set_small/fast_int_set_large test builds

This commit adds Encode/Decode to the test-only implementation which
unbreaks the test builds with `fast_int_set_small` or
`fast_int_set_large` tags.

Release note: None


73635: sql: properly implement create_type_statements index r=rafiss a=otan

See individual commits for details!

Resolves #71414

73648: ui: update cluster-ui version to v22.1.0-prerelease-3 r=maryliag a=maryliag

Update cluster-ui package version

Release note: None

73650: opt: minor optbuilder improvements related to assignment casts r=mgartner a=mgartner

#### opt: do not rely on column names for GENERATED ALWAYS AS IDENTITY checks

Previously, optbuilder relied on matching column names to validate that
mutations do not explicitly write to `GENERATED ALWAYS AS IDENTITY`
columns. There are no known bugs caused by this, but relying on column
names can be brittle. This commit updates the logic to use ordinals of
columns within their table instead.

Release note: None

#### opt: simplify optbuilder update logic

Release note: None

#### opt: simplify assignment cast logic for inserts

Creation of invalid assignment cast errors has been abstracted to a
helper function for reuse for other mutations in a future commit.

Release note: None


Co-authored-by: Aayush Shah <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Marylia Gutierrez <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>

pkg/kv/kvserver/store_pool.go

@@ -350,10 +350,19 @@ type StorePool struct {
 		nodeLocalities map[roachpb.NodeID]localityWithString
 	}
 
-	// isStoreReadyForRoutineReplicaTransfer returns true if the
-	// store is live and thus a good candidate to receive a replica.
-	// This is defined as a closure reference here instead
+	// isStoreReadyForRoutineReplicaTransfer returns true iff the store's node is
+	// live (as indicated by its `NodeLivenessStatus`) and thus a legal candidate
+	// to receive a replica. This is defined as a closure reference here instead
 	// of a regular method so it can be overridden in tests.
+	//
+	// NB: What this method aims to capture is distinct from "dead" nodes. Nodes
+	// are classified as "dead" if they haven't successfully heartbeat their
+	// liveness record in the last `server.time_until_store_dead` seconds.
+	//
+	// Functionally, the distinction is that we simply avoid transferring replicas
+	// to "non-ready" nodes (i.e. nodes that _currently_ have a non-live
+	// `NodeLivenessStatus`), whereas we _actively move replicas off of "dead"
+	// nodes_.
 	isStoreReadyForRoutineReplicaTransfer func(context.Context, roachpb.StoreID) bool
 }
 

pkg/sql/BUILD.bazel

@@ -584,6 +584,7 @@ go_test(
         "//pkg/sql/catalog/descs",
         "//pkg/sql/catalog/lease",
         "//pkg/sql/catalog/multiregion",
+        "//pkg/sql/catalog/schemadesc",
         "//pkg/sql/catalog/systemschema",
         "//pkg/sql/catalog/tabledesc",
         "//pkg/sql/catalog/typedesc",

pkg/sql/crdb_internal.go

@@ -2233,6 +2233,54 @@ CREATE TABLE crdb_internal.builtin_functions (
 	},
 }
 
+func writeCreateTypeDescRow(
+	db catalog.DatabaseDescriptor,
+	sc string,
+	typeDesc catalog.TypeDescriptor,
+	addRow func(...tree.Datum) error,
+) (written bool, err error) {
+	switch typeDesc.GetKind() {
+	case descpb.TypeDescriptor_ENUM:
+		var enumLabels tree.EnumValueList
+		enumLabelsDatum := tree.NewDArray(types.String)
+		for i := 0; i < typeDesc.NumEnumMembers(); i++ {
+			rep := typeDesc.GetMemberLogicalRepresentation(i)
+			enumLabels = append(enumLabels, tree.EnumValue(rep))
+			if err := enumLabelsDatum.Append(tree.NewDString(rep)); err != nil {
+				return false, err
+			}
+		}
+		name, err := tree.NewUnresolvedObjectName(2, [3]string{typeDesc.GetName(), sc}, 0)
+		if err != nil {
+			return false, err
+		}
+		node := &tree.CreateType{
+			Variety:    tree.Enum,
+			TypeName:   name,
+			EnumLabels: enumLabels,
+		}
+		return true, addRow(
+			tree.NewDInt(tree.DInt(db.GetID())),       // database_id
+			tree.NewDString(db.GetName()),             // database_name
+			tree.NewDString(sc),                       // schema_name
+			tree.NewDInt(tree.DInt(typeDesc.GetID())), // descriptor_id
+			tree.NewDString(typeDesc.GetName()),       // descriptor_name
+			tree.NewDString(tree.AsString(node)),      // create_statement
+			enumLabelsDatum,
+		)
+	case descpb.TypeDescriptor_MULTIREGION_ENUM:
+		// Multi-region enums are created implicitly, so we don't have create
+		// statements for them.
+		return false, nil
+	case descpb.TypeDescriptor_ALIAS:
+		// Alias types are created implicitly, so we don't have create
+		// statements for them.
+		return false, nil
+	default:
+		return false, errors.AssertionFailedf("unknown type descriptor kind %s", typeDesc.GetKind().String())
+	}
+}
+
 var crdbInternalCreateTypeStmtsTable = virtualSchemaTable{
 	comment: "CREATE statements for all user defined types accessible by the current user in current database (KV scan)",
 	schema: `
@@ -2243,53 +2291,37 @@ CREATE TABLE crdb_internal.create_type_statements (
   descriptor_id      INT,
   descriptor_name    STRING,
   create_statement   STRING,
-  enum_members       STRING[] -- populated only for ENUM types
+  enum_members       STRING[], -- populated only for ENUM types
+  INDEX (descriptor_id)
 )
 `,
 	populate: func(ctx context.Context, p *planner, db catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
 		return forEachTypeDesc(ctx, p, db, func(db catalog.DatabaseDescriptor, sc string, typeDesc catalog.TypeDescriptor) error {
-			switch typeDesc.GetKind() {
-			case descpb.TypeDescriptor_ENUM:
-				var enumLabels tree.EnumValueList
-				enumLabelsDatum := tree.NewDArray(types.String)
-				for i := 0; i < typeDesc.NumEnumMembers(); i++ {
-					rep := typeDesc.GetMemberLogicalRepresentation(i)
-					enumLabels = append(enumLabels, tree.EnumValue(rep))
-					if err := enumLabelsDatum.Append(tree.NewDString(rep)); err != nil {
-						return err
-					}
-				}
-				name, err := tree.NewUnresolvedObjectName(2, [3]string{typeDesc.GetName(), sc}, 0)
-				if err != nil {
-					return err
-				}
-				node := &tree.CreateType{
-					Variety:    tree.Enum,
-					TypeName:   name,
-					EnumLabels: enumLabels,
+			_, err := writeCreateTypeDescRow(db, sc, typeDesc, addRow)
+			return err
+		})
+	},
+	indexes: []virtualIndex{
+		{
+			populate: func(
+				ctx context.Context,
+				constraint tree.Datum,
+				p *planner,
+				db catalog.DatabaseDescriptor,
+				addRow func(...tree.Datum) error,
+			) (matched bool, err error) {
+				d := tree.UnwrapDatum(p.EvalContext(), constraint)
+				if d == tree.DNull {
+					return false, nil
 				}
-				if err := addRow(
-					tree.NewDInt(tree.DInt(db.GetID())),       // database_id
-					tree.NewDString(db.GetName()),             // database_name
-					tree.NewDString(sc),                       // schema_name
-					tree.NewDInt(tree.DInt(typeDesc.GetID())), // descriptor_id
-					tree.NewDString(typeDesc.GetName()),       // descriptor_name
-					tree.NewDString(tree.AsString(node)),      // create_statement
-					enumLabelsDatum,
-				); err != nil {
-					return err
+				id := descpb.ID(tree.MustBeDInt(d))
+				scName, typDesc, err := getSchemaAndTypeByTypeID(ctx, p, id)
+				if err != nil || typDesc == nil {
+					return false, err
 				}
-			case descpb.TypeDescriptor_MULTIREGION_ENUM:
-				// Multi-region enums are created implicitly, so we don't have create
-				// statements for them.
-			case descpb.TypeDescriptor_ALIAS:
-			// Alias types are created implicitly, so we don't have create
-			// statements for them.
-			default:
-				return errors.AssertionFailedf("unknown type descriptor kind %s", typeDesc.GetKind().String())
-			}
-			return nil
-		})
+				return writeCreateTypeDescRow(db, scName, typDesc, addRow)
+			},
+		},
 	},
 }
 

pkg/sql/logictest/testdata/logic_test/crdb_internal

@@ -762,6 +762,15 @@ SELECT * FROM crdb_internal.create_type_statements
 
 # Test the virtual index as well.
 
+query ITTITTT
+SELECT * FROM crdb_internal.create_type_statements WHERE descriptor_id = (('enum1'::regtype::oid::int) - 100000)::oid
+----
+54  test  public  66  enum1  CREATE TYPE public.enum1 AS ENUM ('hello', 'hi')  {hello,hi}
+
+query ITTITTT
+SELECT * FROM crdb_internal.create_type_statements WHERE descriptor_id = 'foo'::regclass::oid
+----
+
 statement ok
 SET application_name = "test_txn_statistics"
 

pkg/sql/logictest/testdata/logic_test/create_statements

@@ -289,15 +289,17 @@ CREATE TABLE crdb_internal.create_type_statements (
    descriptor_id INT8 NULL,
    descriptor_name STRING NULL,
    create_statement STRING NULL,
-   enum_members STRING[] NULL
+   enum_members STRING[] NULL,
+   INDEX create_type_statements_descriptor_id_idx (descriptor_id ASC) STORING (database_id, database_name, schema_name, descriptor_name, create_statement, enum_members)
 )  CREATE TABLE crdb_internal.create_type_statements (
    database_id INT8 NULL,
    database_name STRING NULL,
    schema_name STRING NULL,
    descriptor_id INT8 NULL,
    descriptor_name STRING NULL,
    create_statement STRING NULL,
-   enum_members STRING[] NULL
+   enum_members STRING[] NULL,
+   INDEX create_type_statements_descriptor_id_idx (descriptor_id ASC) STORING (database_id, database_name, schema_name, descriptor_name, create_statement, enum_members)
 )  {}  {}
 CREATE TABLE crdb_internal.cross_db_references (
    object_database STRING NOT NULL,

pkg/sql/opt/exec/explain/plan_gist_factory.go

@@ -15,7 +15,6 @@ import (
 	"context"
 	"encoding/base64"
 	"encoding/binary"
-	"io"
 
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
@@ -39,11 +38,14 @@ func init() {
 	}
 }
 
-// version tracks major changes to how we encode plans or to the operator set.
-// It isn't necessary to increment it when adding a single operator but if we
-// remove an operator or change the operator set or decide to use a more
+// gistVersion tracks major changes to how we encode plans or to the operator
+// set. It isn't necessary to increment it when adding a single operator but if
+// we remove an operator or change the operator set or decide to use a more
 // efficient encoding version should be incremented.
-var version = 1
+//
+// Version history:
+//   1. Initial version.
+var gistVersion = 1
 
 // PlanGist is a compact representation of a logical plan meant to be used as
 // a key and log for different plans used to implement a particular query. A
@@ -78,18 +80,14 @@ func (fp PlanGist) Hash() uint64 {
 // PlanGistFactory is an exec.Factory that produces a gist by eaves
 // dropping on the exec builder phase of compilation.
 type PlanGistFactory struct {
-	// PlanGistFactory implements a writer interface in order to write to the
-	// gist buffer and maintain a hash. io.MultiWriter could work but this is more
-	// efficient.
-	io.Writer
-
 	wrappedFactory exec.Factory
-	// buffer is used for reading and writing (i.e. decoding and encoding) but on
-	// on the write path it is used via the writer field which is a multi-writer
-	// writing to the buffer and to the hash. The exception is when we're dealing
-	// with ids where we will write the id to the buffer and the "string" to the
-	// hash.  This allows the hash to be id agnostic (ie hash's will be stable
-	// across plans from different databases with different DDL history).
+
+	// buffer is used for reading and writing (i.e. decoding and encoding).
+	// Data that is written to the buffer is also added to the hash. The
+	// exception is when we're dealing with ids where we will write the id to the
+	// buffer and the "string" to the hash. This allows the hash to be id agnostic
+	// (ie hash's will be stable across plans from different databases with
+	// different DDL history).
 	buffer bytes.Buffer
 	hash   util.FNV64
 
@@ -98,15 +96,16 @@ type PlanGistFactory struct {
 }
 
 var _ exec.Factory = &PlanGistFactory{}
-var _ io.Writer = &PlanGistFactory{}
 
-func (f *PlanGistFactory) Write(data []byte) (int, error) {
-	i, err := f.buffer.Write(data)
-	f.writeHash(data)
-	return i, err
+// writeAndHash writes an arbitrary slice of bytes to the buffer and hashes each
+// byte.
+func (f *PlanGistFactory) writeAndHash(data []byte) int {
+	i, _ := f.buffer.Write(data)
+	f.updateHash(data)
+	return i
 }
 
-func (f *PlanGistFactory) writeHash(data []byte) {
+func (f *PlanGistFactory) updateHash(data []byte) {
 	for _, b := range data {
 		f.hash.Add(uint64(b))
 	}
@@ -117,7 +116,7 @@ func NewPlanGistFactory(wrappedFactory exec.Factory) *PlanGistFactory {
 	f := new(PlanGistFactory)
 	f.wrappedFactory = wrappedFactory
 	f.hash.Init()
-	f.encodeInt(version)
+	f.encodeInt(gistVersion)
 	return f
 }
 
@@ -186,8 +185,8 @@ func DecodePlanGistToPlan(s string, cat cat.Catalog) (plan *Plan, retErr error)
 	}()
 
 	ver := f.decodeInt()
-	if ver != version {
-		return nil, errors.Errorf("unsupported old plan gist version %d", ver)
+	if ver != gistVersion {
+		return nil, errors.Errorf("unsupported gist version %d (expected %d)", ver, gistVersion)
 	}
 
 	for {
@@ -242,10 +241,7 @@ func (f *PlanGistFactory) encodeOperator(op execOperator) {
 func (f *PlanGistFactory) encodeInt(i int) {
 	var buf [binary.MaxVarintLen64]byte
 	n := binary.PutVarint(buf[:], int64(i))
-	_, err := f.Write(buf[:n])
-	if err != nil {
-		panic(err)
-	}
+	f.writeAndHash(buf[:n])
 }
 
 func (f *PlanGistFactory) decodeInt() int {
@@ -266,7 +262,7 @@ func (f *PlanGistFactory) encodeDataSource(id cat.StableID, name tree.Name) {
 	if err != nil {
 		panic(err)
 	}
-	f.writeHash([]byte(string(name)))
+	f.updateHash([]byte(string(name)))
 }
 
 func (f *PlanGistFactory) encodeID(id cat.StableID) {
@@ -332,10 +328,8 @@ func (f *PlanGistFactory) decodeResultColumns() colinfo.ResultColumns {
 }
 
 func (f *PlanGistFactory) encodeByte(b byte) {
-	_, err := f.Write([]byte{b})
-	if err != nil {
-		panic(err)
-	}
+	f.buffer.WriteByte(b)
+	f.hash.Add(uint64(b))
 }
 
 func (f *PlanGistFactory) decodeByte() byte {
@@ -364,6 +358,14 @@ func (f *PlanGistFactory) decodeBool() bool {
 	return val != 0
 }
 
+func (f *PlanGistFactory) encodeFastIntSet(s util.FastIntSet) {
+	lenBefore := f.buffer.Len()
+	if err := s.Encode(&f.buffer); err != nil {
+		panic(err)
+	}
+	f.updateHash(f.buffer.Bytes()[lenBefore:])
+}
+
 // TODO: enable this or remove it...
 func (f *PlanGistFactory) encodeColumnOrdering(cols colinfo.ColumnOrdering) {
 }
@@ -373,10 +375,7 @@ func (f *PlanGistFactory) decodeColumnOrdering() colinfo.ColumnOrdering {
 }
 
 func (f *PlanGistFactory) encodeScanParams(params exec.ScanParams) {
-	err := params.NeededCols.Encode(f)
-	if err != nil {
-		panic(err)
-	}
+	f.encodeFastIntSet(params.NeededCols)
 
 	if params.IndexConstraint != nil {
 		f.encodeInt(params.IndexConstraint.Spans.Count())

pkg/sql/opt/optbuilder/insert.go

@@ -22,6 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
@@ -627,11 +628,18 @@ func (mb *mutationBuilder) buildInputForInsert(
 			checkDatumTypeFitsColumnType(mb.tab.Column(ord), inCol.typ)
 		}
 
-		// Check if the input column is created with `GENERATED ALWAYS AS IDENTITY`
-		// syntax. If yes, and user does not specify the `OVERRIDING SYSTEM VALUE`
-		// syntax in the `INSERT` statement,
-		// checkColumnIsNotGeneratedAlwaysAsIdentity will raise an error.
-		checkColumnIsNotGeneratedAlwaysAsIdentity(mb.tab.Column(ord))
+		// Raise an error if the target column is a `GENERATED ALWAYS AS
+		// IDENTITY` column. Such a column is not allowed to be explicitly
+		// written to.
+		//
+		// TODO(janexing): Implement the OVERRIDING SYSTEM VALUE syntax for
+		// INSERT which allows a GENERATED ALWAYS AS IDENTITY column to be
+		// overwritten.
+		// See https://github.com/cockroachdb/cockroach/issues/68201.
+		if col := mb.tab.Column(ord); col.IsGeneratedAlwaysAsIdentity() {
+			colName := string(col.ColName())
+			panic(sqlerrors.NewGeneratedAlwaysAsIdentityColumnOverrideError(colName))
+		}
 
 		// Assign name of input column.
 		inCol.name = scopeColName(tree.Name(mb.md.ColumnMeta(mb.targetColList[i]).Alias))