sql: prevent DROP SCHEMA CASCADE from droping types with references #59281

ajwerner · 2021-01-22T03:43:47Z

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.

cockroach-teamcity · 2021-01-22T03:43:55Z

This change is

ajwerner · 2021-01-22T17:03:56Z

This doesn't quite work for subtle reasons. We need to filter out any dependencies which are already dropped.

arulajmani

Change looks good. Maybe I'm missing something here, but if the dependency is already dropped, then wouldn't that mean the reference is removed from the type descriptor as well?

Reviewable status: complete! 0 of 0 LGTMs obtained

ajwerner

The root of the problem is in

cockroach/pkg/sql/drop_table.go

Lines 360 to 367 in a106e2f

    
           // Remove any references to types that this table has if a job is meant to be 
        
           // queued. If not, then the job that is handling the drop table will also 
        
           // clean up all of the types to be dropped. 
        
           if !droppingParent { 
        
           	if err := p.removeBackRefsFromAllTypesInTable(ctx, tableDesc); err != nil { 
        
           		return droppedViews, err 
        
           	} 
        
           }

It turns out this thing is actually totally bogus.

CREATE SCHEMA sc1;
CREATE SCHEMA sc2;
CREATE TYPE sc2.typ AS ENUM ('a');
CREATE TABLE sc1.tab (k sc2.typ);
DROP SCHEMA sc1 CASCADE;
DROP TYPE sc2.typ;

ERROR: type "typ" has dependent objects: descriptor not found

😬

I'll add another commit.

Reviewable status: complete! 0 of 0 LGTMs obtained

ajwerner · 2021-01-28T15:02:13Z

Okay, added another commit to deal with the drop cascade problem. I removes an optimization that occurs during DROP DATABASE CASCADE but I don't think I care. The main reason I don't think I care is that we should optimize it away separately by not writing the descriptors directly during statement execution but rather accumulate them in the descriptor collection and then write them in a batch at the end. The whole idea that we just point update all of the descriptor potentially many times during execution is madness. The optimization there was a half-measure.

thoszhang

Reviewed 2 of 4 files at r1, 1 of 1 files at r2, 2 of 2 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

pkg/sql/resolver.go, line 407 at r3 (raw file):

) (fullyQualifiedNames []*tree.TableName, _ error) {
	for _, id := range ids {
		desc, err := p.Descriptors().GetMutableTableVersionByID(ctx, id, p.txn)

Ideally we'd use GetImmutableTableByID with AvoidCached: true, IncludeDropped: true, IncludeOffline: true here. It is clunky and I do wonder whether we should just include dropped/offline descriptors by default when resolving by ID, but that's a separate topic.

pkg/sql/logictest/testdata/logic_test/drop_type, line 369 at r3 (raw file):

DROP SCHEMA schema_to_drop CASCADE;

nit: extra newline

pkg/sql/logictest/testdata/logic_test/drop_type, line 371 at r3 (raw file):

# We also want to test that it's not a problem when dropping tables which are
# being deleted but show up as a dependency

Can you clarify this comment to say that we're testing that DROP SCHEMA CASCADE correctly removes backreferences on types in a different schema?

pkg/sql/logictest/testdata/logic_test/drop_type, line 385 at r3 (raw file):

Quoted 4 lines of code…

statement ok
DROP TABLE t;
statement ok
DROP SCHEMA schema_to_drop CASCADE;

Don't you want these to go with the previous set of statements?

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang)

pkg/sql/resolver.go, line 407 at r3 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Ideally we'd use GetImmutableTableByID with AvoidCached: true, IncludeDropped: true, IncludeOffline: true here. It is clunky and I do wonder whether we should just include dropped/offline descriptors by default when resolving by ID, but that's a separate topic.

Done.

pkg/sql/logictest/testdata/logic_test/drop_type, line 371 at r3 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

# We also want to test that it's not a problem when dropping tables which are
# being deleted but show up as a dependency
Can you clarify this comment to say that we're testing that DROP SCHEMA CASCADE correctly removes backreferences on types in a different schema?

Done.

pkg/sql/logictest/testdata/logic_test/drop_type, line 385 at r3 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

statement ok
DROP TABLE t;
statement ok
DROP SCHEMA schema_to_drop CASCADE;
Don't you want these to go with the previous set of statements?

very much yes

thoszhang

Reviewed 3 of 3 files at r4, 2 of 2 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

pkg/sql/resolver.go, line 407 at r3 (raw file):

Previously, ajwerner wrote…

Done.

FWIW Required doesn't do anything when looking up descriptors by ID. You'll always get an error if it doesn't exist.

pkg/sql/logictest/testdata/logic_test/drop_type, line 387 at r5 (raw file):


statement ok
DROP SCHEMA sc2 CASCADE;

Does the success of this statement alone indicate that removing the backreferences worked?

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.

Before this commit we'd errantly skip dropping type references under a false assumption that all the types were also being dropped. This could lead to dangling backreferences to types from tables in other schemas. Release note (bug fix): Fixed a bug whereby dropping a schema with a table that used a user-defined type which was not being dropped (because it is in a different schema) would result in a descriptor corruption due to a dangling back-reference to a dropped table on the type descriptor.

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang)

pkg/sql/resolver.go, line 407 at r3 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

FWIW Required doesn't do anything when looking up descriptors by ID. You'll always get an error if it doesn't exist.

Removed.

pkg/sql/logictest/testdata/logic_test/drop_type, line 387 at r5 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Does the success of this statement alone indicate that removing the backreferences worked?

It's actually the the DROP TYPE sc2.typ which indicates that removing the backreferences worked. I split it out and added a comment.

thoszhang

Reviewed 3 of 3 files at r6, 2 of 2 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained

ajwerner · 2021-02-01T19:43:20Z

TFTR!

bors r+

craig · 2021-02-01T21:00:21Z

Build succeeded:

GitHub CI (Cockroach)

ajwerner requested review from arulajmani and a team January 22, 2021 03:43

ajwerner added the backport-20.2.x label Jan 22, 2021

arulajmani approved these changes Jan 22, 2021

View reviewed changes

ajwerner commented Jan 23, 2021

View reviewed changes

ajwerner force-pushed the ajwerner/drop-schema-cascade-types branch from f5a87ce to ae4ddea Compare January 28, 2021 15:00

thoszhang reviewed Jan 29, 2021

View reviewed changes

ajwerner force-pushed the ajwerner/drop-schema-cascade-types branch from ae4ddea to 29b7664 Compare February 1, 2021 07:23

ajwerner commented Feb 1, 2021

View reviewed changes

thoszhang reviewed Feb 1, 2021

View reviewed changes

ajwerner added 2 commits February 1, 2021 11:03

ajwerner force-pushed the ajwerner/drop-schema-cascade-types branch from 29b7664 to 8ba36eb Compare February 1, 2021 16:04

ajwerner commented Feb 1, 2021

View reviewed changes

thoszhang approved these changes Feb 1, 2021

View reviewed changes

craig bot merged commit f37712a into cockroachdb:master Feb 1, 2021

ajwerner mentioned this pull request Mar 1, 2021

release-20.2: sql: prevent DROP SCHEMA CASCADE from droping types with references #61259

Merged

rafiss added this to the 20.2 milestone Apr 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: prevent DROP SCHEMA CASCADE from droping types with references #59281

sql: prevent DROP SCHEMA CASCADE from droping types with references #59281

ajwerner commented Jan 22, 2021

cockroach-teamcity commented Jan 22, 2021

ajwerner commented Jan 22, 2021

arulajmani left a comment

ajwerner left a comment

ajwerner commented Jan 28, 2021

thoszhang left a comment

ajwerner left a comment

thoszhang left a comment

ajwerner left a comment

thoszhang left a comment

ajwerner commented Feb 1, 2021

craig bot commented Feb 1, 2021

	// Remove any references to types that this table has if a job is meant to be
	// queued. If not, then the job that is handling the drop table will also
	// clean up all of the types to be dropped.
	if !droppingParent {
	if err := p.removeBackRefsFromAllTypesInTable(ctx, tableDesc); err != nil {
	return droppedViews, err
	}
	}

sql: prevent DROP SCHEMA CASCADE from droping types with references #59281

sql: prevent DROP SCHEMA CASCADE from droping types with references #59281

Conversation

ajwerner commented Jan 22, 2021

cockroach-teamcity commented Jan 22, 2021

ajwerner commented Jan 22, 2021

arulajmani left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner commented Jan 28, 2021

thoszhang left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

thoszhang left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

thoszhang left a comment

Choose a reason for hiding this comment

ajwerner commented Feb 1, 2021

craig bot commented Feb 1, 2021