Merge cockroachdb#119035 cockroachdb#133270

119035: sql, sem, opt, explain, memo, kv: audit bit-flag-checking helpers r=DrewKimball,mgartner a=michae2

`@mgartner's` [comment](cockroachdb#118931 (review)) on cockroachdb#118931 inspired me to audit all the other helper functions that check whether a flag is set in a bitfield. I might have found a couple bugs.

See individual commits for details.

Epic: None
Release note: None

133270: raft: deflake non-determinisctic raft node tests r=iskettaneh a=iskettaneh

We sporadically see that some raft node_test tests fail due to the leader not being stable. This commit should reduce the chances of that happening by increasing the election timeout to 250ms (instead of 50ms).

I couldn't reproduce the bug locally with this change.

If the bug still happens, we can try to force leadership to make it more deterministic.

Fixes: cockroachdb#132992, cockroachdb#131676, cockroachdb#132205, cockroachdb#133048

Release note: None

Co-authored-by: Michael Erickson <michae2@cockroachlabs.com>
Co-authored-by: Ibrahim Kettaneh <ibrahim.kettaneh@cockroachlabs.com>

pkg/kv/kvpb/batch.go

@@ -405,25 +405,25 @@ func (ba *BatchRequest) IsCompleteTransaction() bool {
 	panic(fmt.Sprintf("unreachable. Batch requests: %s", TruncatedRequestsString(ba.Requests, 1024)))
 }
 
-// hasFlag returns true iff one of the requests within the batch contains the
-// specified flag.
-func (ba *BatchRequest) hasFlag(flag flag) bool {
+// hasFlag returns true iff at least one of the requests within the batch
+// contains at least one of the specified flags.
+func (ba *BatchRequest) hasFlag(flags flag) bool {
 	for _, union := range ba.Requests {
-		if (union.GetInner().flags() & flag) != 0 {
+		if (union.GetInner().flags() & flags) != 0 {
 			return true
 		}
 	}
 	return false
 }
 
-// hasFlagForAll returns true iff all of the requests within the batch contains
-// the specified flag.
-func (ba *BatchRequest) hasFlagForAll(flag flag) bool {
+// hasFlagForAll returns true iff all of the requests within the batch contain
+// all of the specified flags.
+func (ba *BatchRequest) hasFlagForAll(flags flag) bool {
 	if len(ba.Requests) == 0 {
 		return false
 	}
 	for _, union := range ba.Requests {
-		if (union.GetInner().flags() & flag) == 0 {
+		if (union.GetInner().flags() & flags) != flags {
 			return false
 		}
 	}

pkg/kv/kvserver/asim/tests/output.go

@@ -55,9 +55,9 @@ func (o OutputFlags) set(f OutputFlags) OutputFlags {
 	return o | f
 }
 
-// Has returns true if this flag has the given f OutputFlags on.
+// Has returns true if this flag has all of the given f OutputFlags on.
 func (o OutputFlags) Has(f OutputFlags) bool {
-	return o&f != 0
+	return o&f == f
 }
 
 type testResult struct {

pkg/raft/rafttest/node.go

@@ -49,7 +49,7 @@ func startNode(id raftpb.PeerID, peers []raft.Peer, iface iface) *node {
 	st := raft.NewMemoryStorage()
 	c := &raft.Config{
 		ID:                        id,
-		ElectionTick:              10,
+		ElectionTick:              50,
 		HeartbeatTick:             1,
 		Storage:                   st,
 		MaxSizePerMsg:             1024 * 1024,

pkg/sql/event_log.go

@@ -446,8 +446,9 @@ func (*EventLogTestingKnobs) ModuleTestingKnobs() {}
 // event should be directed to.
 type LogEventDestination int
 
+// hasFlag returns true if the receiver has all of the given flags.
 func (d LogEventDestination) hasFlag(f LogEventDestination) bool {
-	return d&f != 0
+	return d&f == f
 }
 
 const (

pkg/sql/explain_plan.go

@@ -107,7 +107,7 @@ func (e *explainPlanNode) startExec(params runParams) error {
 			if e.options.Mode == tree.ExplainDistSQL {
 				flags := execinfrapb.DiagramFlags{
 					ShowInputTypes:    e.options.Flags[tree.ExplainFlagTypes],
-					MakeDeterministic: e.flags.Deflake.Has(explain.DeflakeAll) || params.p.execCfg.TestingKnobs.DeterministicExplain,
+					MakeDeterministic: e.flags.Deflake.HasAny(explain.DeflakeAll) || params.p.execCfg.TestingKnobs.DeterministicExplain,
 				}
 				diagram, err := execinfrapb.GeneratePlanDiagram(params.p.stmt.String(), flows, flags)
 				if err != nil {

pkg/sql/opt/exec/execbuilder/testdata/join

@@ -2219,3 +2219,128 @@ vectorized: true
       missing stats
       table: cards@cards_pkey
       spans: FULL SCAN
+
+# Test that STRAIGHT JOIN is not as strong a hint as LOOKUP JOIN when exploring.
+
+statement ok
+CREATE TABLE t119035 (
+  k INT PRIMARY KEY,
+  a INT,
+  b INT,
+  INDEX (a, b)
+)
+
+query T
+EXPLAIN (OPT, MEMO)
+SELECT * FROM (VALUES (1, 10), (2, 20), (3, 30)) as v(x, y)
+INNER STRAIGHT JOIN t119035 ON a > x
+----
+memo (optimized, ~10KB, required=[presentation: info:10] [distribution: test])
+ ├── G1: (explain G2 [presentation: x:1,y:2,k:3,a:4,b:5] [distribution: test])
+ │    └── [presentation: info:10] [distribution: test]
+ │         ├── best: (explain G2="[presentation: x:1,y:2,k:3,a:4,b:5] [distribution: test]" [presentation: x:1,y:2,k:3,a:4,b:5] [distribution: test])
+ │         └── cost: 1196.99
+ ├── G2: (inner-join G3 G4 G5)
+ │    ├── [presentation: x:1,y:2,k:3,a:4,b:5] [distribution: test]
+ │    │    ├── best: (inner-join G3="[distribution: test]" G4="[distribution: test]" G5)
+ │    │    └── cost: 1196.97
+ │    └── []
+ │         ├── best: (inner-join G3 G4 G5)
+ │         └── cost: 1196.97
+ ├── G3: (values G6 id=v1)
+ │    ├── [distribution: test]
+ │    │    ├── best: (values G6 id=v1)
+ │    │    └── cost: 0.04
+ │    └── []
+ │         ├── best: (values G6 id=v1)
+ │         └── cost: 0.04
+ ├── G4: (scan t119035,cols=(3-5)) (scan t119035@t119035_a_b_idx,cols=(3-5))
+ │    ├── [distribution: test]
+ │    │    ├── best: (scan t119035,cols=(3-5))
+ │    │    └── cost: 1149.22
+ │    └── []
+ │         ├── best: (scan t119035,cols=(3-5))
+ │         └── cost: 1149.22
+ ├── G5: (filters G7)
+ ├── G6: (scalar-list G8 G9 G10)
+ ├── G7: (gt G11 G12)
+ ├── G8: (tuple G13)
+ ├── G9: (tuple G14)
+ ├── G10: (tuple G15)
+ ├── G11: (variable a)
+ ├── G12: (variable column1)
+ ├── G13: (scalar-list G16 G17)
+ ├── G14: (scalar-list G18 G19)
+ ├── G15: (scalar-list G20 G21)
+ ├── G16: (const 1)
+ ├── G17: (const 10)
+ ├── G18: (const 2)
+ ├── G19: (const 20)
+ ├── G20: (const 3)
+ └── G21: (const 30)
+inner-join (cross)
+ ├── flags: disallow hash join (store left side) and lookup join (into left side) and inverted join (into left side)
+ ├── values
+ │    ├── (1, 10)
+ │    ├── (2, 20)
+ │    └── (3, 30)
+ ├── scan t119035
+ └── filters
+      └── a > column1
+
+query T
+EXPLAIN (OPT, MEMO)
+SELECT * FROM (VALUES (1, 10), (2, 20), (3, 30)) as v(x, y)
+INNER LOOKUP JOIN t119035 ON a > x
+----
+memo (optimized, ~11KB, required=[presentation: info:10] [distribution: test])
+ ├── G1: (explain G2 [presentation: x:1,y:2,k:3,a:4,b:5] [distribution: test])
+ │    └── [presentation: info:10] [distribution: test]
+ │         ├── best: (explain G2="[presentation: x:1,y:2,k:3,a:4,b:5] [distribution: test]" [presentation: x:1,y:2,k:3,a:4,b:5] [distribution: test])
+ │         └── cost: 2172.81
+ ├── G2: (inner-join G3 G4 G5) (lookup-join G3 G6 t119035@t119035_a_b_idx,outCols=(1-5))
+ │    ├── [presentation: x:1,y:2,k:3,a:4,b:5] [distribution: test]
+ │    │    ├── best: (lookup-join G3="[distribution: test]" G6 t119035@t119035_a_b_idx,outCols=(1-5))
+ │    │    └── cost: 2172.79
+ │    └── []
+ │         ├── best: (lookup-join G3 G6 t119035@t119035_a_b_idx,outCols=(1-5))
+ │         └── cost: 2172.79
+ ├── G3: (values G7 id=v1)
+ │    ├── [distribution: test]
+ │    │    ├── best: (values G7 id=v1)
+ │    │    └── cost: 0.04
+ │    └── []
+ │         ├── best: (values G7 id=v1)
+ │         └── cost: 0.04
+ ├── G4: (scan t119035,cols=(3-5)) (scan t119035@t119035_a_b_idx,cols=(3-5))
+ │    ├── [distribution: test]
+ │    │    ├── best: (scan t119035,cols=(3-5))
+ │    │    └── cost: 1149.22
+ │    └── []
+ │         ├── best: (scan t119035,cols=(3-5))
+ │         └── cost: 1149.22
+ ├── G5: (filters G8)
+ ├── G6: (filters)
+ ├── G7: (scalar-list G9 G10 G11)
+ ├── G8: (gt G12 G13)
+ ├── G9: (tuple G14)
+ ├── G10: (tuple G15)
+ ├── G11: (tuple G16)
+ ├── G12: (variable a)
+ ├── G13: (variable column1)
+ ├── G14: (scalar-list G17 G18)
+ ├── G15: (scalar-list G19 G20)
+ ├── G16: (scalar-list G21 G22)
+ ├── G17: (const 1)
+ ├── G18: (const 10)
+ ├── G19: (const 2)
+ ├── G20: (const 20)
+ ├── G21: (const 3)
+ └── G22: (const 30)
+inner-join (lookup t119035@t119035_a_b_idx)
+ ├── flags: force lookup join (into right side)
+ ├── values
+ │    ├── (1, 10)
+ │    ├── (2, 20)
+ │    └── (3, 30)
+ └── filters (true)

pkg/sql/opt/exec/explain/emit.go

@@ -569,7 +569,7 @@ func (e *emitter) emitNodeAttributes(n *Node) error {
 					}
 
 					var duration string
-					if e.ob.flags.Deflake.Has(DeflakeVolatile) {
+					if e.ob.flags.Deflake.HasAny(DeflakeVolatile) {
 						duration = "<hidden>"
 					} else {
 						timeSinceStats := timeutil.Since(s.TableStatsCreatedAt)
@@ -581,7 +581,7 @@ func (e *emitter) emitNodeAttributes(n *Node) error {
 
 					var forecastStr string
 					if s.Forecast {
-						if e.ob.flags.Deflake.Has(DeflakeVolatile) {
+						if e.ob.flags.Deflake.HasAny(DeflakeVolatile) {
 							forecastStr = "; using stats forecast"
 						} else {
 							timeSinceStats := timeutil.Since(s.ForecastAt)

pkg/sql/opt/exec/explain/flags.go

@@ -57,9 +57,9 @@ const (
 	DeflakeAll DeflakeFlags = DeflakeDistribution | DeflakeVectorized | DeflakeNodes | DeflakeVolatile
 )
 
-// Has returns true if the receiver has the given deflake flag set.
-func (f DeflakeFlags) Has(flag DeflakeFlags) bool {
-	return (f & flag) != 0
+// HasAny returns true if the receiver has any of the given deflake flags set.
+func (f DeflakeFlags) HasAny(flags DeflakeFlags) bool {
+	return (f & flags) != 0
 }
 
 // MakeFlags crates Flags from ExplainOptions.

pkg/sql/opt/exec/explain/output.go

@@ -110,7 +110,7 @@ func (ob *OutputBuilder) AddField(key, value string) {
 // AddFlakyField adds an information field under the current node, hiding the
 // value depending on the given deflake flags.
 func (ob *OutputBuilder) AddFlakyField(flags DeflakeFlags, key, value string) {
-	if ob.flags.Deflake.Has(flags) {
+	if ob.flags.Deflake.HasAny(flags) {
 		value = "<hidden>"
 	}
 	ob.AddField(key, value)
@@ -285,7 +285,7 @@ func (ob *OutputBuilder) AddTopLevelField(key, value string) {
 // AddFlakyTopLevelField adds a top-level field, hiding the value depending on
 // the given deflake flags.
 func (ob *OutputBuilder) AddFlakyTopLevelField(flags DeflakeFlags, key, value string) {
-	if ob.flags.Deflake.Has(flags) {
+	if ob.flags.Deflake.HasAny(flags) {
 		value = "<hidden>"
 	}
 	ob.AddTopLevelField(key, value)
@@ -319,7 +319,7 @@ func (ob *OutputBuilder) AddPlanType(generic, optimized bool) {
 // AddPlanningTime adds a top-level planning time field. Cannot be called
 // while inside a node.
 func (ob *OutputBuilder) AddPlanningTime(delta time.Duration) {
-	if ob.flags.Deflake.Has(DeflakeVolatile) {
+	if ob.flags.Deflake.HasAny(DeflakeVolatile) {
 		delta = 10 * time.Microsecond
 	}
 	ob.AddTopLevelField("planning time", string(humanizeutil.Duration(delta)))
@@ -328,7 +328,7 @@ func (ob *OutputBuilder) AddPlanningTime(delta time.Duration) {
 // AddExecutionTime adds a top-level execution time field. Cannot be called
 // while inside a node.
 func (ob *OutputBuilder) AddExecutionTime(delta time.Duration) {
-	if ob.flags.Deflake.Has(DeflakeVolatile) {
+	if ob.flags.Deflake.HasAny(DeflakeVolatile) {
 		delta = 100 * time.Microsecond
 	}
 	ob.AddTopLevelField("execution time", string(humanizeutil.Duration(delta)))
@@ -337,7 +337,7 @@ func (ob *OutputBuilder) AddExecutionTime(delta time.Duration) {
 // AddClientTime adds a top-level client-level protocol time field. Cannot be
 // called while inside a node.
 func (ob *OutputBuilder) AddClientTime(delta time.Duration) {
-	if ob.flags.Deflake.Has(DeflakeVolatile) {
+	if ob.flags.Deflake.HasAny(DeflakeVolatile) {
 		delta = time.Microsecond
 	}
 	ob.AddTopLevelField("client time", string(humanizeutil.Duration(delta)))
@@ -396,7 +396,7 @@ func (ob *OutputBuilder) AddNetworkStats(messages, bytes int64) {
 // information entirely if we're redacting. Since disk spilling is rare we only
 // include this field is bytes is greater than zero.
 func (ob *OutputBuilder) AddMaxDiskUsage(bytes int64) {
-	if !ob.flags.Deflake.Has(DeflakeVolatile) && bytes > 0 {
+	if !ob.flags.Deflake.HasAny(DeflakeVolatile) && bytes > 0 {
 		ob.AddTopLevelField("max sql temp disk usage",
 			string(humanizeutil.IBytes(bytes)))
 	}
@@ -407,7 +407,7 @@ func (ob *OutputBuilder) AddMaxDiskUsage(bytes int64) {
 // independent of platform because the grunning library isn't currently
 // supported on all platforms.
 func (ob *OutputBuilder) AddCPUTime(cpuTime time.Duration) {
-	if !ob.flags.Deflake.Has(DeflakeVolatile) {
+	if !ob.flags.Deflake.HasAny(DeflakeVolatile) {
 		ob.AddTopLevelField("sql cpu time", string(humanizeutil.Duration(cpuTime)))
 	}
 }

pkg/sql/opt/exec/factory.go

@@ -76,16 +76,19 @@ const (
 	PlanFlagCheckContainsLocking
 )
 
-func (pf PlanFlags) IsSet(flag PlanFlags) bool {
-	return (pf & flag) != 0
+// IsSet returns true if the receiver has all of the given flags set.
+func (pf PlanFlags) IsSet(flags PlanFlags) bool {
+	return (pf & flags) == flags
 }
 
-func (pf *PlanFlags) Set(flag PlanFlags) {
-	*pf |= flag
+// Set sets all of the given flags in the receiver.
+func (pf *PlanFlags) Set(flags PlanFlags) {
+	*pf |= flags
 }
 
-func (pf *PlanFlags) Unset(flag PlanFlags) {
-	*pf &^= flag
+// Unset unsets all of the given flags in the receiver.
+func (pf *PlanFlags) Unset(flags PlanFlags) {
+	*pf &^= flags
 }
 
 // ScanParams contains all the parameters for a table scan.

pkg/sql/opt/memo/expr.go

@@ -542,9 +542,9 @@ func (jf JoinFlags) Empty() bool {
 	return jf == 0
 }
 
-// Has returns true if the given flag is set.
-func (jf JoinFlags) Has(flag JoinFlags) bool {
-	return jf&flag != 0
+// Has returns true if all of the given flags are set in the receiver.
+func (jf JoinFlags) Has(flags JoinFlags) bool {
+	return jf&flags == flags
 }
 
 func (jf JoinFlags) String() string {

pkg/sql/opt/props/logical.go

@@ -360,26 +360,26 @@ func (r *Relational) Statistics() *Statistics {
 	return &r.stats
 }
 
-// IsAvailable returns true if the specified rule property has been populated
-// on this relational properties instance.
-func (r *Relational) IsAvailable(p AvailableRuleProps) bool {
-	return (r.Rule.Available & p) != 0
+// IsAvailable returns true if all of the specified rule properties have been
+// populated on this relational properties instance.
+func (r *Relational) IsAvailable(ps AvailableRuleProps) bool {
+	return (r.Rule.Available & ps) == ps
 }
 
-// SetAvailable sets the available bits for the given properties, in order to
-// mark them as populated on this relational properties instance.
-func (r *Relational) SetAvailable(p AvailableRuleProps) {
-	r.Rule.Available |= p
+// SetAvailable sets the available bits for all of the given properties, in
+// order to mark them as populated on this relational properties instance.
+func (r *Relational) SetAvailable(ps AvailableRuleProps) {
+	r.Rule.Available |= ps
 }
 
-// IsAvailable returns true if the specified rule property has been populated
-// on this scalar properties instance.
-func (s *Scalar) IsAvailable(p AvailableRuleProps) bool {
-	return (s.Rule.Available & p) != 0
+// IsAvailable returns true if all of the specified rule property have been
+// populated on this scalar properties instance.
+func (s *Scalar) IsAvailable(ps AvailableRuleProps) bool {
+	return (s.Rule.Available & ps) == ps
 }
 
-// SetAvailable sets the available bits for the given properties, in order to
-// mark them as populated on this scalar properties instance.
-func (s *Scalar) SetAvailable(p AvailableRuleProps) {
-	s.Rule.Available |= p
+// SetAvailable sets the available bits for all of the given properties, in
+// order to mark them as populated on this scalar properties instance.
+func (s *Scalar) SetAvailable(ps AvailableRuleProps) {
+	s.Rule.Available |= ps
 }