Skip to content

Commit

Permalink
Merge #98517 #98580 #98688 #98700
Browse files Browse the repository at this point in the history 
98517: multitenant: add AdminMerge capability r=knz a=ecwall

Fixes #95138

AdminMerge is currently only called by the system tenant even though it is
named similarly to other Admin* functions so it does not need its own
capability for now.

This changes its required capability from noCapCheckNeeded to onlySystemTenant
to prevent secondary tenants from calling it.

Release note: None

98580: rpc: bump threshold for latency jump reporting r=erikgrinaker a=tbg

For months I've seen this misfire in nearly every single log line I've
looked at, and I've had to grep it out in many L2 incidents.
Maybe it works better when we suppress it for latencies <=50ms.

Touches #96262.
Fixes #98066.

Epic: none
Release note: None


98688: colexecbase: fix a recently introduced bug with identity cast r=yuzefovich a=yuzefovich

This commit fixes a recently introduced bug that can occur when we're randomizing `coldata.BatchSize()` (which we do in tests). In particular, we capped a global singleton at one batch size value, but later we can change it to a higher value, which could lead to index out of bounds. This is now fixed by always using the max batch size.

Fixes: #98660.

Release note: None

98700: opt: fix hoist of ANY comparison with tuples r=mgartner a=mgartner

#### opt: fix hoist of ANY comparison with tuples

