Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release-20.2: sql: fix bug where bad mutation job state could block dropping tables #58255

Merged

Conversation

thoszhang
Copy link
Contributor

Backport:

Please see individual PRs for details.

/cc @cockroachdb/release

Previously, if a drop table statement was executed in a transaction
following other schema changes to the table in the same transaction,
an error would occur. This error was due to the drop table statement
marking previous jobs as succeeded and then proceeding to modify them.
This change ensures that drop table statement will delete all existing
jobs from the job cache so that it does not interfere with previous jobs.

Release note (sql change): A table can successfully be dropped in
a transaction following other schema changes to the table in the
same transaction.
@thoszhang thoszhang requested a review from ajwerner December 23, 2020 20:53
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@thoszhang
Copy link
Contributor Author

The race detector does not like this test. Let me fix it.

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.

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.
@thoszhang thoszhang force-pushed the backport20.2-56589-57836 branch from 8b4596a to 43f27d6 Compare December 23, 2020 23:27
@thoszhang
Copy link
Contributor Author

diff --git a/pkg/sql/schema_changer_test.go b/pkg/sql/schema_changer_test.go
index d45d242e9f..163da27d2a 100644
--- a/pkg/sql/schema_changer_test.go
+++ b/pkg/sql/schema_changer_test.go
@@ -6132,7 +6132,7 @@ INSERT INTO t.test VALUES (1, 2), (2, 2);
        g := ctxgroup.WithContext(ctx)
        g.GoCtx(func(ctx context.Context) error {
                // Try to create a unique index which won't be valid and will need a rollback.
-               _, err = sqlDB.Exec(`CREATE UNIQUE INDEX i ON t.test(v);`)
+               _, err := sqlDB.Exec(`CREATE UNIQUE INDEX i ON t.test(v);`)
                assert.Regexp(t, "violates unique constraint", err)
                return nil
        })

Should be fine now.

Copy link
Contributor

@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.

:lgtm:

Reviewed 2 of 2 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@thoszhang
Copy link
Contributor Author

This is hitting #51544. I'm just going to re-run the tests.

@thoszhang thoszhang merged commit 4ff7891 into cockroachdb:release-20.2 Dec 24, 2020
@thoszhang thoszhang deleted the backport20.2-56589-57836 branch December 24, 2020 02:09
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.

4 participants