Skip to content

Commit

Permalink
Merge #57836 #58204
Browse files Browse the repository at this point in the history
57836: sql: fix bug where bad mutation job state could block dropping tables r=lucy-zhang a=lucy-zhang

Previously, while dropping a table, we would mark all the jobs
associated with mutations on the table as `succeeded`, under the
assumption that they were running. The job registry API prohibits this
when the jobs are not `running` (or `pending`), so if a mutation was
stuck on the table descriptor with a failed or nonexistent job, dropping
the table would fail.

This PR fixes the bug by checking the job state before attempting to
update the job. It also fixes a related failure to drop a table caused
by a valid mutation job not being in a `running` state.

Fixes #57597.

Release note (bug fix): Fixed a bug where prior schema changes on a
table that failed and could not be fully reverted could prevent the
table from being dropped.

58204: opt: use join filters to imply IS NOT NULL partial index predicates r=RaduBerinde a=mgartner

#### opt: use join filters to imply IS NOT NULL partial index predicates

This commit updates the reject null normalization rules so that:

1. `RejectNullsUnderJoinRight` matches `SemiJoin` and `AntiJoin`.
2. A `Scan` with a `col IS NOT NULL` predicate requests null rejection
   of `col`.

In combination, these two changes allow partial indexes with
`IS NOT NULL` predicates to be used in more cases.

As an example, they can be used in JOINs where the ON condition
implicitly implies the predicate:

    CREATE TABLE a (a INT PRIMARY KEY)
    CREATE TABLE b (b INT, INDEX (b) WHERE b IS NOT NULL)

    SELECT * FROM a JOIN b ON a = b

As a another example, partial indexes with `IS NOT NULL` predicates can
be used to satisfy foreign key checks.

    CREATE TABLE parent (id INT PRIMARY KEY)

    CREATE TABLE child (
      id INT PRIMARY KEY,
      p_id INT,
      CONSTRAINT fk FOREIGN KEY (p_id) REFERENCES parent(id),
      INDEX (p_id) WHERE p_id IS NOT NULL
    )

    DELETE FROM p WHERE id = 1

The `DELETE` requires a foreign key check to ensure that no existing
rows in `child` have a matching `p_id`. Prior to this commit, the check
performed a full table scan rather than using the partial index because
its predicate was not implied. By pushing down the null-rejecting filter
derived from the check's `child.p_id = parent.id` filter, the partial
index can be used.

Fixes #57841

Release justification: This improves performance for joins and foreign
key checks on tables with partial indexes. Partial indexes are a new
feature introduced in 20.2.

Release note (performance improvement): Partial indexes with IS NOT NULL
predicates can be used in cases where JOIN filters implicitly imply the
predicate. This results in more efficient query plans for JOINs and
foreign checks.


Co-authored-by: Lucy Zhang <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
  • Loading branch information
