Skip to content

Commit

Permalink
Merge #49126 #49163
Browse files Browse the repository at this point in the history 
49126: opt: improve the rule that converts COUNT to COUNT_ROWS r=RaduBerinde a=RaduBerinde

Improve the ConvertCountToCountRows rule to check for any scalar expression that
is never null, not just bare variable references.

Release note (performance improvement): `COUNT` is now converted to `COUNT(*)`
(aka `COUNT_ROWS`) in more cases.

49163: opt: move interleaved fast delete logic to the optimizer r=RaduBerinde a=RaduBerinde

#### sql: move delete fast path tests to logictests

Rewrite the interleaved fast path tests as logic/execbuilder tests, so that they
continue to work when the fast path is reimplemented in the optimizer.

Release note: None

#### opt: move interleaved fast delete logic to the optimizer

DeleteRange is used as a fast path for delete when it is safe to simply delete
an entire range of keys. There are two separate paths, depending if the table
has interleaved children. The non-interleaved path is implemented in the
optimizer, but the interleaved path is legacy code called from the exec factory.

This change moves the interleaved fast path to the optimizer. This is necessary
to allow use of the fast path when opt-driven cascades are enabled.

The conditions for using DeleteRange on an interleaving are quite strict - there
must be foreign key with ON DELETE cascade set up for all tables in the
interleaving.

The existing path did not check the columns of the foreign keys; in principle
we could set up foreign keys between the expected tables but on columns that
don't match the interleaving. This is not a problem in practice right now
because any of those cases would result in secondary indexes, which prevent the
fast path regardless. But this won't be the case once we relax the index
requirements, so the new path checks the columns as well.

Release note: None

Co-authored-by: Radu Berinde <[email protected]>
  • Loading branch information
@RaduBerinde
craig[bot] and RaduBerinde committed May 18, 2020
3 parents eeefaad + 7d80364 + 406d140 commit 2090a5b
Show file tree
Hide file tree
Showing 14 changed files with 594 additions and 488 deletions.
74 changes: 0 additions & 74 deletions pkg/sql/delete.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
"context"
"sync"

"github.com/cockroachdb/cockroach/pkg/sql/row"
"github.com/cockroachdb/cockroach/pkg/sql/rowcontainer"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
Expand Down Expand Up @@ -215,79 +214,6 @@ func (d *deleteNode) Close(ctx context.Context) {
deleteNodePool.Put(d)
}

func canDeleteFastInterleaved(table *ImmutableTableDescriptor, fkTables row.FkTableMetadata) bool {
// If there are no interleaved tables then don't take the fast path.
// This avoids superfluous use of DelRange in cases where there isn't as much of a performance boost.
hasInterleaved := false
for _, idx := range table.AllNonDropIndexes() {
if len(idx.InterleavedBy) > 0 {
hasInterleaved = true
break
}
}
if !hasInterleaved {
return false
}

// if the base table is interleaved in another table, fail
for _, idx := range fkTables[table.ID].Desc.AllNonDropIndexes() {
if len(idx.Interleave.Ancestors) > 0 {
return false
}
}

interleavedQueue := []sqlbase.ID{table.ID}
// interleavedIdxs will contain all of the table and index IDs of the indexes
// interleaved in any of the interleaved hierarchy of the input table.
interleavedIdxs := make(map[sqlbase.ID]map[sqlbase.IndexID]struct{})
for len(interleavedQueue) > 0 {
tableID := interleavedQueue[0]
interleavedQueue = interleavedQueue[1:]
if _, ok := fkTables[tableID]; !ok {
return false
}
tableDesc := fkTables[tableID].Desc
if tableDesc == nil {
return false
}
for _, idx := range tableDesc.AllNonDropIndexes() {
// Don't allow any secondary indexes
// TODO(emmanuel): identify the cases where secondary indexes can still work with the fast path and allow them
if idx.ID != tableDesc.PrimaryIndex.ID {
return false
}

for _, ref := range idx.InterleavedBy {
if _, ok := interleavedIdxs[ref.Table]; !ok {
interleavedIdxs[ref.Table] = make(map[sqlbase.IndexID]struct{})
}
interleavedIdxs[ref.Table][ref.Index] = struct{}{}

}

for _, ref := range idx.InterleavedBy {
interleavedQueue = append(interleavedQueue, ref.Table)
}

}

// The index can't be referenced by anything that's not the interleaved relationship.
for i := range tableDesc.InboundFKs {
fk := &tableDesc.InboundFKs[i]
if _, ok := interleavedIdxs[fk.OriginTableID]; !ok {
// This table is referenced by a foreign key that lives outside of the
// interleaved hierarchy, so we can't fast path delete.
return false
}
// All of these references MUST be ON DELETE CASCADE.
if fk.OnDelete != sqlbase.ForeignKeyReference_CASCADE {
return false
}
}
}
return true
}

