Skip to content

backupccl: stop swallowing error in restoreResumer.OnFailOrCancel()#43218

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
ajwerner:ajwerner/stop-swallowing-errors-in-restoreResumer.OnFailOrCancel
Dec 17, 2019
Merged

backupccl: stop swallowing error in restoreResumer.OnFailOrCancel()#43218
craig[bot] merged 1 commit intocockroachdb:masterfrom
ajwerner:ajwerner/stop-swallowing-errors-in-restoreResumer.OnFailOrCancel

Conversation

@ajwerner
Copy link
Copy Markdown
Contributor

In restoreResumer.OnFailOrCancel() we'd return nil when encountering a
non-nil error in a call to sqlbase.RemovePublicTableNamespaceEntry().
Error handling in jobs.Resumer.OnFailOrCancel() is a tricky proposition but
this behavior is certainly wrong.

Release note: None

@ajwerner ajwerner requested a review from arulajmani December 16, 2019 23:30
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@ajwerner
Copy link
Copy Markdown
Contributor Author

@arulajmani feel free to indicate an alternative victim to review this one-liner. While if err != nil { return nil } is not the right thing to do here, just thinking about error handling inside of OnFailOrCancel stresses me out.

Copy link
Copy Markdown
Collaborator

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor comment about error wrapping.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @arulajmani)


pkg/ccl/backupccl/restore.go, line 1793 at r1 (raw file):

		err := sqlbase.RemovePublicTableNamespaceEntry(ctx, txn, tbl.ParentID, tbl.Name)
		if err != nil {
			return err

nit: Maybe return errors.Wrap(err, "dropping tables")? That seemed to be the error wrapping before I made the change that broke this.

In `restoreResumer.OnFailOrCancel()` we'd return nil when encountering a
non-nil error in a call to `sqlbase.RemovePublicTableNamespaceEntry()`.
Error handling in `jobs.Resumer.OnFailOrCancel()` is a tricky proposition but
this behavior is certainly wrong.

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/stop-swallowing-errors-in-restoreResumer.OnFailOrCancel branch from d6d13e5 to db30a84 Compare December 17, 2019 14:38
Copy link
Copy Markdown
Contributor Author

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/ccl/backupccl/restore.go, line 1793 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: Maybe return errors.Wrap(err, "dropping tables")? That seemed to be the error wrapping before I made the change that broke this.

Done.

craig bot pushed a commit that referenced this pull request Dec 17, 2019
43194: opt: use transitive equalities to infer filters r=rytaft a=rytaft

Previously, the optimizer was not inferring all possible filters
based on equality conditions. For example, consider a query such
as:
```
SELECT * FROM t
JOIN u ON t.a = u.a
JOIN v ON u.a = v.a
JOIN w ON v.a = w.a
WHERE t.a = 5;
```
Ideally, the optimizer should infer additional filter conditions
`u.a = 5 AND v.a = 5 AND w.a = 5` based on the equality conditions.
However, this was not occurring because the optimizer only considered
equality conditions that were at the same level in the query tree as the
filter to be mapped. Now the optimizer also considers equality conditions
that are further down in the tree, so that additional filters can
be inferred based on transitive equalities.

Informs #43039

Release note (performance improvement): The optimizer can now infer
additional filter conditions in some cases based on transitive equalities
between columns.

43218: backupccl: stop swallowing error in restoreResumer.OnFailOrCancel() r=ajwerner a=ajwerner

In `restoreResumer.OnFailOrCancel()` we'd return nil when encountering a
non-nil error in a call to `sqlbase.RemovePublicTableNamespaceEntry()`.
Error handling in `jobs.Resumer.OnFailOrCancel()` is a tricky proposition but
this behavior is certainly wrong.

Release note: None

Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com>
@ajwerner
Copy link
Copy Markdown
Contributor Author

Flaked on #43141

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Dec 17, 2019

Build succeeded

@craig craig bot merged commit db30a84 into cockroachdb:master Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants