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: DROP DATABASE/SCHEMA CASCADE may break sequences that have cross schema references #51889

Closed
arulajmani opened this issue Jul 24, 2020 · 3 comments · Fixed by #60744
Closed
Assignees
Labels
A-sql-sequences Sequence handling in SQL 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 doesn't implement DROP SEQUENCE CASCADE. We simply disallow dropping sequences that have usages by tables when a user tries to drop a sequence. This check is not performed during a DROP DATABASE though -- so if a sequence is used by a table in a different database, we end up having dropped a sequence that a table still relies on. This leads to a corrupted default expression.

To Reproduce

What did you do? Describe in your own words.

[email protected]:53338/movr> CREATE DATABASE db;
CREATE DATABASE

Time: 2.743ms

[email protected]:53338/movr> CREATE SEQUENCE db.seq;
CREATE SEQUENCE

Time: 3.191ms


[email protected]:53338/movr> DROP DATABASE db CASCADE;
DROP DATABASE

Time: 45.877ms

[email protected]:53338/movr> INSERT INTO t(b) VALUES(2);
ERROR: nextval(): relation "db.seq" does not exist
SQLSTATE: 42P01
[email protected]:53338/movr>

Expected behavior
There shouldn't be a default expression after the database is dropped. Possible solutions:

@arulajmani arulajmani added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-sequences Sequence handling in SQL labels Jul 24, 2020
@arulajmani arulajmani changed the title sql: dropping a database which contains a sequence used by a different database's table's default expr results in bad behavior sql: DROP DATABASE/SCHEMA CASCADE may break sequences that have cross schema references Jan 14, 2021
@vy-ton
Copy link
Contributor

vy-ton commented Feb 5, 2021

Should we do the same as #59020 here to prevent this problematic state?

@solongordon or @jordanlewis for 21.1 prioritization

@ajwerner
Copy link
Contributor

ajwerner commented Feb 5, 2021

We should definitely do something. I think that @the-ericwang35's work should fix this for new sequences but it won't fix old ones.

@ajwerner
Copy link
Contributor

ajwerner commented Feb 9, 2021

Postgres drops the default value from the column that depends on it. We could do that. Lower priority.

craig bot pushed a commit that referenced this issue Feb 26, 2021
60744: sql: drop default value when its dependent sequence is dropped r=the-ericwang35 a=the-ericwang35

Fixes #51889 and #20965.

Previously, we did not support `DROP SEQUENCE ... CASCADE`,
and we simply disallowed dropping sequences that have usages by 
tables when a user tries to drop a sequence.

However,  we did not make this check when doing 
`DROP DATABASE/SCHEMA ... CASCADE`. As a result,
if a sequence is used by a table in a different database/schema,
we end up dropping a sequence that the table still relies on,
resulting in a corrupted default expression.

This patch addresses this issue by implementing 
`DROP SCHEMA ... CASCADE`, which solves the above issue.
When we drop anything that contains/owns a sequence with
`CASCADE`, we drop those sequences as if 
`DROP SCHEMA ... CASCADE` was called on them, which
prevents the `DEFAULT` expressions from being corrupted, 
and also allows users to use `DROP SEQUENCE ... CASCADE.

`DROP SEQUENCE ... CASCADE` behaves as in Postgres, 
wherein any default expressions that rely on the dropped 
sequences are also dropped.

Release note (bug fix): Fixed a bug which could result in corrupted descriptors when dropping a database which contains a sequence that was referenced from another database.

Release note (bug fix): Fixed a bug where DROP SEQUENCE CASCADE would not properly remove references to itself from table default expressions.

Release note (sql change): Added support for dropping a table which owns a sequence. 

Release justification: low risk bug fix for existing functionality

60952: server: Migrate nodes, {hot-,}ranges, health endpoints to v2 API r=knz a=itsbilal

This change adds API v2 compatible versions of the node list, range info
per node, ranges info across nodes, hot ranges, and health endpoints.
These endpoints all support API v2 header-based authentication,
pagination (if applicable), and only return relevant information
in the response payloads.

One part of #55947.

Release note (api change): Add these new HTTP API endpoints:
 - `/api/v2/nodes/`: Lists all nodes in the cluster
 - `/api/v2/nodes/<node_id>/ranges`: Lists all ranges on the specified node
 - `/api/v2/ranges/hot/`: Lists hot ranges in the cluster
 - `/api/v2/ranges/<range_id>/`: Describes range in more detail
 - `/api/v2/health/`: Returns an HTTP 200 response if node is healthy.

Release justification: Adds more HTTP API endpoints in parallel
 that do not touch existing code.

Co-authored-by: Eric Wang <[email protected]>
Co-authored-by: Bilal Akhtar <[email protected]>
@craig craig bot closed this as completed in c02dd6f Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-sequences Sequence handling in SQL C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants