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.

Sign up for GitHub

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

Jump to bottom

sql: DROP SCHEMA CASCADE breaks cross-schema TypeDescriptor references #59021

Closed
arulajmani opened this issue Jan 14, 2021 · 2 comments · Fixed by #59281
Closed

sql: DROP SCHEMA CASCADE breaks cross-schema TypeDescriptor references #59021

arulajmani opened this issue Jan 14, 2021 · 2 comments · Fixed by #59281
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@arulajmani
Copy link
Collaborator

Describe the problem

Currently, CRDB does not support DROP TYPE CASCADE, but we don't check for dependencies when the type is being dropped as part of a DROP SCHEMA CASCADE. This is problematic when the type is involved in a cross schema reference.

To Reproduce

[email protected]:26257/movr> create schema sc;
CREATE SCHEMA

Time: 31ms total (execution 9ms / network 22ms)

[email protected]:26257/movr> create type sc.abc as enum ('a', 'b', 'c');
CREATE TYPE

Time: 8ms total (execution 8ms / network 0ms)

[email protected]:26257/movr> create table t(k INT PRIMARY KEY, a sc.abc);
CREATE TABLE

Time: 59ms total (execution 8ms / network 52ms)

[email protected]:26257/movr> DROP SCHEMA sc;
ERROR: schema "sc" is not empty and CASCADE was not specified
SQLSTATE: 2BP01
[email protected]:26257/movr> DROP SCHEMA sc CASCADE;
DROP SCHEMA

Time: 34ms total (execution 11ms / network 23ms)

[email protected]:26257/movr> Insert into t values(1, 'a');
ERROR: type with ID 60 does not exist
SQLSTATE: 42704

Expected behavior
Similar to DROP TYPE CASCADE, the column that uses the enum being dropped should be dropped.

Additional data / screenshots
In the short term, when dropping a schema which contains types, we should ascertain that none of the types have cross schema references. If they exist, we should fail the DROP SCHEMA CASCADE, until the user manually removes the columns that use that type.
In the long term, we should really be supporting DROP TYPE CASCADE : #51480

Related to: #51889
@arulajmani arulajmani added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Jan 14, 2021
@arulajmani arulajmani mentioned this issue Jan 14, 2021
Closed
@jordanlewis
Copy link
Member

Can we prevent DROP SCHEMA CASCADE for now if there are any user defined types in the schema at all, and force people to drop types themselves?

@ajwerner
Copy link
Contributor

Wish I had seen this earlier. This seems very bad. I'll make a PR.

ajwerner added a commit to ajwerner/cockroach that referenced this issue Jan 22, 2021
@ajwerner
sql: prevent DROP SCHEMA CASCADE from droping types with references
f5a87ce 
Before this patch, a DROP SCHEMA CASCADE could cause database corruption
by dropping types which were referenced by other tables. This would lead to
bad errors like:

```
ERROR: object already exists: desc 687: type ID 685 in descriptor not found: descriptor not found
SQLSTATE: 42710
```

And doctor errors like:
```
   Table 687: ParentID  50, ParentSchemaID 29, Name 't': type ID 685 in descriptor not found: descriptor not found
```

Fixes cockroachdb#59021.

Release note (bug fix): Fixed a bug where `DROP SCHEMA ... CASCADE` could
result in types which are referenced being dropped.
@ajwerner ajwerner mentioned this issue Jan 22, 2021
Merged
ajwerner added a commit to ajwerner/cockroach that referenced this issue Jan 28, 2021
@ajwerner
sql: prevent DROP SCHEMA CASCADE from droping types with references
9dd120c 
Before this patch, a DROP SCHEMA CASCADE could cause database corruption
by dropping types which were referenced by other tables. This would lead to
bad errors like:

```
ERROR: object already exists: desc 687: type ID 685 in descriptor not found: descriptor not found
SQLSTATE: 42710
```

