Merge #46408 #46431 #46433 #46444

46408: opt: fix buggy EliminateUpsertDistinctOnNoColumns rule r=andy-kimball a=andy-kimball

The fix for #37880 added a new ErrorOnDup field to the UpsertDistinctOn
operator. However, the EliminateErrorDistinctNoColumns rule wasn't changed
to respect this flag, and so there's still a case where ON CONFLICT DO
NOTHING returns an error.

This commit eliminates the error by separating out the ErrorOnDup=True case
from the EliminateErrorDistinctNoColumns rule (which raises an error) and
adding it instead to the EliminateDistinctNoColumns rule (which does not
raise an error).

Fixes #46395

Release justification: Bug fixes and low-risk updates to new functionality.
This was always an error, so it's low-risk to make it a non-error.

Release note: None

46431: kv: don't disable the merge queue needlessly in more tests r=nvanbenschoten a=nvanbenschoten

Follow up to #46383.

These tests were disabling the queue to not interfere with its
AdminSplits, but since the tests were written, AdminSplit got
a TTL.

Release note: None
Release justification: test only

46433: kv: re-enable merge queue in TestSplitTriggerRaftSnapshotRace r=nvanbenschoten a=nvanbenschoten

The merge queue was disabled in this test by df26cf6 due to the bug
found in #32784. That bug was fixed by #33312, so we can address the
TODO and re-enable merges in the test.

Release note: None
Release justification: test only

46444: kv: deflake TestRangeInfo by using a manual clock r=nvanbenschoten a=nvanbenschoten

Fixes #46215.

Fallout from #45984.

Release justification: testing only
Release note: None.

Co-authored-by: Andrew Kimball <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>

pkg/kv/kvclient/kvcoord/dist_sender_server_test.go

@@ -1312,8 +1312,6 @@ func TestPropagateTxnOnError(t *testing.T) {
 			}
 			return nil
 		}
-	// Don't clobber the test's splits.
-	storeKnobs.DisableMergeQueue = true
 
 	s, _, _ := serverutils.StartServer(t,
 		base.TestServerArgs{Knobs: base.TestingKnobs{Store: &storeKnobs}})

pkg/kv/kvserver/client_replica_test.go

