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

sql,schemachanger: disallow dropping indexes when referenced in UDF/View #109611

Conversation

Xiang-Gu
Copy link
Contributor

This PR disallows schema changes (in both legacy and declarative schema changers) that would drop indexes referenced explicitly via index hinting in UDF or view. They include DROP INDEX, ADD COLUMN, DROP COLUMN, and ALTER PRIMARY KEY.

Fixes #108974
Epic: None
Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@Xiang-Gu Xiang-Gu force-pushed the disallow-dropping-index-when-referenced-in-udf-or-view branch from 8bacd7c to 7a4965e Compare August 28, 2023 19:56
@Xiang-Gu Xiang-Gu marked this pull request as ready for review August 28, 2023 20:39
@Xiang-Gu Xiang-Gu requested review from a team as code owners August 28, 2023 20:39
@Xiang-Gu Xiang-Gu requested review from lidorcarmel and removed request for a team August 28, 2023 20:39
@Xiang-Gu
Copy link
Contributor Author

This is ready for a look. CI failure is due to flaky tests unrelated to this PR.

Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

A couple of minor comments but :LGTM:

Reviewed 9 of 9 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @lidorcarmel and @Xiang-Gu)


-- commits line 5 at r1:
minor typo


pkg/sql/rename_table.go line 311 at r1 (raw file):

		viewName = viewFQName.FQString()
	}
	return sqlerrors.NewDependingObjBlocksOpError(op, typeName, objName, "view", viewName)

Could we call this NewDependentBlocksOpError instead?

@Xiang-Gu Xiang-Gu force-pushed the disallow-dropping-index-when-referenced-in-udf-or-view branch from 7a4965e to 8e49387 Compare August 29, 2023 16:03
This commit moves a common error message pattern "cannot drop something
because something else depends on it" into sqlerrors package.

Release note: None
This commits disallows schema changes to a table that would drop an
index that is referenced explicitly via index hinting in a UDF or view
body.

Release note: None
@Xiang-Gu Xiang-Gu force-pushed the disallow-dropping-index-when-referenced-in-udf-or-view branch from 8e49387 to dcb2acf Compare August 29, 2023 16:04
Copy link
Contributor Author

@Xiang-Gu Xiang-Gu 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 (and 1 stale) (waiting on @fqazi and @lidorcarmel)


pkg/sql/rename_table.go line 311 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Could we call this NewDependentBlocksOpError instead?

Done.

@craig
Copy link
Contributor

craig bot commented Aug 29, 2023

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@craig
Copy link
Contributor

craig bot commented Aug 29, 2023

Build succeeded:

@craig craig bot merged commit c2c39fa into cockroachdb:master Aug 29, 2023
@Xiang-Gu Xiang-Gu deleted the disallow-dropping-index-when-referenced-in-udf-or-view branch August 29, 2023 20:22
@rafiss
Copy link
Collaborator

rafiss commented Sep 18, 2023

blathers backport 23.1

@blathers-crl
Copy link

blathers-crl bot commented Sep 18, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from dcb2acf to blathers/backport-release-23.1-109611: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants