Merge cockroachdb#45347 cockroachdb#45502

45347: sql: make secondary indexes not write empty k/v's + bugfixes for primary key changes r=jordanlewis a=rohany

Fixes cockroachdb#45223.

Depends on cockroachdb#45226 to land first.

This PR fixes many bugs around secondary index encodings
and CRUD operations k/v reads and writes.

* Fixes a problem secondary indexes would write empty
  k/v's if it contained a family that had all null values.
* Fix multiple bugs where updates to a table during an online
  primary key change could result an inconsistent
  new primary key.
* Fix an assumption in the updater that assumed indexes
  always had the same number of k/v's. The logic has been
  updated to perform a sort of merge operation to decide
  what k/v's to insert/delete during the update operation.
* Increased testing around secondary indexes k/vs and
  schema change operations.

The release note is None because these are all bugs
introduced in 20.1.

Release note: None

45502: sql: allow rename database for sequences without a db name reference r=rohany a=otan

Resolves immediate concern from cockroachdb#45411
Refs: cockroachdb#34416

See release note for description. This PR should be included ahead of
the more "general" fix of changing the DefaultExpr with the new database
as it unblocks people using
`experimental_serial_normalization=sql_sequence` from using the database
rename feature.

Release note (sql change): Previously, when we renamed a database, any
table referencing a sequence would be blocked from being able to rename
the table. This is to block cases where if the table's reference to the
sequence contains the database name, and the database name changes, we
have no way of overwriting the table's reference to the sequence in the
new database. However, if no database name is included in the sequence
reference, we should continue to allow the database to rename, as is
implemented with this change.

Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
Co-authored-by: Oliver Tan <otan@cockroachlabs.com>

pkg/cmd/roachtest/secondary_indexes.go

@@ -66,17 +66,14 @@ INSERT INTO t VALUES (1, 2, 3, 4), (5, 6, 7, 8), (9, 10, 11, 12);
 		if _, err := conn.Exec(`INSERT INTO t VALUES (13, 14, 15, 16)`); err != nil {
 			t.Fatal(err)
 		}
-		verifyTable := func(conn *gosql.DB) {
+		if _, err := conn.Exec(`UPDATE t SET w = 17 WHERE y = 14`); err != nil {
+			t.Fatal(err)
+		}
+		verifyTable := func(conn *gosql.DB, expected [][]int) {
 			rows, err := conn.Query(`SELECT y, z, w FROM t@i ORDER BY y`)
 			if err != nil {
 				t.Fatal(err)
 			}
-			expected := [][]int{
-				{2, 3, 4},
-				{6, 7, 8},
-				{10, 11, 12},
-				{14, 15, 16},
-			}
 			var y, z, w int
 			count := 0
 			for ; rows.Next(); count++ {
@@ -87,17 +84,40 @@ INSERT INTO t VALUES (1, 2, 3, 4), (5, 6, 7, 8), (9, 10, 11, 12);
 				require.Equal(t, found, expected[count])
 			}
 		}
+		expected := [][]int{
+			{2, 3, 4},
+			{6, 7, 8},
+			{10, 11, 12},
+			{14, 15, 17},
+		}
 		for i := 1; i <= c.spec.NodeCount; i++ {
-			verifyTable(c.Conn(ctx, i))
+			verifyTable(c.Conn(ctx, i), expected)
 		}
 		t.Status("mixed version cluster passed test")
 
 		// Fully upgrade the cluster and ensure that the data is still valid.
 		for i := 2; i <= c.spec.NodeCount; i++ {
 			upgradeNode(i)
 		}
+
+		conn = c.Conn(ctx, 1)
+
+		if _, err := conn.Exec(`INSERT INTO t VALUES (20, 21, 22, 23)`); err != nil {
+			t.Fatal(err)
+		}
+		if _, err := conn.Exec(`UPDATE t SET w = 25, z = 25 WHERE y = 21`); err != nil {
+			t.Fatal(err)
+		}
+
+		expected = [][]int{
+			{2, 3, 4},
+			{6, 7, 8},
+			{10, 11, 12},
+			{14, 15, 17},
+			{21, 25, 25},
+		}
 		for i := 1; i <= c.spec.NodeCount; i++ {
-			verifyTable(c.Conn(ctx, i))
+			verifyTable(c.Conn(ctx, i), expected)
 		}
 		t.Status("passed on fully upgraded cluster")
 	}

pkg/sql/alter_table.go

@@ -449,6 +449,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 				CreatedExplicitly: true,
 				EncodingType:      sqlbase.PrimaryIndexEncoding,
 				Type:              sqlbase.IndexDescriptor_FORWARD,
+				Version:           sqlbase.SecondaryIndexFamilyFormatVersion,
 			}
 
 			// If the new index is requested to be sharded, set up the index descriptor
@@ -490,6 +491,25 @@ func (n *alterTableNode) startExec(params runParams) error {
 				return err
 			}
 
