Merge #100017 #100204

100017: upgrades: update system.web_sessions migration to handle orphaned rows r=rafiss a=andyyang890 This patch updates the system.web_sessions user ID migration to delete orphaned rows (i.e. rows corresponding to users that have been dropped) after backfilling user IDs. They need to be deleted since they block the addition of the NOT NULL constraint on the column. Part of #87079 Release note: None 100204: physicalplan: debugging for segfault in fakeSpanResolverIterator.Seek r=rharding6373,cucaroach a=michae2 Break up a line that is segfaulting into several lines, so that we can tell which part is to blame if it happens again. Informs: #100051 Informs: #100108 Epic: None Release note: None Co-authored-by: Andy Yang <[email protected]> Co-authored-by: Michael Erickson <[email protected]>
cockroachdb · Mar 30, 2023 · f2ed466 · f2ed466
3 parents 2af36b8 + 1229975 + 16027ab
commit f2ed466
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 11 deletions.
diff --git a/pkg/sql/physicalplan/fake_span_resolver.go b/pkg/sql/physicalplan/fake_span_resolver.go
@@ -167,11 +167,15 @@ func (fit *fakeSpanResolverIterator) Seek(
 	// Build ranges corresponding to the fake splits and assign them random
 	// replicas.
 	fit.ranges = make([]fakeRange, len(splits)-1)
+	// TODO(michae2): Condense this logic when #100051 is fixed.
+	nodes := fit.fsr.nodes
+	n := len(nodes)
 	for i := range fit.ranges {
+		j := fit.rng.Intn(n)
 		fit.ranges[i] = fakeRange{
 			startKey: splits[i],
 			endKey:   splits[i+1],
-			replica:  fit.fsr.nodes[fit.rng.Intn(len(fit.fsr.nodes))],
+			replica:  nodes[j],
 		}
 	}
 

diff --git a/pkg/upgrade/upgrades/web_sessions_table_user_id_migration.go b/pkg/upgrade/upgrades/web_sessions_table_user_id_migration.go
@@ -53,6 +53,12 @@ WHERE w.user_id IS NULL AND w.username = u.username
 LIMIT 1000
 `
 
+const deleteRowsForDroppedUsersWebSessionsTableStmt = `
+DELETE FROM system.web_sessions
+WHERE user_id IS NULL
+LIMIT 1000
+`
+
 const setUserIDColumnToNotNullWebSessionsTableStmt = `
 ALTER TABLE system.web_sessions
 ALTER COLUMN user_id SET NOT NULL
@@ -62,16 +68,27 @@ func backfillWebSessionsTableUserIDColumn(
 	ctx context.Context, cs clusterversion.ClusterVersion, d upgrade.TenantDeps,
 ) error {
 	ie := d.DB.Executor()
-	for {
-		rowsAffected, err := ie.ExecEx(ctx, "backfill-user-id-col-web-sessions-table", nil, /* txn */
-			sessiondata.NodeUserSessionDataOverride,
-			backfillUserIDColumnWebSessionsTableStmt,
-		)
-		if err != nil {
-			return err
-		}
-		if rowsAffected == 0 {
-			break
+	for _, op := range []struct {
+		name  string
+		query string
+	}{
+		{
+			name:  "backfill-user-id-col-web-sessions-table",
+			query: backfillUserIDColumnWebSessionsTableStmt,
+		},
+		{
+			name:  "delete-rows-for-dropped-users-web-sessions-table",
+			query: deleteRowsForDroppedUsersWebSessionsTableStmt,
+		},
+	} {
+		for {
+			rowsAffected, err := ie.ExecEx(ctx, op.name, nil /* txn */, sessiondata.NodeUserSessionDataOverride, op.query)
+			if err != nil {
+				return err
+			}
+			if rowsAffected == 0 {
+				break
+			}
 		}
 	}
 

diff --git a/pkg/upgrade/upgrades/web_sessions_table_user_id_migration_test.go b/pkg/upgrade/upgrades/web_sessions_table_user_id_migration_test.go
@@ -80,6 +80,12 @@ VALUES (
 	})
 	tdb.CheckQueryResults(t, "SELECT count(*) FROM system.web_sessions", [][]string{{strconv.Itoa(numUsers)}})
 
+	// Drop a user to test that migration deletes orphaned rows.
+	if numUsers > 0 {
+		tdb.Exec(t, "DROP USER testuser0")
+		numUsers -= 1
+	}
+
 	// Run migrations.
 	_, err := tc.Conns[0].ExecContext(ctx, `SET CLUSTER SETTING version = $1`,
 		clusterversion.ByKey(clusterversion.V23_1WebSessionsTableHasUserIDColumn).String())