@@ -1454,6 +1454,7 @@ func TestRangeInfo(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	storeCfg := kvserver.TestStoreConfig(nil /* clock */)
 	storeCfg.TestingKnobs.DisableMergeQueue = true
+	storeCfg.Clock = nil // manual clock
 	mtc := &multiTestContext{
 		storeConfig: &storeCfg,
 		// This test was written before the multiTestContext started creating many

pkg/kv/kvserver/client_split_test.go

@@ -477,8 +477,6 @@ func TestSplitTriggerRaftSnapshotRace(t *testing.T) {
 	ctx := context.Background()
 	const numNodes = 3
 	var args base.TestClusterArgs
-	// TODO(benesch): remove this while closing #32784.
-	args.ServerArgs.Knobs.Store = &kvserver.StoreTestingKnobs{DisableMergeQueue: true}
 	// NB: the merge queue is enabled for additional "chaos". Note that the test
 	// uses three nodes and so there is no replica movement, which would other-
 	// wise tickle Raft snapshots for unrelated reasons.

pkg/kv/kvserver/client_status_test.go

@@ -15,7 +15,6 @@ import (
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/kv"
-	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -24,11 +23,7 @@ import (
 
 func TestComputeStatsForKeySpan(t *testing.T) {
 	defer leaktest.AfterTest(t)()
-	storeCfg := kvserver.TestStoreConfig(nil /* clock */)
-	storeCfg.TestingKnobs.DisableMergeQueue = true
-	mtc := &multiTestContext{
-		storeConfig: &storeCfg,
-	}
+	mtc := &multiTestContext{}
 	defer mtc.Stop()
 	mtc.Start(t, 3)
 

pkg/sql/logictest/testdata/logic_test/upsert

@@ -1000,6 +1000,18 @@ UPSERT INTO t44466 (c2, c0) VALUES (0, 0), (1, 0)
 statement ok
 DROP TABLE t44466
 
+# ------------------------------------------------------------------------------
+# Regression for #46395.
+# ------------------------------------------------------------------------------
+statement ok
+CREATE TABLE t46395(c0 INT UNIQUE DEFAULT 0, c1 INT);
+
+statement ok
+INSERT INTO t46395(c1) VALUES (0), (1) ON CONFLICT (c0) DO NOTHING;
+
+statement ok
+DROP TABLE t46395
+
 # ------------------------------------------------------------------------------
 # Duplicate primary key inputs.
 # ------------------------------------------------------------------------------
@@ -1181,6 +1193,21 @@ SELECT * FROM target
 6  NULL  NULL
 7  NULL  NULL
 
+statement ok
+INSERT INTO target SELECT 8, y, z FROM (VALUES (2, 2), (2, 3)) s(y, z)
+ON CONFLICT (x) DO NOTHING
+
+query III rowsort
+SELECT * FROM target
+----
+1  1     2
+2  3     3
+4  1     NULL
+5  1     NULL
+6  NULL  NULL
+7  NULL  NULL
+8  2     2
+
 statement ok
 DROP TABLE source
 

pkg/sql/opt/norm/groupby.go

@@ -28,6 +28,12 @@ func (c *CustomFuncs) NullsAreDistinct(distinctOp opt.Operator) bool {
 	return distinctOp == opt.UpsertDistinctOnOp
 }
 
+// RaisesErrorOnDup returns true if an UpsertDistinct operator raises an error
+// when duplicate values are detected.
+func (c *CustomFuncs) RaisesErrorOnDup(private *memo.GroupingPrivate) bool {
+	return private.ErrorOnDup
+}
+
 // RemoveGroupingCols returns a new grouping private struct with the given
 // columns removed from the grouping column set.
 func (c *CustomFuncs) RemoveGroupingCols(

pkg/sql/opt/norm/rules/groupby.opt

@@ -140,19 +140,19 @@
     $groupingPrivate
 )
 
-# EliminateDistinctOnNoColumns eliminates a DistinctOn with no grouping columns,
-# replacing it with a projection and a LIMIT 1. For example:
+# EliminateDistinctNoColumns eliminates a distinct operator with no grouping
+# columns, replacing it with a projection and a LIMIT 1. For example:
 #   SELECT DISTINCT ON (a) a, b FROM ab WHERE a=1
 # is equivalent to:
 #   SELECT a, b FROM ab WHERE a=1 LIMIT 1
 #
-# Note that this rule does not apply to UpsertDistinctOn, since that will raise
-# an error if there are duplicate rows.
-[EliminateDistinctOnNoColumns, Normalize]
-(DistinctOn
+# Note that this rule does not apply to UpsertDistinctOn with ErrorOnDup = true,
+# since that will raise an error if there are duplicate rows.
+[EliminateDistinctNoColumns, Normalize]
+(DistinctOn | UpsertDistinctOn
     $input:*
     $aggregations:*
-    $groupingPrivate:* & (HasNoGroupingCols $groupingPrivate)
+    $groupingPrivate:* & (HasNoGroupingCols $groupingPrivate) & ^(RaisesErrorOnDup $groupingPrivate)
 )
 =>
 (ConstructProjectionFromDistinctOn
@@ -161,16 +161,16 @@
     $aggregations
 )
 
-# EliminateUpsertDistinctOnNoColumns is similar to EliminateDistinctOnNoColumns,
-# except that it will raise an error if there are no grouping columns and the
-# input has more than one row. No grouping columns means there is at most one
-# group. And the Max1Row operator is needed to raise an error if that group has
-# more than one row, which is a requirement of the Upsert operator.
-[EliminateUpsertDistinctOnNoColumns, Normalize]
+# EliminateErrorDistinctNoColumns is similar to EliminateDistinctNoColumns,
+# except that Max1Row will raise an error if there are no grouping columns and
+# the input has more than one row. No grouping columns means there is at most
+# one group. And the Max1Row operator is needed to raise an error if that group
+# has more than one row, which is a requirement of the Upsert operator.
+[EliminateErrorDistinctNoColumns, Normalize]
 (UpsertDistinctOn
     $input:*
     $aggregations:*
-    $groupingPrivate:* & (HasNoGroupingCols $groupingPrivate)
+    $groupingPrivate:* & (HasNoGroupingCols $groupingPrivate) & (RaisesErrorOnDup $groupingPrivate)
 )
 =>
 (ConstructProjectionFromDistinctOn

pkg/sql/opt/norm/testdata/rules/groupby

@@ -1055,10 +1055,10 @@ project
                 └── column10:10
 
 # --------------------------------------------------
-# EliminateDistinctOnNoColumns
+# EliminateDistinctNoColumns
 # --------------------------------------------------
 
-norm expect=EliminateDistinctOnNoColumns
+norm expect=EliminateDistinctNoColumns
 SELECT DISTINCT ON (a) a, b FROM abc WHERE a = 1
 ----
 limit
@@ -1077,7 +1077,7 @@ limit
  │         └── a:1 = 1 [outer=(1), constraints=(/1: [/1 - /1]; tight), fd=()-->(1)]
  └── 1
 
-norm expect=EliminateDistinctOnNoColumns
+norm expect=EliminateDistinctNoColumns
 SELECT DISTINCT ON (b) b, c FROM abc WHERE b = 1 ORDER BY b, c
 ----
 limit
@@ -1100,10 +1100,72 @@ limit
  │              └── b:2 = 1 [outer=(2), constraints=(/2: [/1 - /1]; tight), fd=()-->(2)]
  └── 1
 
+norm expect=EliminateDistinctNoColumns
+INSERT INTO a (k, i, s) SELECT 1, i, 'foo' FROM a WHERE i = 1
+ON CONFLICT (s, i) DO NOTHING
+----
+insert a
+ ├── columns: <none>
+ ├── insert-mapping:
+ │    ├── "?column?":11 => k:1
+ │    ├── i:7 => i:2
+ │    ├── column13:13 => f:3
+ │    ├── "?column?":12 => s:4
+ │    └── column14:14 => j:5
+ ├── cardinality: [0 - 0]
+ ├── side-effects, mutations
+ └── project
+      ├── columns: i:7!null "?column?":11!null "?column?":12!null column13:13 column14:14
+      ├── cardinality: [0 - 1]
+      ├── key: ()
+      ├── fd: ()-->(7,11-14)
+      └── limit
+           ├── columns: i:7!null "?column?":11!null "?column?":12!null column13:13 column14:14 i:16 s:18
+           ├── cardinality: [0 - 1]
+           ├── key: ()
+           ├── fd: ()-->(7,11-14,16,18)
+           ├── select
+           │    ├── columns: i:7!null "?column?":11!null "?column?":12!null column13:13 column14:14 i:16 s:18
+           │    ├── fd: ()-->(7,11-14,18)
+           │    ├── limit hint: 1.00
+           │    ├── left-join (hash)
+           │    │    ├── columns: i:7!null "?column?":11!null "?column?":12!null column13:13 column14:14 i:16 s:18
+           │    │    ├── fd: ()-->(7,11-14)
+           │    │    ├── limit hint: 1.00
+           │    │    ├── project
+           │    │    │    ├── columns: column13:13 column14:14 "?column?":11!null "?column?":12!null i:7!null
+           │    │    │    ├── fd: ()-->(7,11-14)
+           │    │    │    ├── select
+           │    │    │    │    ├── columns: i:7!null
+           │    │    │    │    ├── fd: ()-->(7)
+           │    │    │    │    ├── scan a
+           │    │    │    │    │    └── columns: i:7!null
+           │    │    │    │    └── filters
+           │    │    │    │         └── i:7 = 1 [outer=(7), constraints=(/7: [/1 - /1]; tight), fd=()-->(7)]
+           │    │    │    └── projections
+           │    │    │         ├── CAST(NULL AS FLOAT8) [as=column13:13]
+           │    │    │         ├── CAST(NULL AS JSONB) [as=column14:14]
+           │    │    │         ├── 1 [as="?column?":11]
+           │    │    │         └── 'foo' [as="?column?":12]
+           │    │    ├── select
+           │    │    │    ├── columns: i:16!null s:18!null
+           │    │    │    ├── key: (16)
+           │    │    │    ├── fd: ()-->(18)
+           │    │    │    ├── scan a
+           │    │    │    │    ├── columns: i:16!null s:18!null
+           │    │    │    │    └── key: (16,18)
+           │    │    │    └── filters
+           │    │    │         └── s:18 = 'foo' [outer=(18), constraints=(/18: [/'foo' - /'foo']; tight), fd=()-->(18)]
+           │    │    └── filters
+           │    │         └── i:7 = i:16 [outer=(7,16), constraints=(/7: (/NULL - ]; /16: (/NULL - ]), fd=(7)==(16), (16)==(7)]
+           │    └── filters
+           │         └── s:18 IS NULL [outer=(18), constraints=(/18: [/NULL - /NULL]; tight), fd=()-->(18)]
+           └── 1
+
 # --------------------------------------------------
-# EliminateUpsertDistinctOnNoColumns
+# EliminateErrorDistinctNoColumns
 # --------------------------------------------------
-norm expect=EliminateUpsertDistinctOnNoColumns
+norm expect=EliminateErrorDistinctNoColumns
 INSERT INTO a (k, i, s) SELECT 1, i, 'foo' FROM a WHERE i = 1
 ON CONFLICT (s, i) DO UPDATE SET f=1.1
 ----

pkg/sql/opt/xform/optimizer.go

@@ -914,7 +914,8 @@ func (o *Optimizer) disableRules(probability float64) {
 		// Needed to prevent execbuilder error.
 		// TODO(radu): the DistinctOn execution path should be fixed up so it
 		// supports distinct on an empty column set.
-		int(opt.EliminateDistinctOnNoColumns),
+		int(opt.EliminateDistinctNoColumns),
+		int(opt.EliminateErrorDistinctNoColumns),
 	)
 
 	for i := opt.RuleName(1); i < opt.NumRuleNames; i++ {