3 people committed Dec 24, 2020
3 parents 7e604ec + 94be025 + b7398e8 commit 80d24ed
Show file tree
Hide file tree
Showing 8 changed files with 493 additions and 67 deletions.
44 changes: 41 additions & 3 deletions pkg/sql/drop_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import (
"fmt"
"strings"

"github.com/cockroachdb/cockroach/pkg/jobs"
"github.com/cockroachdb/cockroach/pkg/kv"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/security"
"github.com/cockroachdb/cockroach/pkg/server/telemetry"
Expand All @@ -26,6 +28,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
"github.com/cockroachdb/cockroach/pkg/util/timeutil"
"github.com/cockroachdb/errors"
Expand Down Expand Up @@ -442,6 +445,12 @@ func (p *planner) initiateDropTable(
// subsequent schema changes in the transaction (ie. this drop table statement) do not get a cache hit
// and do not try to update succeeded jobs, which would raise an error. Instead, this drop table
// statement will create a new job to drop the table.
//
// Note that we still wait for jobs removed from the cache to finish running
// after the transaction, since they're not removed from the jobsCollection.
// Also, changes made here do not affect schema change jobs created in this
// transaction with no mutation ID; they remain in the cache, and will be
// updated when writing the job record to drop the table.
jobIDs := make(map[int64]struct{})
var id descpb.MutationID
for _, m := range tableDesc.Mutations {
Expand All @@ -455,9 +464,38 @@ func (p *planner) initiateDropTable(
}
}
for jobID := range jobIDs {
if err := p.ExecCfg().JobRegistry.Succeeded(ctx, p.txn, jobID); err != nil {
return errors.Wrapf(err,
"failed to mark job %d as as successful", errors.Safe(jobID))
// Mark jobs as succeeded when possible, but be defensive about jobs that
// are already in a terminal state or nonexistent. This could happen for
// schema change jobs that couldn't be successfully reverted and ended up in
// a failed state. Such jobs could have already been GCed from the jobs
// table by the time this code runs.
mutationJob, err := p.execCfg.JobRegistry.LoadJobWithTxn(ctx, jobID, p.txn)
if err != nil {
if jobs.HasJobNotFoundError(err) {
log.Warningf(ctx, "mutation job %d not found", jobID)
continue
}
return err
}
if err := mutationJob.WithTxn(p.txn).Update(
ctx, func(txn *kv.Txn, md jobs.JobMetadata, ju *jobs.JobUpdater) error {
status := md.Status
switch status {
case jobs.StatusSucceeded, jobs.StatusCanceled, jobs.StatusFailed:
log.Warningf(ctx, "mutation job %d in unexpected state %s", jobID, status)
return nil
case jobs.StatusRunning, jobs.StatusPending:
status = jobs.StatusSucceeded
default:
// We shouldn't mark jobs as succeeded if they're not in a state where
// they're eligible to ever succeed, so mark them as failed.
status = jobs.StatusFailed
}
log.Infof(ctx, "marking mutation job %d for dropped table as %s", jobID, status)
ju.UpdateStatus(status)
return nil
}); err != nil {
return errors.Wrap(err, "updating mutation job for dropped table")
}
delete(p.ExtendedEvalContext().SchemaChangeJobCache, tableDesc.ID)
}
Expand Down
88 changes: 88 additions & 0 deletions pkg/sql/opt/exec/execbuilder/testdata/fk
Original file line number Diff line number Diff line change
Expand Up @@ -1520,3 +1520,91 @@ vectorized: true
estimated row count: 10
table: small_parent@primary
spans: FULL SCAN

# Test that partial indexes with IS NOT NULL predicates are used for performing
# FK checks.
subtest partial_index

statement ok
CREATE TABLE partial_parent (
id INT PRIMARY KEY
)

statement ok
CREATE TABLE partial_child (
id INT PRIMARY KEY,
parent_id INT,
CONSTRAINT fk FOREIGN KEY (parent_id) REFERENCES partial_parent(id),
INDEX partial_idx (parent_id) WHERE parent_id IS NOT NULL
)

query T
EXPLAIN DELETE FROM partial_parent WHERE id = 1
----
distribution: local
vectorized: true
·
• root
├── • delete
│ │ from: partial_parent
│ │
│ └── • buffer
│ │ label: buffer 1
│ │
│ └── • scan
│ missing stats
│ table: partial_parent@primary
│ spans: [/1 - /1]
└── • constraint-check
└── • error if rows
└── • lookup join (semi)
│ table: partial_child@partial_idx (partial index)
│ equality: (id) = (parent_id)
└── • scan buffer
label: buffer 1

query T
EXPLAIN UPDATE partial_parent SET id = 2 WHERE id = 1
----
distribution: local
vectorized: true
·
• root
├── • update
│ │ table: partial_parent
│ │ set: id
│ │
│ └── • buffer
│ │ label: buffer 1
│ │
│ └── • render
│ │
│ └── • scan
│ missing stats
│ table: partial_parent@primary
│ spans: [/1 - /1]
│ locking strength: for update
└── • constraint-check
└── • error if rows
└── • lookup join (semi)
│ table: partial_child@partial_idx (partial index)
│ equality: (id) = (parent_id)
└── • except
├── • scan buffer
│ label: buffer 1
└── • scan buffer
label: buffer 1

subtest end
125 changes: 125 additions & 0 deletions pkg/sql/opt/exec/execbuilder/testdata/partial_index
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
# LogicTest: local

statement ok
CREATE TABLE t (
a INT PRIMARY KEY,
b INT,
c STRING,
FAMILY (a, b, c),
INDEX b_partial (b) WHERE b > 10
)

statement ok
CREATE TABLE inv (
a INT PRIMARY KEY,
b JSON,
c STRING,
INVERTED INDEX i (b) WHERE c IN ('foo', 'bar'),
FAMILY (a, b, c)
)

# ---------------------------------------------------------
# EXPLAIN
# ---------------------------------------------------------

# EXPLAIN output shows the partial index label on scans and joins on partial
# indexes.
query T
EXPLAIN SELECT b FROM t WHERE b > 10
----
distribution: local
vectorized: true
·
• scan
missing stats
table: t@b_partial (partial index)
spans: FULL SCAN

query T
EXPLAIN SELECT t1.a FROM t t1 INNER LOOKUP JOIN t t2 ON t1.a = t2.b AND t2.b > 10
----
distribution: local
vectorized: true
·
• lookup join
│ table: t@b_partial (partial index)
│ equality: (a) = (b)
└── • scan
missing stats
table: t@primary
spans: [/11 - ]

query T
EXPLAIN SELECT a FROM inv@i WHERE b @> '{"x": "y"}' AND c IN ('foo', 'bar')
----
distribution: local
vectorized: true
·
• scan
missing stats
table: inv@i (partial index)
spans: 1 span

query T
EXPLAIN SELECT a FROM inv@i WHERE b @> '{"x": "y"}' AND c = 'foo'
----
distribution: local
vectorized: true
·
• filter
│ filter: c = 'foo'
└── • index join
│ table: inv@primary
└── • scan
missing stats
table: inv@i (partial index)
spans: 1 span

query T
EXPLAIN SELECT * FROM inv@i WHERE b @> '{"x": "y"}' AND c IN ('foo', 'bar')
----
distribution: local
vectorized: true
·
• index join
│ table: inv@primary
└── • scan
missing stats
table: inv@i (partial index)
spans: 1 span

# ---------------------------------------------------------
# JOIN
# ---------------------------------------------------------

statement ok
CREATE TABLE a (a INT PRIMARY KEY);

statement ok
CREATE TABLE b (b INT, INDEX (b) WHERE b IS NOT NULL)

# The partial index can be used because the ON condition implicitly implies the
# partial index predicate, b IS NOT NULL.
query T
EXPLAIN SELECT * FROM a JOIN b ON a = b
----
distribution: local
vectorized: true
·
• merge join
│ equality: (a) = (b)
│ left cols are key
├── • scan
│ missing stats
│ table: a@primary
│ spans: FULL SCAN
└── • scan
missing stats
table: b@b_b_idx (partial index)
spans: FULL SCAN
58 changes: 0 additions & 58 deletions pkg/sql/opt/exec/execbuilder/testdata/partial_index_nonmetamorphic
Original file line number Diff line number Diff line change
Expand Up @@ -882,64 +882,6 @@ UPSERT INTO inv VALUES (6, '{"x": "y", "num": 8}', 'baz')
Scan /Table/55/1/6{-/#}
CPut /Table/55/1/6/0 -> /TUPLE/

# ---------------------------------------------------------
# EXPLAIN
# ---------------------------------------------------------

# EXPLAIN output shows the partial index label on scans over partial indexes.
query T
EXPLAIN SELECT b FROM t WHERE b > 10
----
distribution: local
vectorized: true
·
• scan
missing stats
table: t@b_partial (partial index)
spans: FULL SCAN

query T
EXPLAIN SELECT a FROM inv@i WHERE b @> '{"x": "y"}' AND c IN ('foo', 'bar')
----
distribution: local
vectorized: true
·
• scan
missing stats
table: inv@i (partial index)
spans: 1 span

query T
EXPLAIN SELECT a FROM inv@i WHERE b @> '{"x": "y"}' AND c = 'foo'
----
distribution: local
vectorized: true
·
• filter
│ filter: c = 'foo'
└── • index join
│ table: inv@primary
└── • scan
missing stats
table: inv@i (partial index)
spans: 1 span

query T
EXPLAIN SELECT * FROM inv@i WHERE b @> '{"x": "y"}' AND c IN ('foo', 'bar')
----
distribution: local
vectorized: true
·
• index join
│ table: inv@primary
└── • scan
missing stats
table: inv@i (partial index)
spans: 1 span

# Regression test for #57085. Cascading DELETEs should not issue DEL operations
# for partial indexes of a child table when the deleted row was not in the
# partial index.
Expand Down
Loading

0 comments on commit 80d24ed

Please sign in to comment.