release-20.1: sql: fix bugs in adding foreign keys #57810
Conversation
During the rollback of a schema change where a foreign key is added, we need to remove the backreference if it had already been installed. The problem was that we returned an assertion error from the schema changer if the backreference did not exist, which is a normal condition due to the backreference not having been installed yet. This patch fixes the bug by logging and swallowing the error when it occurs. Release note (bug fix): Fixed a bug where schema change jobs to add foreign keys to existing tables, via `ALTER TABLE`, could sometimes not be successfully reverted (either due to being canceled or having failed).
|
This change is |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 2 of 2 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained
pkg/sql/backfill.go, line 473 at r2 (raw file):
// planning to have been dropped in the meantime, since only the // presence of the backreference prevents it. _, err = tabledesc.FindFKReferencedIndex(backrefTable, constraint.ForeignKey.ReferencedColumnIDs)
this code has changed and needs an update to build on this branch.
We had a bug where concurrently deleting a unique index while adding a foreign key using that index as the required unique constraint would cause the foreign key to be added successfully, but with no unique constraint. The problem was that we only checked for the existence of the unique index at "planning" time, in the user transaction, but not when we actually added the backreference on the referenced table. This commit adds another check when we actually install the backreference. Release note (bug fix): Fixes a bug where concurrent addition of a foreign key constraint and drop of a unique index could cause the foreign key constraint to be added with no unique constraint on the referenced columns.
thoszhang
force-pushed
the
backport20.1-57598
branch
from
77a612f
to
970b79b
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/backfill.go, line 473 at r2 (raw file):
Previously, ajwerner wrote…
this code has changed and needs an update to build on this branch.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:
|
I think you'll get some trivial merge conflict in the tests in #55058. |
Ack. Rebased. |
Backport 2/2 commits from #57598.
/cc @cockroachdb/release
See individual commits for details.
Fixes #57592.
Fixes #57596.