func (d *deleteNode) enableAutoCommit() {
d.run.td.enableAutoCommit()
}
78 changes: 0 additions & 78 deletions pkg/sql/delete_range.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,84 +61,6 @@ var _ planNode = &deleteRangeNode{}
var _ planNodeFastPath = &deleteRangeNode{}
var _ batchedPlanNode = &deleteRangeNode{}

// canDeleteFast determines if the deletion of `rows` can be done
// without actually scanning them.
// This should be called after plan simplification for optimal results.
//
// fkTables is required primarily when performing deletes with interleaved child
// tables as we need to pass those foreign key table references to the Fetcher
// (to ensure that keys deleted from child tables are truly within the range to be deleted).
//
// This logic should be kept in sync with exec.Builder.canUseDeleteRange.
// TODO(andyk): Remove when the heuristic planner code is removed.
func maybeCreateDeleteFastNode(
ctx context.Context,
source planNode,
desc *ImmutableTableDescriptor,
fkTables row.FkTableMetadata,
fastPathInterleaved bool,
rowsNeeded bool,
) (*deleteRangeNode, bool) {
// Check that there are no secondary indexes, interleaving, FK
// references checks, etc., ie. there is no extra work to be done
// per row deleted.
if !fastPathDeleteAvailable(ctx, desc) && !fastPathInterleaved {
return nil, false
}

// If the rows are needed (a RETURNING clause), we can't skip them.
if rowsNeeded {
return nil, false
}

// Check whether the source plan is "simple": that it contains no remaining
// filtering, limiting, sorting, etc.
// TODO(dt): We could probably be smarter when presented with an
// index-join, but this goes away anyway once we push-down more of
// SQL.
maybeScan := source
if sel, ok := maybeScan.(*renderNode); ok {
// There may be a projection to drop/rename some columns which the
// optimizations did not remove at this point. We just ignore that
// projection for the purpose of this check.
maybeScan = sel.source.plan
}

scan, ok := maybeScan.(*scanNode)
if !ok {
// Not simple enough. Bail.
return nil, false
}

// A scan ought to be simple enough, except when it's not: a scan
// may have a remaining filter. We can't be fast over that.
if scan.filter != nil {
if log.V(2) {
log.Infof(ctx, "delete forced to scan: values required for filter (%s)", scan.filter)
}
return nil, false
}

if scan.hardLimit != 0 {
if log.V(2) {
log.Infof(ctx, "delete forced to scan: scan has limit %d", scan.hardLimit)
}
return nil, false
}

interleavedDesc := make([]*sqlbase.ImmutableTableDescriptor, 0, len(fkTables))
for _, tableEntry := range fkTables {
interleavedDesc = append(interleavedDesc, tableEntry.Desc)
}

return &deleteRangeNode{
interleavedFastPath: fastPathInterleaved,
spans: scan.spans,
desc: desc,
interleavedDesc: interleavedDesc,
}, true
}

// BatchedNext implements the batchedPlanNode interface.
func (d *deleteRangeNode) BatchedNext(params runParams) (bool, error) {
return false, nil
Expand Down
215 changes: 0 additions & 215 deletions pkg/sql/delete_test.go
View file

This file was deleted.

Loading

0 comments on commit 2090a5b

Please sign in to comment.