And doctor errors like:
```
   Table 687: ParentID  50, ParentSchemaID 29, Name 't': type ID 685 in descriptor not found: descriptor not found
```

Fixes cockroachdb#59021.

Release note (bug fix): Fixed a bug where `DROP SCHEMA ... CASCADE` could
result in types which are referenced being dropped.
ajwerner added a commit to ajwerner/cockroach that referenced this issue Feb 1, 2021
@ajwerner
sql: prevent DROP SCHEMA CASCADE from droping types with references
4ba58dc 
Before this patch, a DROP SCHEMA CASCADE could cause database corruption
by dropping types which were referenced by other tables. This would lead to
bad errors like:

```
ERROR: object already exists: desc 687: type ID 685 in descriptor not found: descriptor not found
SQLSTATE: 42710
```

And doctor errors like:
```
   Table 687: ParentID  50, ParentSchemaID 29, Name 't': type ID 685 in descriptor not found: descriptor not found
```

Fixes cockroachdb#59021.

Release note (bug fix): Fixed a bug where `DROP SCHEMA ... CASCADE` could
result in types which are referenced being dropped.
craig bot pushed a commit that referenced this issue Feb 1, 2021
@itsbilal @thoszhang
@HonoreDB @ajwerner @aayushshah15
Merge #58436 #58906 #59258 #59281 #59591 #59659
f37712a 
58436: server: add /api/v2/ tree with auth/pagination, port listSessions endpoint r=itsbilal a=itsbilal

This change adds the skeleton for a new API tree that lives in
`/api/v2/` in the http listener, and currently reimplements the `/sessions/`
endpoint that is also implemented in `/_status/`. The new v2 API tree avoids
the need to use GRPC Gateway, as well as cookie-based authentication which is
less intuitive and idiomatic for REST APIs. Instead, for authentication,
it uses a new session header that needs to be set on every request.

As many RPC fan-out APIs use statusServer.iterateNodes, this change
implements a pagination-aware method, paginatedIterateNodes, that
works on a sorted set of node IDs and arranges results in such a way
to be able to return the next `limit` results of an arbitary slice
after the `next` cursor passed in. An example of how this works in practice
is the new `/api/v2/sessions/` endpoint.

A dependency on gorilla/mux is added to be able to pattern-match
arguments in the URL. This was already an indirect dependency; now it's
a direct dependency of cockroach.

TODO that are likely to fall over into future PRs:
 - API Documentation, need to explore using swagger here.
 - Porting over remaining /_admin/ and /_status/ APIs, incl. SQL based ones

Part of #55947.

