Merge #77561 #77775

77561: changefeedccl: set server.child_metrics.enabled public column to true r=sherman-grewal a=sherman-grewal

changefeedccl: set server.child_metrics.enabled public column
to true

Resolves #77536

Since we're expecting people to use this setting to
use metrics labels, it should show up when running SHOW
ALL CLUSTER SETTINGS.

Release note (enterprise change): The public column
of setting server.child_metrics.enabled will be set
to true.

Release justification: This change is very small and low
risk

77775: opt: fix upsert fast path for hash-sharded primary indexes r=mgartner a=mgartner

In #76358, a change was made to consider both partitioning and
hash-shard columns as implicit, which was required for finding arbiters
for UPSERT statements in tables that have partitioned, hash-sharded
primary indexes. A side-effect of this change is that it prevents the
planning of the fast-path of UPSERTs into tables with hash-sharded
primary indexes. This commit fixes this regression.

Release justification: This fixes a performance regression for
hash-sharded indexes, which are being released as non-experimental in
the upcoming release.

There is no release note because this regression is not present in any
releases.

Release note: None

Co-authored-by: Sherman Grewal <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>

docs/generated/settings/settings-for-tenants.txt

@@ -47,6 +47,7 @@ security.ocsp.timeout	duration	3s	timeout before considering the OCSP server unr
 server.auth_log.sql_connections.enabled	boolean	false	if set, log SQL client connect and disconnect events (note: may hinder performance on loaded nodes)
 server.auth_log.sql_sessions.enabled	boolean	false	if set, log SQL session login/disconnection events (note: may hinder performance on loaded nodes)
 server.authentication_cache.enabled	boolean	true	enables a cache used during authentication to avoid lookups to system tables when retrieving per-user authentication-related information
+server.child_metrics.enabled	boolean	false	enables the exporting of child metrics, additional prometheus time series with extra labels
 server.clock.forward_jump_check_enabled	boolean	false	if enabled, forward clock jumps > max_offset/2 will cause a panic
 server.clock.persist_upper_bound_interval	duration	0s	the interval between persisting the wall time upper bound of the clock. The clock does not generate a wall time greater than the persisted timestamp and will panic if it sees a wall time greater than this value. When cockroach starts, it waits for the wall time to catch-up till this persisted timestamp. This guarantees monotonic wall time across server restarts. Not setting this or setting a value of 0 disables this feature.
 server.eventlog.enabled	boolean	true	if set, logged notable events are also stored in the table system.eventlog

docs/generated/settings/settings.html

@@ -60,6 +60,7 @@
 <tr><td><code>server.auth_log.sql_connections.enabled</code></td><td>boolean</td><td><code>false</code></td><td>if set, log SQL client connect and disconnect events (note: may hinder performance on loaded nodes)</td></tr>
 <tr><td><code>server.auth_log.sql_sessions.enabled</code></td><td>boolean</td><td><code>false</code></td><td>if set, log SQL session login/disconnection events (note: may hinder performance on loaded nodes)</td></tr>
 <tr><td><code>server.authentication_cache.enabled</code></td><td>boolean</td><td><code>true</code></td><td>enables a cache used during authentication to avoid lookups to system tables when retrieving per-user authentication-related information</td></tr>
+<tr><td><code>server.child_metrics.enabled</code></td><td>boolean</td><td><code>false</code></td><td>enables the exporting of child metrics, additional prometheus time series with extra labels</td></tr>
 <tr><td><code>server.clock.forward_jump_check_enabled</code></td><td>boolean</td><td><code>false</code></td><td>if enabled, forward clock jumps > max_offset/2 will cause a panic</td></tr>
 <tr><td><code>server.clock.persist_upper_bound_interval</code></td><td>duration</td><td><code>0s</code></td><td>the interval between persisting the wall time upper bound of the clock. The clock does not generate a wall time greater than the persisted timestamp and will panic if it sees a wall time greater than this value. When cockroach starts, it waits for the wall time to catch-up till this persisted timestamp. This guarantees monotonic wall time across server restarts. Not setting this or setting a value of 0 disables this feature.</td></tr>
 <tr><td><code>server.consistency_check.max_rate</code></td><td>byte size</td><td><code>8.0 MiB</code></td><td>the rate limit (bytes/sec) to use for consistency checks; used in conjunction with server.consistency_check.interval to control the frequency of consistency checks. Note that setting this too high can negatively impact performance.</td></tr>

