Skip to content

Commit

Permalink
Merge #76473 #77052
Browse files Browse the repository at this point in the history 
76473: kvserver: activate a `StoreRebalancer` log message without verbosity levels set r=aayushshah15 a=aayushshah15

The `rebalanceStore()` goroutine is only called roughly once a minute, so
there's no good reason for this message to be only activated behind a verbosity
level.

Release note: None


77052: opt: fetch virtual columns during cascading delete r=mgartner a=mgartner

#### opt: fetch virtual columns during cascading delete

Previously, virtual columns were not always fetched from child tables in
FK relationships during cascading deletes. This prevented the execution
engine from correctly removing KV entries in indexes on virtual columns,
causing index corruption. Virtual columns are now always fetched from
child tables during cascading deletes.

Fixes #76852

Release note (bug fix): A bug has been fixed that could corrupt indexes
containing virtual columns or expressions. The bug only occured when
the index's table had a foreign key reference to another table with an
`ON DELETE CASCADE` action, and a row was deleted in the referenced
table. This bug was present since virtual columns were added in version
21.1.0.

#### opt: remove includeVirtualComputed field from columnKinds

The `columnKinds.includeVirtualComputed` column field is always `true`,
so there is no need for it. It has been removed.

Release note: None


Co-authored-by: Aayush Shah <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
  • Loading branch information