Release note (api change): Adds a new API tree, in /api/v2/*, currently
undocumented, that avoids the use of and cookie-based
authentication in favour of sessions in headers, and support for pagination.

58906: sql: improve interface for injecting descriptors into internal executor r=lucy-zhang a=lucy-zhang

As part of validating new schema elements (unique indexes, constraints)
in the schema changer, we inject in-memory descriptors to be used by the
internal executor that are never written to disk.

This is currently done by creating an entirely new dummy
`descs.Collection` and adding the injected descriptors as uncommitted
descriptors, and then setting this collection as the `tcModifier` on the
internal executor.  Then all subsequent queries using the internal
executor, which each get their own child `connExecutor` and
`descs.Collection`, will have their collection's uncommitted descriptors
replaced by the ones in the dummy collection.

This commit introduces the concept of a "synthetic descriptor" to refer
to these injected descriptors, and slightly improves the interface in
two ways:

1. Instead of creating a new `descs.Collection` to hold synthetic
   descriptors to copy, we now just use a slice of
   `catalog.Descriptor`s.
2. Synthetic descriptors are now made explicit in the `descs.Collection`
   and precede uncommitted descriptors when resolving immutable
   descriptors. Resolving these descriptors mutably is now illegal and
   will return an assertion error.

This commit doesn't change the fact that the synthetic descriptors to be
injected into each query/statement's child `descs.Collection` are set
on the internal executor itself. This is still not threadsafe. It would
make more sense for these descriptors to be scoped at the level of
individual queries.

Related to #34304.

Release note: None

59258: changefeedccl: Freeze table name to the (optionally fully qualified) statement time name r=[miretskiy] a=HonoreDB

Previously, Kafka topics and top-level keys were always derived from the
table name in changefeeds. If the table name changed, the feed
eventually failed, and if the table name was non-unique across
databases, collisions were unavoidable. This PR adds a WITH
full_table_name option to changefeeds, and honors it by serializing
movr.public.drivers as the statement time name and relying on that.

There are probably more things that need to change downstream.

Release note (sql change): Added "WITH full_table_name" option to create
a changefeed on "movr.public.drivers" instead of
"drivers".

59281: sql: prevent DROP SCHEMA CASCADE from droping types with references r=ajwerner a=ajwerner

Before this patch, a DROP SCHEMA CASCADE could cause database corruption
by dropping types which were referenced by other tables. This would lead to
bad errors like:

```
ERROR: object already exists: desc 687: type ID 685 in descriptor not found: descriptor not found
SQLSTATE: 42710
```

And doctor errors like:
```
   Table 687: ParentID  50, ParentSchemaID 29, Name 't': type ID 685 in descriptor not found: descriptor not found
```

Fixes #59021.

Release note (bug fix): Fixed a bug where `DROP SCHEMA ... CASCADE` could
result in types which are referenced being dropped.

59591: sql: hook up multi-region DDL to new zone config attributes r=aayushshah15 a=aayushshah15

After the introduction of #57184, we can constrain voting replicas
separately from non-voting replicas using the new attributes
`num_voters` and `voter_constraints`. This commit connects our
multi-region DDL statements to leverage these new attributes.

Broadly speaking,
- The existing `constraints` and `num_replicas` fields are set at the
database level, to be inherited by table/partition level zone configs.
- The new attributes: `num_voters` and `voter_constraints` (along with
accompanying `lease_preferences` for these voters) are set at the
table/partition level.

This brings the implementation of these DDL statements inline with the
functional specfication.

Fixes #57663

Release note: None


59659: sql: fix bug preventing adding FKs referencing hidden columns r=lucy-zhang a=lucy-zhang

The validation query for adding foreign keys had a pointless `SELECT *`
on the referenced table that caused hidden columns to be omitted,
so attempting to add foreign key constraints referencing hidden columns
would fail. This PR fixes the query.

Fixes #59582.

Release note (bug fix): Fixed a bug preventing foreign key constraints
referencing hidden columns (e.g., `rowid`) from being added.

Co-authored-by: Bilal Akhtar <[email protected]>
Co-authored-by: Lucy Zhang <[email protected]>
Co-authored-by: Aaron Zinger <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Aayush Shah <[email protected]>
@craig craig bot closed this as completed in c0025b6 Feb 1, 2021
@ajwerner ajwerner mentioned this issue Mar 1, 2021
Merged
ajwerner added a commit to ajwerner/cockroach that referenced this issue Mar 1, 2021
@ajwerner
sql: prevent DROP SCHEMA CASCADE from droping types with references
a3c3180 
Before this patch, a DROP SCHEMA CASCADE could cause database corruption
by dropping types which were referenced by other tables. This would lead to
bad errors like:

```
ERROR: object already exists: desc 687: type ID 685 in descriptor not found: descriptor not found
SQLSTATE: 42710
```

And doctor errors like:
```
   Table 687: ParentID  50, ParentSchemaID 29, Name 't': type ID 685 in descriptor not found: descriptor not found
```

Fixes cockroachdb#59021.

Release note (bug fix): Fixed a bug where `DROP SCHEMA ... CASCADE` could
result in types which are referenced being dropped.
@ajwerner ajwerner reopened this Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: prevent DROP SCHEMA CASCADE from droping types with references ajwerner/cockroach
3 participants
@jordanlewis @ajwerner @arulajmani