Prior to this commit, when hoisting Any expressions like
`<left> = ANY (SELECT <right> ...)`, we constructed
`(IsNot <left|right> Null)` expressions which are equivalent to
`<left|right> IS DISTINCT FROM NULL`. As discovered in #46675, these
expressions have different behavior than `<left> IS NOT NULL` when
`<left>` is a tuple. As a result, the hoisting transformations could
construct invalid plans that cause incorrect results. This commit fixes
the issue by using `IsTupleNotNull` expressions when `<left>` and
`<right> are tupleq.

Fixes #98691

Release note (bug fix): A bug has been fixes that caused incorrect
results of ANY comparisons of tuples. For example, an expression like
`(x, y) = ANY (SELECT a, b FROM t WHERE ...)` could return `true`
instead of the correct result of `NULL` when `x` and `y` were `NULL`, or
`a` and `b` were `NULL`. This could only occur if the subquery was
correlated, i.e., it references columns from the outer part of the
query. This bug was present since the cost-based optimizer was
introduced in version 2.1.


Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
  • Loading branch information
@ecwall @tbg
@yuzefovich @mgartner
5 people committed Mar 15, 2023
5 parents 77a55f9 + 0619746 + 3fed3d4 + e8fd9a9 + a48ec29 commit 3016744
Show file tree
Hide file tree
Showing 8 changed files with 287 additions and 80 deletions.
26 changes: 12 additions & 14 deletions pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,8 @@ var reqMethodToCap = map[kvpb.Method]tenantcapabilities.CapabilityID{
kvpb.Barrier: noCapCheckNeeded,
kvpb.ClearRange: noCapCheckNeeded,
kvpb.ConditionalPut: noCapCheckNeeded,
kvpb.DeleteRange: noCapCheckNeeded,
kvpb.Delete: noCapCheckNeeded,
kvpb.DeleteRange: noCapCheckNeeded,
kvpb.EndTxn: noCapCheckNeeded,
kvpb.Export: noCapCheckNeeded,
kvpb.Get: noCapCheckNeeded,
Expand All @@ -147,8 +147,8 @@ var reqMethodToCap = map[kvpb.Method]tenantcapabilities.CapabilityID{
kvpb.RecoverTxn: noCapCheckNeeded,
kvpb.Refresh: noCapCheckNeeded,
kvpb.RefreshRange: noCapCheckNeeded,
kvpb.ResolveIntentRange: noCapCheckNeeded,
kvpb.ResolveIntent: noCapCheckNeeded,
kvpb.ResolveIntentRange: noCapCheckNeeded,
kvpb.ReverseScan: noCapCheckNeeded,
kvpb.RevertRange: noCapCheckNeeded,
kvpb.Scan: noCapCheckNeeded,
Expand All @@ -161,24 +161,22 @@ var reqMethodToCap = map[kvpb.Method]tenantcapabilities.CapabilityID{
kvpb.AdminRelocateRange: tenantcapabilities.CanAdminRelocateRange,
kvpb.AdminTransferLease: tenantcapabilities.CanAdminRelocateRange,

// TODO(ecwall): The following should also be authorized via specific capabilities.
kvpb.AdminMerge: noCapCheckNeeded,

// TODO(knz,arul): Verify with the relevant teams whether secondary
// tenants have legitimate access to any of those.
kvpb.TruncateLog: onlySystemTenant,
kvpb.AdminMerge: onlySystemTenant,
kvpb.AdminVerifyProtectedTimestamp: onlySystemTenant,
kvpb.CheckConsistency: onlySystemTenant,
kvpb.ComputeChecksum: onlySystemTenant,
kvpb.GC: onlySystemTenant,
kvpb.Merge: onlySystemTenant,
kvpb.RequestLease: onlySystemTenant,
kvpb.TransferLease: onlySystemTenant,
kvpb.Migrate: onlySystemTenant,
kvpb.Probe: onlySystemTenant,
kvpb.QueryResolvedTimestamp: onlySystemTenant,
kvpb.RecomputeStats: onlySystemTenant,
kvpb.ComputeChecksum: onlySystemTenant,
kvpb.CheckConsistency: onlySystemTenant,
kvpb.AdminVerifyProtectedTimestamp: onlySystemTenant,
kvpb.Migrate: onlySystemTenant,
kvpb.RequestLease: onlySystemTenant,
kvpb.Subsume: onlySystemTenant,
kvpb.QueryResolvedTimestamp: onlySystemTenant,
kvpb.GC: onlySystemTenant,
kvpb.TransferLease: onlySystemTenant,
kvpb.TruncateLog: onlySystemTenant,
}

const (
Expand Down
7 changes: 5 additions & 2 deletions pkg/rpc/clock_offset.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,11 +264,14 @@ func (r *RemoteClockMonitor) UpdateOffset(
info.avgNanos.Add(newLatencyf)
r.metrics.LatencyHistogramNanos.RecordValue(roundTripLatency.Nanoseconds())

// See: https://github.com/cockroachdb/cockroach/issues/96262
// See: https://github.com/cockroachdb/cockroach/issues/98066
const thresh = 50 * 1e6 // 50ms
// If the roundtrip jumps by 50% beyond the previously recorded average, report it in logs.
// Don't report it again until it falls below 40% above the average.
// (Also requires latency > 1ms to avoid trigger on noise on low-latency connections and
// (Also requires latency > thresh to avoid trigger on noise on low-latency connections and
// the running average to be non-zero to avoid triggering on startup.)
if newLatencyf > 1e6 && prevAvg > 0.0 &&
if newLatencyf > thresh && prevAvg > 0.0 &&
info.trigger.triggers(newLatencyf, prevAvg*1.4, prevAvg*1.5) {
log.Health.Warningf(ctx, "latency jump (prev avg %.2fms, current %.2fms)",
prevAvg/1e6, newLatencyf/1e6)
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/colexec/colexecbase/cast.eg.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/sql/colexec/colexecbase/cast_tmpl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ var _ colexecop.ClosableOperator = &castIdentityOp{}
var identityOrder []int

func init() {
identityOrder = make([]int, coldata.BatchSize())
identityOrder = make([]int, coldata.MaxBatchSize)
for i := range identityOrder {
identityOrder[i] = i
}
Expand Down
29 changes: 29 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/subquery_correlated
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1303,3 +1303,32 @@ WHERE key IN (
3 2
4 1
4 3

# Regression test for #98691.
statement ok
CREATE TABLE t98691 (
a INT,
b INT
)

statement ok
INSERT INTO t98691 VALUES (1, 10)

query B
SELECT (NULL, NULL) = ANY (
SELECT a, b FROM t98691 WHERE a > i
) FROM (VALUES (0), (0)) v(i)
----
NULL
NULL

statement ok
INSERT INTO t98691 VALUES (NULL, NULL)

query B
SELECT (2, 20) = ANY (
SELECT a, b FROM t98691 WHERE a > i OR a IS NULL
) FROM (VALUES (0), (0)) v(i)
----
NULL
NULL
18 changes: 16 additions & 2 deletions pkg/sql/opt/norm/decorrelate_funcs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1009,6 +1009,20 @@ func (r *subqueryHoister) constructGroupByAny(
aggVar := r.f.ConstructVariable(aggColID)
caseColID := r.f.Metadata().AddColumn("case", types.Bool)

var scalarNotNull opt.ScalarExpr
if scalar.DataType().Family() == types.TupleFamily {
scalarNotNull = r.f.ConstructIsTupleNotNull(scalar)
} else {
scalarNotNull = r.f.ConstructIsNot(scalar, memo.NullSingleton)
}

var inputNotNull opt.ScalarExpr
if inputVar.DataType().Family() == types.TupleFamily {
inputNotNull = r.f.ConstructIsTupleNotNull(inputVar)
} else {
inputNotNull = r.f.ConstructIsNot(inputVar, memo.NullSingleton)
}

return r.f.ConstructProject(
r.f.ConstructScalarGroupBy(
r.f.ConstructProject(
Expand All @@ -1022,7 +1036,7 @@ func (r *subqueryHoister) constructGroupByAny(
)},
),
memo.ProjectionsExpr{r.f.ConstructProjectionsItem(
r.f.ConstructIsNot(inputVar, memo.NullSingleton),
inputNotNull,
notNullColID,
)},
opt.ColSet{},
Expand All @@ -1042,7 +1056,7 @@ func (r *subqueryHoister) constructGroupByAny(
r.f.ConstructWhen(
r.f.ConstructAnd(
aggVar,
r.f.ConstructIsNot(scalar, memo.NullSingleton),
scalarNotNull,
),
r.f.ConstructTrue(),
),
Expand Down
133 changes: 73 additions & 60 deletions pkg/sql/opt/norm/testdata/rules/combo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1252,6 +1252,74 @@ HoistProjectSubquery
- └── 5
+ └── case:15 [as=r:12, outer=(15)]
================================================================================
FoldNonNullIsNotNull
Cost: 2209.75
================================================================================
project
├── columns: r:12
├── inner-join-apply
│ ├── columns: x:1!null case:15
│ ├── key: (1)
│ ├── fd: (1)-->(15)
│ ├── scan xy
│ │ ├── columns: x:1!null
│ │ └── key: (1)
│ ├── project
│ │ ├── columns: case:15
│ │ ├── outer: (1)
│ │ ├── cardinality: [1 - 1]
│ │ ├── key: ()
│ │ ├── fd: ()-->(15)
│ │ ├── scalar-group-by
│ │ │ ├── columns: bool_or:14
│ │ │ ├── outer: (1)
│ │ │ ├── cardinality: [1 - 1]
│ │ │ ├── key: ()
│ │ │ ├── fd: ()-->(14)
│ │ │ ├── project
│ │ │ │ ├── columns: notnull:13!null
│ │ │ │ ├── outer: (1)
│ │ │ │ ├── cardinality: [0 - 1]
│ │ │ │ ├── key: ()
│ │ │ │ ├── fd: ()-->(13)
│ │ │ │ ├── select
│ │ │ │ │ ├── columns: i:6
│ │ │ │ │ ├── outer: (1)
│ │ │ │ │ ├── cardinality: [0 - 1]
│ │ │ │ │ ├── key: ()
│ │ │ │ │ ├── fd: ()-->(6)
│ │ │ │ │ ├── project
│ │ │ │ │ │ ├── columns: i:6
│ │ │ │ │ │ ├── outer: (1)
│ │ │ │ │ │ ├── cardinality: [0 - 1]
│ │ │ │ │ │ ├── key: ()
│ │ │ │ │ │ ├── fd: ()-->(6)
│ │ │ │ │ │ └── select
│ │ │ │ │ │ ├── columns: k:5!null i:6
│ │ │ │ │ │ ├── outer: (1)
│ │ │ │ │ │ ├── cardinality: [0 - 1]
│ │ │ │ │ │ ├── key: ()
│ │ │ │ │ │ ├── fd: ()-->(5,6)
│ │ │ │ │ │ ├── scan a
│ │ │ │ │ │ │ ├── columns: k:5!null i:6
│ │ │ │ │ │ │ ├── key: (5)
│ │ │ │ │ │ │ └── fd: (5)-->(6)
│ │ │ │ │ │ └── filters
│ │ │ │ │ │ └── k:5 = x:1 [outer=(1,5), constraints=(/1: (/NULL - ]; /5: (/NULL - ]), fd=(1)==(5), (5)==(1)]
│ │ │ │ │ └── filters
│ │ │ │ │ └── (5 = i:6) IS NOT false [outer=(6)]
│ │ │ │ └── projections
│ │ │ │ └── i:6 IS NOT NULL [as=notnull:13, outer=(6)]
│ │ │ └── aggregations
│ │ │ └── bool-or [as=bool_or:14, outer=(13)]
│ │ │ └── notnull:13
│ │ └── projections
- │ │ └── CASE WHEN bool_or:14 AND (5 IS NOT NULL) THEN true WHEN bool_or:14 IS NULL THEN false ELSE CAST(NULL AS BOOL) END [as=case:15, outer=(14)]
+ │ │ └── CASE WHEN bool_or:14 AND true THEN true WHEN bool_or:14 IS NULL THEN false ELSE CAST(NULL AS BOOL) END [as=case:15, outer=(14)]
│ └── filters (true)
└── projections
└── case:15 [as=r:12, outer=(15)]
================================================================================
CommuteVar
Cost: 2209.75
================================================================================
Expand Down Expand Up @@ -1315,7 +1383,7 @@ CommuteVar
│ │ │ └── bool-or [as=bool_or:14, outer=(13)]
│ │ │ └── notnull:13
│ │ └── projections
│ │ └── CASE WHEN bool_or:14 AND (5 IS NOT NULL) THEN true WHEN bool_or:14 IS NULL THEN false ELSE CAST(NULL AS BOOL) END [as=case:15, outer=(14)]
│ │ └── CASE WHEN bool_or:14 AND true THEN true WHEN bool_or:14 IS NULL THEN false ELSE CAST(NULL AS BOOL) END [as=case:15, outer=(14)]
│ └── filters (true)
└── projections
└── case:15 [as=r:12, outer=(15)]
Expand Down Expand Up @@ -1395,7 +1463,7 @@ PushSelectIntoProject
│ │ │ └── bool-or [as=bool_or:14, outer=(13)]
│ │ │ └── notnull:13
│ │ └── projections
│ │ └── CASE WHEN bool_or:14 AND (5 IS NOT NULL) THEN true WHEN bool_or:14 IS NULL THEN false ELSE CAST(NULL AS BOOL) END [as=case:15, outer=(14)]
│ │ └── CASE WHEN bool_or:14 AND true THEN true WHEN bool_or:14 IS NULL THEN false ELSE CAST(NULL AS BOOL) END [as=case:15, outer=(14)]
│ └── filters (true)
└── projections
└── case:15 [as=r:12, outer=(15)]
Expand Down Expand Up @@ -1473,7 +1541,7 @@ MergeSelects
│ │ │ └── bool-or [as=bool_or:14, outer=(13)]
│ │ │ └── notnull:13
│ │ └── projections
│ │ └── CASE WHEN bool_or:14 AND (5 IS NOT NULL) THEN true WHEN bool_or:14 IS NULL THEN false ELSE CAST(NULL AS BOOL) END [as=case:15, outer=(14)]
│ │ └── CASE WHEN bool_or:14 AND true THEN true WHEN bool_or:14 IS NULL THEN false ELSE CAST(NULL AS BOOL) END [as=case:15, outer=(14)]
│ └── filters (true)
└── projections
└── case:15 [as=r:12, outer=(15)]
Expand Down Expand Up @@ -1554,7 +1622,7 @@ EliminateSelect
│ │ │ └── bool-or [as=bool_or:14, outer=(13)]
│ │ │ └── notnull:13
│ │ └── projections
│ │ └── CASE WHEN bool_or:14 AND (5 IS NOT NULL) THEN true WHEN bool_or:14 IS NULL THEN false ELSE CAST(NULL AS BOOL) END [as=case:15, outer=(14)]
│ │ └── CASE WHEN bool_or:14 AND true THEN true WHEN bool_or:14 IS NULL THEN false ELSE CAST(NULL AS BOOL) END [as=case:15, outer=(14)]
│ └── filters (true)
└── projections
└── case:15 [as=r:12, outer=(15)]
Expand Down Expand Up @@ -1624,62 +1692,7 @@ MergeProjects
│ │ │ └── bool-or [as=bool_or:14, outer=(13)]
│ │ │ └── notnull:13
│ │ └── projections
│ │ └── CASE WHEN bool_or:14 AND (5 IS NOT NULL) THEN true WHEN bool_or:14 IS NULL THEN false ELSE CAST(NULL AS BOOL) END [as=case:15, outer=(14)]
│ └── filters (true)
└── projections
└── case:15 [as=r:12, outer=(15)]
================================================================================
FoldNonNullIsNotNull
Cost: 2209.73
================================================================================
project
├── columns: r:12
├── inner-join-apply
│ ├── columns: x:1!null case:15
│ ├── key: (1)
│ ├── fd: (1)-->(15)
│ ├── scan xy
│ │ ├── columns: x:1!null
│ │ └── key: (1)
│ ├── project
│ │ ├── columns: case:15
│ │ ├── outer: (1)
│ │ ├── cardinality: [1 - 1]
│ │ ├── key: ()
│ │ ├── fd: ()-->(15)
│ │ ├── scalar-group-by
│ │ │ ├── columns: bool_or:14
│ │ │ ├── outer: (1)
│ │ │ ├── cardinality: [1 - 1]
│ │ │ ├── key: ()
│ │ │ ├── fd: ()-->(14)
│ │ │ ├── project
│ │ │ │ ├── columns: notnull:13!null
│ │ │ │ ├── outer: (1)
│ │ │ │ ├── cardinality: [0 - 1]
│ │ │ │ ├── key: ()
│ │ │ │ ├── fd: ()-->(13)
│ │ │ │ ├── select
│ │ │ │ │ ├── columns: k:5!null i:6
│ │ │ │ │ ├── outer: (1)
│ │ │ │ │ ├── cardinality: [0 - 1]
│ │ │ │ │ ├── key: ()
│ │ │ │ │ ├── fd: ()-->(5,6)
│ │ │ │ │ ├── scan a
│ │ │ │ │ │ ├── columns: k:5!null i:6
│ │ │ │ │ │ ├── key: (5)
│ │ │ │ │ │ └── fd: (5)-->(6)
│ │ │ │ │ └── filters
│ │ │ │ │ ├── k:5 = x:1 [outer=(1,5), constraints=(/1: (/NULL - ]; /5: (/NULL - ]), fd=(1)==(5), (5)==(1)]
│ │ │ │ │ └── (i:6 = 5) IS NOT false [outer=(6)]
│ │ │ │ └── projections
│ │ │ │ └── i:6 IS NOT NULL [as=notnull:13, outer=(6)]
│ │ │ └── aggregations
│ │ │ └── bool-or [as=bool_or:14, outer=(13)]
│ │ │ └── notnull:13
│ │ └── projections
- │ │ └── CASE WHEN bool_or:14 AND (5 IS NOT NULL) THEN true WHEN bool_or:14 IS NULL THEN false ELSE CAST(NULL AS BOOL) END [as=case:15, outer=(14)]
+ │ │ └── CASE WHEN bool_or:14 AND true THEN true WHEN bool_or:14 IS NULL THEN false ELSE CAST(NULL AS BOOL) END [as=case:15, outer=(14)]
│ │ └── CASE WHEN bool_or:14 AND true THEN true WHEN bool_or:14 IS NULL THEN false ELSE CAST(NULL AS BOOL) END [as=case:15, outer=(14)]
│ └── filters (true)
└── projections
└── case:15 [as=r:12, outer=(15)]
Expand Down
Loading

0 comments on commit 3016744

Please sign in to comment.