@aayushshah15 @mgartner
3 people committed Feb 25, 2022
3 parents 6a24990 + 3d3a45e + 1b0ebc1 commit 4a8d2f5
Show file tree
Hide file tree
Showing 9 changed files with 109 additions and 85 deletions.
12 changes: 5 additions & 7 deletions pkg/kv/kvserver/store_rebalancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,12 +253,7 @@ func (sr *StoreRebalancer) scorerOptions() *qpsScorerOptions {
func (sr *StoreRebalancer) rebalanceStore(
ctx context.Context, mode LBRebalancingMode, allStoresList StoreList,
) {
// First check if we should transfer leases away to better balance QPS.
options := sr.scorerOptions()
// We only bother rebalancing stores that are fielding more than the
// cluster-level overfull threshold of QPS.
qpsMaxThreshold := overfullQPSThreshold(options, allStoresList.candidateQueriesPerSecond.mean)

var localDesc *roachpb.StoreDescriptor
for i := range allStoresList.stores {
if allStoresList.stores[i].StoreID == sr.rq.store.StoreID() {
Expand All @@ -271,19 +266,22 @@ func (sr *StoreRebalancer) rebalanceStore(
return
}

// We only bother rebalancing stores that are fielding more than the
// cluster-level overfull threshold of QPS.
qpsMaxThreshold := overfullQPSThreshold(options, allStoresList.candidateQueriesPerSecond.mean)
if !(localDesc.Capacity.QueriesPerSecond > qpsMaxThreshold) {
log.VEventf(ctx, 1, "local QPS %.2f is below max threshold %.2f (mean=%.2f); no rebalancing needed",
log.Infof(ctx, "local QPS %.2f is below max threshold %.2f (mean=%.2f); no rebalancing needed",
localDesc.Capacity.QueriesPerSecond, qpsMaxThreshold, allStoresList.candidateQueriesPerSecond.mean)
return
}

var replicasToMaybeRebalance []replicaWithStats
storeMap := storeListToMap(allStoresList)

// First check if we should transfer leases away to better balance QPS.
log.Infof(ctx,
"considering load-based lease transfers for s%d with %.2f qps (mean=%.2f, upperThreshold=%.2f)",
localDesc.StoreID, localDesc.Capacity.QueriesPerSecond, allStoresList.candidateQueriesPerSecond.mean, qpsMaxThreshold)

hottestRanges := sr.replRankings.topQPS()
for localDesc.Capacity.QueriesPerSecond > qpsMaxThreshold {
replWithStats, target, considerForRebalance := sr.chooseLeaseToTransfer(
Expand Down
35 changes: 35 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/fk
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3835,3 +3835,38 @@ NOTICE: type of foreign key column "parent_id" (int4) is not identical to refere

statement ok
DROP DATABASE db_type_test

# Regression test for #76852. Correctly maintain child indexes on virtual
# columns during a cascading delete.
statement ok
CREATE TABLE p76852 (
p STRING PRIMARY KEY,
i INT
);
CREATE TABLE c76852 (
b BOOL,
v STRING AS (b::STRING) VIRTUAL,
p STRING REFERENCES p76852 (p) ON DELETE CASCADE,
PRIMARY KEY (p, b),
INDEX idx (v)
);

statement ok
INSERT INTO p76852 (p, i) VALUES ('foo', 1);
INSERT INTO c76852 (b, p) VALUES (true, 'foo');
DELETE FROM p76852 WHERE true;

# Fast path case.
query B
SELECT b FROM c76852@idx WHERE b
----

statement ok
INSERT INTO p76852 (p, i) VALUES ('foo', 1);
INSERT INTO c76852 (b, p) VALUES (true, 'foo');
DELETE FROM p76852 WHERE i = 1;

# Non-fast path case.
query B
SELECT b FROM c76852@idx WHERE b
----
31 changes: 9 additions & 22 deletions pkg/sql/opt/optbuilder/fk_cascade.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,10 +284,9 @@ func (cb *onDeleteFastCascadeBuilder) Build(
mb.fetchScope = b.buildScan(
b.addTable(cb.childTable, &mb.alias),
tableOrdinals(cb.childTable, columnKinds{
includeMutations: false,
includeSystem: false,
includeInverted: false,
includeVirtualComputed: false,
includeMutations: false,
includeSystem: false,
includeInverted: false,
}),
nil, /* indexFlags */
noRowLocking,
Expand Down Expand Up @@ -503,20 +502,12 @@ func (b *Builder) buildDeleteCascadeMutationInput(
bindingProps *props.Relational,
oldValues opt.ColList,
) (outScope *scope) {
// We must fetch virtual computed columns for cascades that result in an
// update to the child table. The execution engine requires that the fetch
// columns are a superset of the update columns. See the related panic in
// execFactory.ConstructUpdate.
action := fk.DeleteReferenceAction()
fetchVirtualComputedCols := action == tree.SetNull || action == tree.SetDefault

outScope = b.buildScan(
b.addTable(childTable, childTableAlias),
tableOrdinals(childTable, columnKinds{
includeMutations: false,
includeSystem: false,
includeInverted: false,
includeVirtualComputed: fetchVirtualComputedCols,
includeMutations: false,
includeSystem: false,
includeInverted: false,
}),
nil, /* indexFlags */
noRowLocking,
Expand Down Expand Up @@ -747,16 +738,12 @@ func (b *Builder) buildUpdateCascadeMutationInput(
oldValues opt.ColList,
newValues opt.ColList,
) (outScope *scope) {
// We must fetch virtual computed columns for cascades. The execution engine
// requires that the fetch columns are a superset of the update columns. See
// the related panic in execFactory.ConstructUpdate.
outScope = b.buildScan(
b.addTable(childTable, childTableAlias),
tableOrdinals(childTable, columnKinds{
includeMutations: false,
includeSystem: false,
includeInverted: false,
includeVirtualComputed: true,
includeMutations: false,
includeSystem: false,
includeInverted: false,
}),
nil, /* indexFlags */
noRowLocking,
Expand Down
14 changes: 6 additions & 8 deletions pkg/sql/opt/optbuilder/mutation_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,10 +282,9 @@ func (mb *mutationBuilder) buildInputForUpdate(
mb.fetchScope = mb.b.buildScan(
mb.b.addTable(mb.tab, &mb.alias),
tableOrdinals(mb.tab, columnKinds{
includeMutations: true,
includeSystem: true,
includeInverted: false,
includeVirtualComputed: true,
includeMutations: true,
includeSystem: true,
includeInverted: false,
}),
indexFlags,
noRowLocking,
Expand Down Expand Up @@ -398,10 +397,9 @@ func (mb *mutationBuilder) buildInputForDelete(
mb.fetchScope = mb.b.buildScan(
mb.b.addTable(mb.tab, &mb.alias),
tableOrdinals(mb.tab, columnKinds{
includeMutations: true,
includeSystem: true,
includeInverted: false,
includeVirtualComputed: true,
includeMutations: true,
includeSystem: true,
includeInverted: false,
}),
indexFlags,
noRowLocking,
Expand Down
21 changes: 9 additions & 12 deletions pkg/sql/opt/optbuilder/mutation_builder_arbiter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,10 +287,9 @@ func (mb *mutationBuilder) buildAntiJoinForDoNothingArbiter(
fetchScope := mb.b.buildScan(
mb.b.addTable(mb.tab, &mb.alias),
tableOrdinals(mb.tab, columnKinds{
includeMutations: false,
includeSystem: false,
includeInverted: false,
includeVirtualComputed: true,
includeMutations: false,
includeSystem: false,
includeInverted: false,
}),
nil, /* indexFlags */
noRowLocking,
Expand Down Expand Up @@ -373,10 +372,9 @@ func (mb *mutationBuilder) buildLeftJoinForUpsertArbiter(
mb.fetchScope = mb.b.buildScan(
mb.b.addTable(mb.tab, &mb.alias),
tableOrdinals(mb.tab, columnKinds{
includeMutations: true,
includeSystem: true,
includeInverted: false,
includeVirtualComputed: true,
includeMutations: true,
includeSystem: true,
includeInverted: false,
}),
nil, /* indexFlags */
noRowLocking,
Expand Down Expand Up @@ -581,10 +579,9 @@ func (h *arbiterPredicateHelper) tableScope() *scope {
if h.tableScopeLazy == nil {
h.tableScopeLazy = h.mb.b.buildScan(
h.tabMeta, tableOrdinals(h.tabMeta.Table, columnKinds{
includeMutations: false,
includeSystem: false,
includeInverted: false,
includeVirtualComputed: true,
includeMutations: false,
includeSystem: false,
includeInverted: false,
}),
nil, /* indexFlags */
noRowLocking,
Expand Down
7 changes: 3 additions & 4 deletions pkg/sql/opt/optbuilder/mutation_builder_unique.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -399,10 +399,9 @@ func (h *uniqueCheckHelper) buildInsertionCheck() memo.UniqueChecksItem {
func (h *uniqueCheckHelper) buildTableScan() (outScope *scope, ordinals []int) {
tabMeta := h.mb.b.addTable(h.mb.tab, tree.NewUnqualifiedTableName(h.mb.tab.Name()))
ordinals = tableOrdinals(tabMeta.Table, columnKinds{
includeMutations: false,
includeSystem: false,
includeInverted: false,
includeVirtualComputed: true,
includeMutations: false,
includeSystem: false,
includeInverted: false,
})
return h.mb.b.buildScan(
tabMeta,
Expand Down
14 changes: 6 additions & 8 deletions pkg/sql/opt/optbuilder/select.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,9 @@ func (b *Builder) buildDataSource(
return b.buildScan(
tabMeta,
tableOrdinals(t, columnKinds{
includeMutations: false,
includeSystem: true,
includeInverted: false,
includeVirtualComputed: true,
includeMutations: false,
includeSystem: true,
includeInverted: false,
}),
indexFlags, locking, inScope,
)
Expand Down Expand Up @@ -399,10 +398,9 @@ func (b *Builder) buildScanFromTableRef(
ordinals = resolveNumericColumnRefs(tab, ref.Columns)
} else {
ordinals = tableOrdinals(tab, columnKinds{
includeMutations: false,
includeSystem: true,
includeInverted: false,
includeVirtualComputed: true,
includeMutations: false,
includeSystem: true,
includeInverted: false,
})
}

Expand Down
55 changes: 35 additions & 20 deletions pkg/sql/opt/optbuilder/testdata/fk-on-delete-cascade
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1005,7 +1005,8 @@ exec-ddl
CREATE TABLE child_virt (
c INT PRIMARY KEY,
p INT REFERENCES parent_virt(p) ON DELETE CASCADE,
v INT AS (p) VIRTUAL
v INT AS (p) VIRTUAL,
v2 STRING AS (c::STRING) VIRTUAL
)
----

Expand All @@ -1028,17 +1029,24 @@ root
└── cascade
└── delete child_virt
├── columns: <none>
├── fetch columns: c:12 child_virt.p:13
├── fetch columns: c:13 child_virt.p:14 v:15 v2:16
└── select
├── columns: c:12!null child_virt.p:13!null
├── scan child_virt
│ ├── columns: c:12!null child_virt.p:13
│ └── computed column expressions
│ └── v:14
│ └── child_virt.p:13
├── columns: c:13!null child_virt.p:14!null v:15 v2:16!null
├── project
│ ├── columns: v:15 v2:16!null c:13!null child_virt.p:14
│ ├── scan child_virt
│ │ ├── columns: c:13!null child_virt.p:14
│ │ └── computed column expressions
│ │ ├── v:15
│ │ │ └── child_virt.p:14
│ │ └── v2:16
│ │ └── c:13::STRING
│ └── projections
│ ├── child_virt.p:14 [as=v:15]
│ └── c:13::STRING [as=v2:16]
└── filters
├── child_virt.p:13 > 1
└── child_virt.p:13 IS DISTINCT FROM CAST(NULL AS INT8)
├── child_virt.p:14 > 1
└── child_virt.p:14 IS DISTINCT FROM CAST(NULL AS INT8)

# No fast path.
build-cascades
Expand All @@ -1060,17 +1068,24 @@ root
└── cascade
└── delete child_virt
├── columns: <none>
├── fetch columns: c:12 child_virt.p:13
├── fetch columns: c:13 child_virt.p:14 v:15 v2:16
└── semi-join (hash)
├── columns: c:12!null child_virt.p:13
├── scan child_virt
│ ├── columns: c:12!null child_virt.p:13
│ └── computed column expressions
│ └── v:14
│ └── child_virt.p:13
├── columns: c:13!null child_virt.p:14 v:15 v2:16!null
├── project
│ ├── columns: v:15 v2:16!null c:13!null child_virt.p:14
│ ├── scan child_virt
│ │ ├── columns: c:13!null child_virt.p:14
│ │ └── computed column expressions
│ │ ├── v:15
│ │ │ └── child_virt.p:14
│ │ └── v2:16
│ │ └── c:13::STRING
│ └── projections
│ ├── child_virt.p:14 [as=v:15]
│ └── c:13::STRING [as=v2:16]
├── with-scan &1
│ ├── columns: p:17!null
│ ├── columns: p:19!null
│ └── mapping:
│ └── parent_virt.p:4 => p:17
│ └── parent_virt.p:4 => p:19
└── filters
└── child_virt.p:13 = p:17
└── child_virt.p:14 = p:19
5 changes: 1 addition & 4 deletions pkg/sql/opt/optbuilder/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -708,9 +708,6 @@ type columnKinds struct {

// If true, include inverted index columns.
includeInverted bool

// If true, include virtual computed columns.
includeVirtualComputed bool
}

// tableOrdinals returns a slice of ordinals that correspond to table columns of
Expand All @@ -727,7 +724,7 @@ func tableOrdinals(tab cat.Table, k columnKinds) []int {
ordinals := make([]int, 0, n)
for i := 0; i < n; i++ {
col := tab.Column(i)
if shouldInclude[col.Kind()] && (k.includeVirtualComputed || !col.IsVirtualComputed()) {
if shouldInclude[col.Kind()] {
ordinals = append(ordinals, i)
}
}
Expand Down

0 comments on commit 4a8d2f5

Please sign in to comment.