pkg/server/status/recorder.go

@@ -90,7 +90,7 @@ type storeMetrics interface {
 var childMetricsEnabled = settings.RegisterBoolSetting(
 	settings.TenantWritable, "server.child_metrics.enabled",
 	"enables the exporting of child metrics, additional prometheus time series with extra labels",
-	false)
+	false).WithPublic()
 
 // MetricsRecorder is used to periodically record the information in a number of
 // metric registries.

pkg/sql/opt/cat/index.go

@@ -182,6 +182,11 @@ type Index interface {
 	// list, and ImplicitColumnCount < LaxKeyColumnCount.
 	ImplicitColumnCount() int
 
+	// ImplicitPartitioningColumnCount returns the number of implicit columns at
+	// the front of the index that are implicit partitioning columns. The value
+	// returned is always <= ImplicitColumnCount.
+	ImplicitPartitioningColumnCount() int
+
 	// GeoConfig returns a geospatial index configuration. If non-nil, it
 	// describes the configuration for this geospatial inverted index.
 	GeoConfig() *geoindex.Config

pkg/sql/opt/exec/execbuilder/testdata/hash_sharded_index

@@ -677,8 +677,6 @@ vectorized: true
           spans: /6/1234/0
           locking strength: for update
 
-# TODO(mgartner): We should be able to perform the UPSERT fast path in this
-# case, because the shard column is a function of the other PK columns.
 query T
 EXPLAIN (VERBOSE) UPSERT INTO t_hash_indexed VALUES (4321, 8765);
 ----
@@ -690,39 +688,17 @@ vectorized: true
 │ estimated row count: 0 (missing stats)
 │ into: t_hash_indexed(crdb_internal_a_shard_8, a, b)
 │ auto commit
-│ arbiter constraints: t_hash_indexed_pkey
 │
 └── • project
-    │ columns: (crdb_internal_a_shard_8_comp, column1, column2, crdb_internal_a_shard_8, a, b, crdb_internal_a_shard_8_comp, column2, crdb_internal_a_shard_8, check1)
+    │ columns: (crdb_internal_a_shard_8_comp, column1, column2, column2, check1)
     │
-    └── • render
-        │ columns: (check1, column1, column2, crdb_internal_a_shard_8_comp, crdb_internal_a_shard_8, a, b)
-        │ estimated row count: 1 (missing stats)
-        │ render check1: crdb_internal_a_shard_8_comp IN (0, 1, 2, 3, 4, 5, 6, 7)
-        │ render column1: column1
-        │ render column2: column2
-        │ render crdb_internal_a_shard_8_comp: crdb_internal_a_shard_8_comp
-        │ render crdb_internal_a_shard_8: crdb_internal_a_shard_8
-        │ render a: a
-        │ render b: b
-        │
-        └── • cross join (left outer)
-            │ columns: (column1, column2, crdb_internal_a_shard_8_comp, crdb_internal_a_shard_8, a, b)
-            │ estimated row count: 1 (missing stats)
-            │
-            ├── • values
-            │     columns: (column1, column2, crdb_internal_a_shard_8_comp)
-            │     size: 3 columns, 1 row
-            │     row 0, expr 0: 4321
-            │     row 0, expr 1: 8765
-            │     row 0, expr 2: 1
-            │
-            └── • scan
-                  columns: (crdb_internal_a_shard_8, a, b)
-                  estimated row count: 1 (missing stats)
-                  table: t_hash_indexed@t_hash_indexed_pkey
-                  spans: /1/4321/0
-                  locking strength: for update
+    └── • values
+          columns: (column1, column2, crdb_internal_a_shard_8_comp, check1)
+          size: 4 columns, 1 row
+          row 0, expr 0: 4321
+          row 0, expr 1: 8765
+          row 0, expr 2: 1
+          row 0, expr 3: true
 
 query T
 EXPLAIN (VERBOSE) INSERT INTO t_hash_indexed VALUES (4321, 8765) ON CONFLICT (a) DO UPDATE SET a = 4321

pkg/sql/opt/indexrec/hypothetical_index.go

@@ -200,6 +200,11 @@ func (hi *hypotheticalIndex) ImplicitColumnCount() int {
 	return 0
 }
 
+// ImplicitPartitioningColumnCount is part of the cat.Index interface.
+func (hi *hypotheticalIndex) ImplicitPartitioningColumnCount() int {
+	return 0
+}
+
 // GeoConfig is part of the cat.Index interface.
 // TODO(nehageorge): Add support for spatial index recommendations.
 func (hi *hypotheticalIndex) GeoConfig() *geoindex.Config {

pkg/sql/opt/optbuilder/insert.go

@@ -365,10 +365,7 @@ func (mb *mutationBuilder) needExistingRows() bool {
 	// If there are any implicit partitioning columns in the primary index,
 	// these columns will need to be fetched.
 	primaryIndex := mb.tab.Index(cat.PrimaryIndex)
-	// TODO(mgartner): It should be possible to perform an UPSERT fast path for a
-	// non-partitioned hash-sharded primary index, but this restriction disallows
-	// it.
-	if primaryIndex.ImplicitColumnCount() > 0 {
+	if primaryIndex.ImplicitPartitioningColumnCount() > 0 {
 		return true
 	}
 
@@ -865,7 +862,7 @@ func (mb *mutationBuilder) setUpsertCols(insertCols tree.NameList) {
 	// Never update primary key columns. Implicit partitioning columns are not
 	// considered part of the primary key in this case.
 	conflictIndex := mb.tab.Index(cat.PrimaryIndex)
-	skipCols := conflictIndex.ImplicitColumnCount()
+	skipCols := conflictIndex.ImplicitPartitioningColumnCount()
 	for i, n := skipCols, conflictIndex.KeyColumnCount(); i < n; i++ {
 		mb.updateColIDs[conflictIndex.Column(i).Ordinal()] = 0
 	}

pkg/sql/opt/optbuilder/mutation_builder.go

@@ -1183,7 +1183,8 @@ func getUniqueConstraintOrdinals(tab cat.Table, uc cat.UniqueConstraint) util.Fa
 }
 
 // getExplicitPrimaryKeyOrdinals returns the ordinals of the primary key
-// columns, excluding any implicit partitioning columns in the primary index.
+// columns, excluding any implicit partitioning or hash-shard columns in the
+// primary index.
 func getExplicitPrimaryKeyOrdinals(tab cat.Table) util.FastIntSet {
 	index := tab.Index(cat.PrimaryIndex)
 	skipCols := index.ImplicitColumnCount()

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

@@ -963,6 +963,11 @@ func (ti *Index) ImplicitColumnCount() int {
 	return 0
 }
 
+// ImplicitPartitioningColumnCount is part of the cat.Index interface.
+func (ti *Index) ImplicitPartitioningColumnCount() int {
+	return 0
+}
+
 // GeoConfig is part of the cat.Index interface.
 func (ti *Index) GeoConfig() *geoindex.Config {
 	return ti.geoConfig

pkg/sql/opt_catalog.go

@@ -1455,6 +1455,11 @@ func (oi *optIndex) ImplicitColumnCount() int {
 	return implicitColCnt
 }
 
+// ImplicitPartitioningColumnCount is part of the cat.Index interface.
+func (oi *optIndex) ImplicitPartitioningColumnCount() int {
+	return oi.idx.GetPartitioning().NumImplicitColumns()
+}
+
 // GeoConfig is part of the cat.Index interface.
 func (oi *optIndex) GeoConfig() *geoindex.Config {
 	return &oi.idx.IndexDesc().GeoConfig
@@ -2227,6 +2232,11 @@ func (oi *optVirtualIndex) ImplicitColumnCount() int {
 	return 0
 }
 
+// ImplicitPartitioningColumnCount is part of the cat.Index interface.
+func (oi *optVirtualIndex) ImplicitPartitioningColumnCount() int {
+	return 0
+}
+
 // GeoConfig is part of the cat.Index interface.
 func (oi *optVirtualIndex) GeoConfig() *geoindex.Config {
 	return nil