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: gate potentially unsafe schema changes behind sql_safe_updates #51863

Closed
ajwerner opened this issue Jul 23, 2020 · 2 comments
Closed

sql: gate potentially unsafe schema changes behind sql_safe_updates #51863

ajwerner opened this issue Jul 23, 2020 · 2 comments
Assignees
Labels
A-schema-changes C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@ajwerner
Copy link
Contributor

Is your feature request related to a problem? Please describe.
There are some combinations of schema changes which can be hazardous and potentially cause data loss. Their are two classes of known problems to which this issue pertains. One, it can be problematic to drop columns which have constraints (see #47719). The second set of problems pertains to transactions which drop columns and perform other asynchronous schema changes which might fail. In those scenarios (#46541, #47712).

These situations are bad, but are not hard to avoid. Let's help guide our users to avoid them.

Describe the solution you'd like

I'd like the sql_safe_updates flag to prevent the two above problems. If you want to drop a column and you have sql_safe_updates enabled, then you need to drop its constraints first and you cannot drop it in the same transaction that adds columns with default or computed expressions or adds other constraints.

Describe alternatives you've considered
We should ultimately fix the underlying bugs but it's challenging.

@blathers-crl
Copy link

blathers-crl bot commented Jul 23, 2020

Hi @ajwerner, I've guessed the C-ategory of your issue and suitably labeled it. Please re-label if inaccurate.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Jul 23, 2020
@thoszhang thoszhang added release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. and removed release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. labels Sep 1, 2020
@ajwerner ajwerner self-assigned this Sep 22, 2020
@ajwerner
Copy link
Contributor Author

ajwerner commented Oct 1, 2020

I regret taking so long to test this in my implementation. I changed the implementation and then by the time I got to testing it I realized that all column drops are already blocked by sql_safe_updates and column drops are required in all of these cases :(. At least there's no time pressure on a backport now ...

@ajwerner ajwerner closed this as completed Oct 1, 2020
ajwerner added a commit to ajwerner/cockroach that referenced this issue Oct 6, 2020
Explicit transactions with drop column are very dangerous. Inform the user.

Touches cockroachdb#51863.

Release note (sql change): Added a note to inform users with an issue link
when attempting potentially hazardous DROP COLUMN operations when
sql_safe_updates is enabled.
ajwerner added a commit to ajwerner/cockroach that referenced this issue Oct 13, 2020
Explicit transactions with drop column are very dangerous. Inform the user.

Touches cockroachdb#51863.

Release note (sql change): Added a note to inform users with an issue link
when attempting potentially hazardous DROP COLUMN operations when
sql_safe_updates is enabled.
craig bot pushed a commit that referenced this issue Oct 20, 2020
55248: sql: add issue link to ALTER TABLE DROP COLUMN with sql_safe_updates r=lucy-zhang a=ajwerner

Explicit transactions with drop column are very dangerous. Inform the user.

Touches #51863.

Release note (sql change): Added a note to inform users with an issue link
when attempting potentially hazardous DROP COLUMN operations when
sql_safe_updates is enabled.

55655: xform: organize custom functions into general_funcs.go and scan_funcs.go r=RaduBerinde a=mgartner

#### xform: rename custom_funcs.go to general_funcs.go

This is in preparation for breaking up the custom functions into
multiple files, similar to the organization of normalization custom
functions.

Release note: None

#### xform: move scan-related custom functions to scan_funcs.go

This is one step in breaking up the exploration custom functions into
files based on the rules they facilitate, similar to the normalization
custom functions.

Release note: None


55721: roachtest: bump estimated max warehouses of tpccbench/nodes=3/cpu=16 r=nvanbenschoten a=nvanbenschoten

Now that we're importing instead of restoring and picking up a number
of optimizations along the way, this estimated max warehouse count is
too low. This is why we have been flatlining on:
https://roachperf.crdb.dev/?filter=&view=tpccbench%2Fnodes%3D3%2Fcpu%3D16&tab=gce

55722: sql: ban renaming table to a new database unless both schemas are public r=lucy-zhang a=lucy-zhang

Previously we would seemingly allow renaming tables to a new database
with arbitrary source and target schemas without ever actually
reassigning the schema ID. This could lead to a corrupted descriptor
with an invalid schema ID for the database. This PR fixes the
implementation to return an error when renaming a table to a different
database unless both the source and target schemas are `public`.

Fixes #55710.

Release note (bug fix): Previously, changing the parent database and
schema of a table using `RENAME` was seemingly permitted but would lead
to corruption of the table metadata. Now an error is returned when
attempting to rename a table to a different database except in the case
where both the source and target schemas are the `public` schema in each
database, which continues to be supported.

Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Lucy Zhang <lucy@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-schema-changes C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

No branches or pull requests

2 participants