+			// Ensure that the new primary index stores all columns in the table. We can't
+			// use AllocateID's to fill the stored columns here because it assumes
+			// that the indexed columns are n.PrimaryIndex.ColumnIDs, but here we want
+			// to consider the indexed columns to be newPrimaryIndexDesc.ColumnIDs.
+			newPrimaryIndexDesc.StoreColumnNames, newPrimaryIndexDesc.StoreColumnIDs = nil, nil
+			for _, col := range n.tableDesc.Columns {
+				containsCol := false
+				for _, colID := range newPrimaryIndexDesc.ColumnIDs {
+					if colID == col.ID {
+						containsCol = true
+						break
+					}
+				}
+				if !containsCol {
+					newPrimaryIndexDesc.StoreColumnIDs = append(newPrimaryIndexDesc.StoreColumnIDs, col.ID)
+					newPrimaryIndexDesc.StoreColumnNames = append(newPrimaryIndexDesc.StoreColumnNames, col.Name)
+				}
+			}
+
 			if t.Interleave != nil {
 				if err := params.p.addInterleave(params.ctx, n.tableDesc, newPrimaryIndexDesc, t.Interleave); err != nil {
 					return err

pkg/sql/backfill/backfill.go

@@ -422,11 +422,12 @@ func (ib *IndexBackfiller) BuildIndexEntriesChunk(
 		// We're resetting the length of this slice for variable length indexes such as inverted
 		// indexes which can append entries to the end of the slice. If we don't do this, then everything
 		// EncodeSecondaryIndexes appends to secondaryIndexEntries for a row, would stay in the slice for
-		// subsequent rows and we would then have duplicates in entries on output.
+		// subsequent rows and we would then have duplicates in entries on output. Additionally, we do
+		// not want to include empty k/v pairs while backfilling.
 		buffer = buffer[:0]
 		if buffer, err = sqlbase.EncodeSecondaryIndexes(
 			tableDesc.TableDesc(), ib.added, ib.colIdxMap,
-			ib.rowVals, buffer); err != nil {
+			ib.rowVals, buffer, false /* includeEmpty */); err != nil {
 			return nil, nil, err
 		}
 		entries = append(entries, buffer...)

pkg/sql/logictest/testdata/logic_test/rename_database

@@ -136,7 +136,7 @@ ALTER DATABASE u RENAME TO v
 statement ok
 CREATE VIEW v.v AS SELECT k,v FROM v.kv
 
-statement error cannot rename database because relation "v.public.v" depends on relation "v.public.kv"
+statement error cannot rename database because relation "v.public.v" depends on relation "v.public.kv"\s.*you can drop "v.public.v" instead
 ALTER DATABASE v RENAME TO u
 
 # Check that the default databases can be renamed like any other.
@@ -173,7 +173,8 @@ t
 v
 
 # Test dependent sequences on different databases upon renames
-# return the appropriate error message.
+# return the appropriate error message, as well as testing
+# renaming databases with sequences in the same DB is successful.
 subtest regression_45411
 
 statement ok
@@ -182,8 +183,26 @@ CREATE DATABASE db1; CREATE SEQUENCE db1.seq
 statement ok
 CREATE DATABASE db2; CREATE TABLE db2.tbl (a int DEFAULT nextval('db1.seq'))
 
-statement error cannot rename database because relation "db2.public.tbl" depends on relation "db1.public.seq"
+statement error cannot rename database because relation "db2.public.tbl" depends on relation "db1.public.seq"\s.*you can drop the column default "a" of "db1.public.seq" referencing "db2.public.tbl"
 ALTER DATABASE db1 RENAME TO db3
 
 statement ok
 DROP DATABASE db2 CASCADE; DROP DATABASE db1 CASCADE
+
+statement ok
+CREATE DATABASE db1; CREATE SEQUENCE db1.a_seq; CREATE SEQUENCE db1.b_seq; USE db1;
+
+statement ok
+CREATE TABLE db1.a (a int default nextval('a_seq') + nextval('b_seq') + 1); ALTER DATABASE db1 RENAME TO db2; USE db2;
+
+statement error cannot rename database because relation "db2.public.a" depends on relation "db2.public.a_seq"\s.*you can drop the column default "a" of "db2.public.a_seq" referencing "db2.public.a" or modify the default to not reference the database name "db2"
+DROP TABLE db2.a; CREATE TABLE db2.a (a int default nextval('a_seq') + nextval('db2.b_seq') + 1); ALTER DATABASE db2 RENAME TO db1
+
+statement error cannot rename database because relation "db2.public.a" depends on relation "db2.public.a_seq"\s.*you can drop the column default "a" of "db2.public.a_seq" referencing "db2.public.a" or modify the default to not reference the database name "db2"
+DROP TABLE db2.a; CREATE TABLE db2.a (a int default nextval('a_seq') + nextval('db2.public.b_seq') + 1); ALTER DATABASE db2 RENAME TO db1
+
+statement ok
+DROP TABLE db2.a; CREATE TABLE db2.a (a int default nextval('a_seq') + nextval('public.b_seq') + 1); ALTER DATABASE db2 RENAME TO db1
+
+statement ok
+USE defaultdb; DROP DATABASE db1 CASCADE