sql: reduce UX surprise with DROP DATABASE and ALTER DATABASE RENAME #24246
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to this patch, a user logged into the CLI shell and taking a database name's away while it is set as "current database", either via DROP DATABASE or ALTER DATABASE RENAME, would cause the subsequent prompt refresh and possibly further statements to fail, due to an inexistent current database. This behavior is arguably surprising and raises the question for unsuspecting/beginner users: why does the thing let me do something that breaks everything afterwards? This patch enhances the issue by preventing DROP or ALTER DATABASE RENAME if the name taken away is set as current database, when `sql_safe_updates` is set. (It is set for interactive sessions with `cockroach sql`). The following alternatives were explored but eventually dismissed: - prevent DROP or ALTER DATABASE RENAME altogether on the current database. This is unfriendly to client apps which may expect "DROP DATABASE" followed immediately by "CREATE DATABASE" to keep the session valid. - allow DROP and ALTER DATABASE RENAME but silently change the current database to empty (or some other valid value, like "system"). This is unfriendly to client apps which only set the current database via a URL path and expect the setting to remain stable. Release note (sql change): `DROP DATABASE` and `ALTER DATABASE RENAME` now prevent the removal of a database name if that database is set as the current database (`SET database` / `USE`) and the session setting `sql_safe_updates` is also set.
|
This change is |
|
Comments from Reviewable |
|
Thanks! |
|
@dianasaur323 lmk if this needs to be cherry picked |
|
No, I don’t think it’s a huge deal and it can wait.
On Tue, Mar 27, 2018 at 2:47 PM kena ***@***.***> wrote:
@dianasaur323 <https://github.com/dianasaur323> lmk if this needs to be
cherry picked
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#24246 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFgDWP_JPB6inbm6bXuAE57n753v3FIbks5tiolNgaJpZM4S8sOv>
.
--
Diana Hsieh
diana@cockroachlabs.com
407-690-9048
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #23661. cc @dianasaur323
Prior to this patch, a user logged into the CLI shell and taking a
database name's away while it is set as "current database", either via
DROP DATABASE or ALTER DATABASE RENAME, would cause the subsequent
prompt refresh and possibly further statements to fail, due to an
inexistent current database.
This behavior is arguably surprising and raises the question for
unsuspecting/beginner users: why does the thing let me do something
that breaks everything afterwards?
This patch enhances the issue by preventing DROP or ALTER DATABASE
RENAME if the name taken away is set as current database, when
sql_safe_updatesis set. (It is set for interactive sessions withcockroach sql).The following alternatives were explored but eventually dismissed:
prevent DROP or ALTER DATABASE RENAME altogether on the current
database. This is unfriendly to client apps which may expect "DROP
DATABASE" followed immediately by "CREATE DATABASE" to keep the
session valid.
allow DROP and ALTER DATABASE RENAME but silently change the current
database to empty (or some other valid value, like "system"). This
is unfriendly to client apps which only set the current database via
a URL path and expect the setting to remain stable.
Release note (sql change):
DROP DATABASEandALTER DATABASE RENAMEnow prevent the removal of a database name if that database is set as
the current database (
SET database/USE) and the session settingsql_safe_updatesis also set.