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 view cascade does not handle dependencies being dropped earlier #68600

Closed
fqazi opened this issue Aug 9, 2021 · 0 comments · Fixed by #68601
Closed

sql: drop view cascade does not handle dependencies being dropped earlier #68600

fqazi opened this issue Aug 9, 2021 · 0 comments · Fixed by #68601
Assignees
@fqazi
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@fqazi
Copy link
Collaborator

fqazi commented Aug 9, 2021

When dropping views in a cascaded fashion the current code does not properly deal with
any dependency being dropped earlier. For example in the example below:

CREATE TABLE t1cycle (id INT PRIMARY KEY, name varchar(256))
CREATE VIEW v1cycle AS (SELECT name FROM t1cycle)
CREATE VIEW v2cycle AS (SELECT name AS n1, name AS n2 FROM v1cycle)
CREATE VIEW v3cycle AS (SELECT name, n1 FROM v2cycle, v1cycle);
DROP table t1cycle CASCADE

The drop table will fail on v1cycle since it was dropped earlier when cleaning up v2cycle.
@fqazi fqazi added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Aug 9, 2021
@fqazi fqazi self-assigned this Aug 9, 2021
fqazi added a commit to fqazi/cockroach that referenced this issue Aug 9, 2021
@fqazi
sql: drop view cascade does not handle deps being dropped earlier
5d1674f 
Fixes: cockroachdb#68600

Previously, drop view cascade would only check if a dependency
was dropped when initially going over the list. It did not check
before executing the drop logic, so an earlier dependency could
cause the current object to move to a dropped state. This was
inadequate because cascaded drops of views could fail due to this
reason. To address this, this patch skips dropping dependencies
if at a later stage they are found to be dropped.

Release note (bug fix): Cascaded drop of views could run into
'table ...is already being dropped' errors incorrectly.
@fqazi fqazi mentioned this issue Aug 9, 2021
Merged
craig bot pushed a commit that referenced this issue Aug 9, 2021
@dhartunian @nvanbenschoten @fqazi
Merge #66675 #68497 #68601
764a1a9 
66675: server: pod-to-pod fanout for statements api on tenant  r=dhartunian a=dhartunian

Previously, the Statements endpoint implementation was purely local in
its execution and reported only in-memory data on the current tenant.
This change initializes a gRPC and gRPC-gateway server on the tenant,
much like we do on the hosts already, and uses the newly added
InstanceID subsystem for tenants to implement a gRPC-based fanout for
the Statements endpoint implementation.

The fanout is done much in the same manner as on hosts and we
expose the status server endpoints via HTTP on the tenant as well.

This change is necessary to support serverless observability features
and provide our UIs access to the Statements endpoint. Future work may
move this API to SQL-only

Resolves #64477

Release note (api change): tenant pods now expose the Statements API at
`/_status/statements` on their HTTP port.

68497: kv: don't load applied index twice during Raft snapshot r=nvanbenschoten a=nvanbenschoten

This was harmless, but it was also unnecessary and looked quite
alarming. It would be a major problem if the Raft applied index stored
in a `SnapshotMetadata` was ever incoherent with the applied state in
the snapshot. However, we're reading from a consistent enging snapshot
in `snapshot`, so the Raft applied index can't change. Still, better to
avoid questions.

The "members of the Range struct" comment was very stale, dating back
to acc41dd.

68601: sql: drop view cascade does not handle deps being dropped earlier r=ajwerner a=fqazi

Fixes: #68600

Previously, drop view cascade would only check if a dependency
was dropped when initially going over the list. It did not check
before executing the drop logic, so an earlier dependency could
cause the current object to move to a dropped state. This was
inadequate because cascaded drops of views could fail due to this
reason. To address this, this patch skips dropping dependencies
if at a later stage they are found to be dropped.

Release note (bug fix): Cascaded drop of views could run into
'table ...is already being dropped' errors incorrectly.

Co-authored-by: David Hartunian <davidh@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>
@craig craig bot closed this as completed in ab28823 Aug 9, 2021
This was referenced Aug 9, 2021
Merged
Merged
sajjadrizvi pushed a commit to sajjadrizvi/cockroach that referenced this issue Aug 10, 2021
@fqazi
sql: drop view cascade does not handle deps being dropped earlier
99cb425 
Fixes: cockroachdb#68600

Previously, drop view cascade would only check if a dependency
was dropped when initially going over the list. It did not check
before executing the drop logic, so an earlier dependency could
cause the current object to move to a dropped state. This was
inadequate because cascaded drops of views could fail due to this
reason. To address this, this patch skips dropping dependencies
if at a later stage they are found to be dropped.

Release note (bug fix): Cascaded drop of views could run into
'table ...is already being dropped' errors incorrectly.
fqazi added a commit to fqazi/cockroach that referenced this issue Aug 23, 2021
@fqazi
sql: drop view cascade does not handle deps being dropped earlier
aa58446 
Fixes: cockroachdb#68600

Previously, drop view cascade would only check if a dependency
was dropped when initially going over the list. It did not check
before executing the drop logic, so an earlier dependency could
cause the current object to move to a dropped state. This was
inadequate because cascaded drops of views could fail due to this
reason. To address this, this patch skips dropping dependencies
if at a later stage they are found to be dropped.

Release justification: low risk fix that addresses incorrect drop view
behaviour

Release note (bug fix): Cascaded drop of views could run into
'table ...is already being dropped' errors incorrectly.
fqazi added a commit to fqazi/cockroach that referenced this issue Aug 24, 2021
@fqazi
sql: drop view cascade does not handle deps being dropped earlier
218461d 
Fixes: cockroachdb#68600

Previously, drop view cascade would only check if a dependency
was dropped when initially going over the list. It did not check
before executing the drop logic, so an earlier dependency could
cause the current object to move to a dropped state. This was
inadequate because cascaded drops of views could fail due to this
reason. To address this, this patch skips dropping dependencies
if at a later stage they are found to be dropped.

Release justification: low risk fix that addresses incorrect drop view
behaviour

Release note (bug fix): Cascaded drop of views could run into
'table ...is already being dropped' errors incorrectly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fqazi fqazi

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: drop view cascade does not handle deps being dropped earlier fqazi/cockroach
1 participant
@fqazi