Merge #78435 #78524 #78533 #78607 #78609

78435: opt: fix deduplication of unique constraints in test catalog r=mgartner a=mgartner

This commit fixes a bug in the test catalog which incorrectly
de-duplicated unique constraints that had the same columns but different
predicates.

Fixes #76994

Release note: None

78524: zcfg: rename UpdateZoneConfigForTables to RefreshZoneConfigsForTable r=arulajmani a=RichardJCai

Release note: None

Fixes #77641

78533: ui: don't download .dll.js bundles r=sjbarag a=sjbarag

Loading the CockroachDB admin UI (served from port 8080) would previously download three JS files: `bundle.js`, `vendor.dll.js`, and `protos.dll.js`. Neither `vendor.dll.js` nor `protos.dll.js` need to be included via `<script/>` tag, as they're used only as build-time tooling to accelerate webpack builds. Only a single file -- `bundle.js` -- needs to be loaded for the admin UI to be functional, and Bazel-produced builds don't have any `.dll.js` files to serve. Don't load `.dll.js` files via index.html.

Since dll file aren't being served directly to users anymore, there's no need to embed them in the `cockroach` binary. Stop copying `.dll.js` files into `pkg/ui/dist{oss,ccl}` during builds, reducing the `cockroach` binary size by ~19MB (darwin, x86) when built with GNU make.

78607: sqlsmith: skip crdb_internal.complete_replication_stream r=rytaft a=rytaft

Don't use crdb_internal.complete_replication_stream when generating
sqlsmith queries, since it currently causes a panic.

Informs #78553

Release note: None

78609: cli: de-flake TestDockerCLI/test_demo_node_cmds.tcl r=ajwerner a=tbg

Touches #76391.
(Not closing since `@ajwerner` requested so)

This test is very flaky, so we need a stop-gap. It's also not really useful to get into the gossip info from a cli test.

Release note: None


Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: richardjcai <[email protected]>
Co-authored-by: Sean Barag <[email protected]>
Co-authored-by: Rebecca Taft <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>

Makefile

@@ -1409,9 +1409,6 @@ pkg/ui/assets.ccl.installed: $(UI_CCL_DLLS) $(UI_CCL_MANIFESTS) $(UI_JS_CCL) $(s
 pkg/ui/assets.oss.installed: $(UI_OSS_DLLS) $(UI_OSS_MANIFESTS) $(UI_JS_OSS)
 pkg/ui/assets.%.installed: pkg/ui/workspaces/db-console/webpack.app.js $(shell find pkg/ui/workspaces/db-console/src pkg/ui/workspaces/db-console/styl -type f) | bin/.bootstrap
 	find pkg/ui/dist$*/assets -mindepth 1 -not -name .gitkeep -delete
-	for dll in $(shell find pkg/ui/workspaces/db-console/dist -name '*.dll.js' -type f); do \
-		echo $$dll | sed -E "s/.oss.dll.js|.ccl.dll.js/.dll.js/" | sed -E "s|^.*\/|pkg/ui/dist$*/assets/|" | xargs -I{} cp $$dll {};\
-	done
 	$(NODE_RUN) -C pkg/ui/workspaces/db-console $(WEBPACK) --config webpack.app.js --env.dist=$*
 	touch $@
 

pkg/cli/interactive_tests/test_demo_node_cmds.tcl

@@ -64,26 +64,31 @@ send "\\demo restart 3\r"
 eexpect "node 3 has been restarted"
 eexpect "movr>"
 
-send "select node_id, draining, decommissioning, membership from crdb_internal.gossip_liveness ORDER BY node_id;\r"
-eexpect "1 |  false   |      false      | active"
-eexpect "2 |  false   |      false      | active"
-eexpect "3 |  false   |      false      | active"
-eexpect "4 |  false   |      false      | active"
-eexpect "5 |  false   |      false      | active"
-eexpect "movr>"
+# NB: this is flaky, sometimes n3 is still marked as draining due to
+# gossip propagation delays. See:
+# https://github.com/cockroachdb/cockroach/issues/76391
+# send "select node_id, draining, decommissioning, membership from crdb_internal.gossip_liveness ORDER BY node_id;\r"
+# eexpect "1 |  false   |      false      | active"
+# eexpect "2 |  false   |      false      | active"
+# eexpect "3 |  false   |      false      | active"
+# eexpect "4 |  false   |      false      | active"
+# eexpect "5 |  false   |      false      | active"
+# eexpect "movr>"
 
 # Try commissioning commands
 send "\\demo decommission 4\r"
 eexpect "node 4 has been decommissioned"
 eexpect "movr>"
 
-send "select node_id, draining, decommissioning, membership from crdb_internal.gossip_liveness ORDER BY node_id;\r"
-eexpect "1 |  false   |      false      | active"
-eexpect "2 |  false   |      false      | active"
-eexpect "3 |  false   |      false      | active"
-eexpect "4 |  false   |      true       | decommissioned"
-eexpect "5 |  false   |      false      | active"
-eexpect "movr>"
+# NB: skipping this out of an abundance of caution, see:
+# https://github.com/cockroachdb/cockroach/issues/76391
+# send "select node_id, draining, decommissioning, membership from crdb_internal.gossip_liveness ORDER BY node_id;\r"
+# eexpect "1 |  false   |      false      | active"
+# eexpect "2 |  false   |      false      | active"
+# eexpect "3 |  false   |      false      | active"
+# eexpect "4 |  false   |      true       | decommissioned"
+# eexpect "5 |  false   |      false      | active"
+# eexpect "movr>"
 
 send "\\demo recommission 4\r"
 eexpect "can only recommission a decommissioning node"
@@ -119,14 +124,16 @@ eexpect "node 6 has been shutdown"
 eexpect "movr>"
 
 # By now the node should have stabilized in gossip which allows us to query the more detailed information there.
-send "select node_id, draining, decommissioning, membership from crdb_internal.gossip_liveness ORDER BY node_id;\r"
-eexpect "1 |  false   |      false      | active"
-eexpect "2 |  false   |      false      | active"
-eexpect "3 |  false   |      false      | active"
-eexpect "4 |  false   |      true       | decommissioned"
-eexpect "5 |  false   |      false      | active"
-eexpect "6 |   true   |      false      | active"
-eexpect "movr>"
+# NB: skip this to avoid flakes, see:
+# https://github.com/cockroachdb/cockroach/issues/76391
+# send "select node_id, draining, decommissioning, membership from crdb_internal.gossip_liveness ORDER BY node_id;\r"
+# eexpect "1 |  false   |      false      | active"
+# eexpect "2 |  false   |      false      | active"
+# eexpect "3 |  false   |      false      | active"
+# eexpect "4 |  false   |      true       | decommissioned"
+# eexpect "5 |  false   |      false      | active"
+# eexpect "6 |   true   |      false      | active"
+# eexpect "movr>"
 
 send_eof
 eexpect eof

pkg/internal/sqlsmith/schema.go

@@ -497,6 +497,7 @@ var functions = func() map[tree.FunctionClass]map[oid.Oid][]function {
 			"crdb_internal.reset_index_usage_stats",
 			"crdb_internal.start_replication_stream",
 			"crdb_internal.replication_stream_progress",
+			"crdb_internal.complete_replication_stream",
 		} {
 			skip = skip || strings.Contains(def.Name, substr)
 		}

pkg/server/server_test.go

@@ -829,8 +829,6 @@ func TestServeIndexHTML(t *testing.T) {
 			window.dataFromServer = %s;
 		</script>
 
-		<script src="protos.dll.js" type="text/javascript"></script>
-		<script src="vendor.dll.js" type="text/javascript"></script>
 		<script src="bundle.js" type="text/javascript"></script>
 	</body>
 </html>

pkg/sql/alter_database.go

@@ -803,7 +803,7 @@ func (n *alterDatabasePrimaryRegionNode) switchPrimaryRegion(params runParams) e
 	// If there are super regions defined on the database, we may have to update
 	// the zone config for regional tables.
 	if updatedRegionConfig.IsPlacementRestricted() || isNewPrimaryRegionMemberOfASuperRegion || isOldPrimaryRegionMemberOfASuperRegion {
-		if err := params.p.updateZoneConfigsForTables(
+		if err := params.p.refreshZoneConfigsForTables(
 			params.ctx,
 			n.desc,
 			opts,
@@ -1131,7 +1131,7 @@ func (n *alterDatabaseSurvivalGoalNode) startExec(params runParams) error {
 
 	// Update all REGIONAL BY TABLE tables' zone configurations. This is required as replica
 	// placement for REGIONAL BY TABLE tables is dependant on the survival goal.
-	if err := params.p.updateZoneConfigsForTables(params.ctx, n.desc); err != nil {
+	if err := params.p.refreshZoneConfigsForTables(params.ctx, n.desc); err != nil {
 		return err
 	}
 
@@ -1267,7 +1267,7 @@ func (n *alterDatabasePlacementNode) startExec(params runParams) error {
 	// -> DEFAULT), we need to refresh the zone configuration of all GLOBAL
 	// table's inside the database to either carry a bespoke configuration or go
 	// back to inheriting it from the database.
-	if err := params.p.updateZoneConfigsForTables(
+	if err := params.p.refreshZoneConfigsForTables(
 		params.ctx,
 		n.desc,
 		WithOnlyGlobalTables,
@@ -1438,7 +1438,7 @@ func (n *alterDatabaseDropSuperRegion) startExec(params runParams) error {
 	}
 
 	// Update all regional and regional by row tables.
-	if err := params.p.updateZoneConfigsForTables(
+	if err := params.p.refreshZoneConfigsForTables(
 		params.ctx,
 		n.desc,
 	); err != nil {
@@ -1634,7 +1634,7 @@ func (p *planner) addSuperRegion(
 	}
 
 	// Update all regional and regional by row tables.
-	return p.updateZoneConfigsForTables(
+	return p.refreshZoneConfigsForTables(
 		ctx,
 		desc,
 	)

pkg/sql/database_region_change_finalizer.go

@@ -182,7 +182,7 @@ func (r *databaseRegionChangeFinalizer) updateGlobalTablesZoneConfig(
 		return err
 	}
 
-	err = r.localPlanner.updateZoneConfigsForTables(ctx, dbDesc, WithOnlyGlobalTables)
+	err = r.localPlanner.refreshZoneConfigsForTables(ctx, dbDesc, WithOnlyGlobalTables)
 	if err != nil {
 		return err
 	}

pkg/sql/opt/testutils/testcat/create_table.go

@@ -553,14 +553,7 @@ func (tt *Table) addUniqueConstraint(
 	}
 	sort.Ints(cols)
 
-	// Don't add duplicate constraints.
-	for _, c := range tt.uniqueConstraints {
-		if reflect.DeepEqual(c.columnOrdinals, cols) && c.withoutIndex == withoutIndex {
-			return
-		}
-	}
-
-	// We didn't find an existing constraint, so add a new one.
+	// Create the constraint.
 	u := UniqueConstraint{
 		name:           tt.makeUniqueConstraintName(name, columns),
 		tabID:          tt.TabID,
@@ -572,6 +565,16 @@ func (tt *Table) addUniqueConstraint(
 	if predicate != nil {
 		u.predicate = tree.Serialize(predicate)
 	}
+
+	// Don't add duplicate constraints.
+	for _, c := range tt.uniqueConstraints {
+		if reflect.DeepEqual(c.columnOrdinals, u.columnOrdinals) &&
+			c.predicate == u.predicate &&
+			c.withoutIndex == u.withoutIndex {
+			return
+		}
+	}
+
 	tt.uniqueConstraints = append(tt.uniqueConstraints, u)
 }
 

pkg/sql/opt/testutils/testcat/testdata/table

@@ -386,3 +386,32 @@ TABLE virtpk
  └── PRIMARY INDEX virtpk_pkey
       ├── v int not null as (a + 10) stored
       └── a int not null
+
+# Regression test for #76994. Only unique constraints with the same columns and
+# predicates should be deduplicated.
+exec-ddl
+CREATE TABLE t76994 (
+  k INT PRIMARY KEY,
+  a INT,
+  b INT,
+  UNIQUE WITHOUT INDEX (a) WHERE b < 0,
+  UNIQUE WITHOUT INDEX (a) WHERE b < 0,
+  UNIQUE WITHOUT INDEX (a) WHERE b > 0
+)
+----
+
+exec-ddl
+SHOW CREATE t76994
+----
+TABLE t76994
+ ├── k int not null
+ ├── a int
+ ├── b int
+ ├── crdb_internal_mvcc_timestamp decimal [hidden] [system]
+ ├── tableoid oid [hidden] [system]
+ ├── PRIMARY INDEX t76994_pkey
+ │    └── k int not null
+ ├── UNIQUE WITHOUT INDEX (a)
+ │    └── WHERE b < 0
+ └── UNIQUE WITHOUT INDEX (a)
+      └── WHERE b > 0

pkg/sql/region_util.go

@@ -869,21 +869,21 @@ func applyZoneConfigForMultiRegionDatabase(
 	return nil
 }
 
-type updateZoneConfigOptions struct {
+type refreshZoneConfigOptions struct {
 	filterFunc func(tb *tabledesc.Mutable) bool
 }
 
-type updateZoneConfigOption func(options *updateZoneConfigOptions)
+type refreshZoneConfigOption func(options *refreshZoneConfigOptions)
 
-// updateZoneConfigsForTables loops through all of the tables in the
+// refreshZoneConfigsForTables loops through all of the tables in the
 // specified database and refreshes the zone configs for all tables.
-func (p *planner) updateZoneConfigsForTables(
-	ctx context.Context, desc catalog.DatabaseDescriptor, updateOpts ...updateZoneConfigOption,
+func (p *planner) refreshZoneConfigsForTables(
+	ctx context.Context, desc catalog.DatabaseDescriptor, refreshOpts ...refreshZoneConfigOption,
 ) error {
-	opts := updateZoneConfigOptions{
+	opts := refreshZoneConfigOptions{
 		filterFunc: func(_ *tabledesc.Mutable) bool { return true },
 	}
-	for _, f := range updateOpts {
+	for _, f := range refreshOpts {
 		f(&opts)
 	}
 
@@ -911,17 +911,17 @@ func (p *planner) updateZoneConfigsForTables(
 	)
 }
 
-// WithOnlyGlobalTables modifies an updateZoneConfigOptions to only apply to
+// WithOnlyGlobalTables modifies an refreshZoneConfigOptions to only apply to
 // global tables.
-func WithOnlyGlobalTables(opts *updateZoneConfigOptions) {
+func WithOnlyGlobalTables(opts *refreshZoneConfigOptions) {
 	opts.filterFunc = func(tb *tabledesc.Mutable) bool {
 		return tb.IsLocalityGlobal()
 	}
 }
 
-// WithOnlyRegionalTablesAndGlobalTables modifies an updateZoneConfigOptions to
+// WithOnlyRegionalTablesAndGlobalTables modifies an refreshZoneConfigOptions to
 // only apply to global tables and regional tables.
-func WithOnlyRegionalTablesAndGlobalTables(opts *updateZoneConfigOptions) {
+func WithOnlyRegionalTablesAndGlobalTables(opts *refreshZoneConfigOptions) {
 	opts.filterFunc = func(tb *tabledesc.Mutable) bool {
 		return tb.IsLocalityGlobal() || tb.IsLocalityRegionalByTable()
 	}

pkg/ui/ui.go

@@ -56,8 +56,6 @@ var indexHTMLTemplate = template.Must(template.New("index").Parse(`<!DOCTYPE htm
 			window.dataFromServer = {{.}};
 		</script>
 
-		<script src="protos.dll.js" type="text/javascript"></script>
-		<script src="vendor.dll.js" type="text/javascript"></script>
 		<script src="bundle.js" type="text/javascript"></script>
 	</body>
 </html>