sql: drop types as part of a DROP DATABASE statement#51362
sql: drop types as part of a DROP DATABASE statement#51362craig[bot] merged 1 commit intocockroachdb:masterfrom
DROP DATABASE statement#51362Conversation
|
This change is |
e1b7dda to
8443cf5
Compare
|
Take a look at the second commit please! |
e74341a to
a0b7e1e
Compare
642c56e to
f72ec2b
Compare
|
I've rebased this and it's ready for review |
|
pinging @ajwerner and @lucy-zhang for review. |
thoszhang
left a comment
There was a problem hiding this comment.
Reviewed 3 of 10 files at r2, 21 of 21 files at r3.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rohany)
pkg/sql/drop_table.go, line 324 at r3 (raw file):
// queued. If not, then the job that is handling the drop table will also // clean up all of the types to be dropped. if queueJob {
This parameter currently just indicates whether the object is being dropped as part of DROP DATABASE CASCADE, right? Maybe we should rename it accordingly. It's a little strange to have other behavior depend on the queueJob parameter.
pkg/sql/schema_changer.go, line 1793 at r3 (raw file):
// multiple tables in a single job (e.g., TRUNCATE on multiple tables), so // it's possible for DroppedDatabaseID to be unset. if details.DroppedDatabaseID != sqlbase.InvalidID || len(details.DroppedTables) > 1 || len(details.DroppedTypes) > 0 {
When would DroppedTypes be non-nil when DroppedDatabaseID is unset?
pkg/sql/logictest/testdata/logic_test/drop_type, line 1 at r3 (raw file):
# LogicTest: !3node-tenant
Why the configuration change? Does the current incoherent caching of databases make DROP DATABASE in logic tests in multi-node configurations unreliable or something?
pkg/sql/logictest/testdata/logic_test/drop_type, line 295 at r3 (raw file):
DROP DATABASE d RESTRICT let $t_id
Interesting, I didn't know we supported this
f72ec2b to
d033b25
Compare
rohany
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/drop_table.go, line 324 at r3 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
This parameter currently just indicates whether the object is being dropped as part of
DROP DATABASE CASCADE, right? Maybe we should rename it accordingly. It's a little strange to have other behavior depend on thequeueJobparameter.
Good point, done.
pkg/sql/schema_changer.go, line 1793 at r3 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
When would
DroppedTypesbe non-nil whenDroppedDatabaseIDis unset?
Yeah, I don't think there is a case for that.
pkg/sql/logictest/testdata/logic_test/drop_type, line 1 at r3 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
Why the configuration change? Does the current incoherent caching of databases make DROP DATABASE in logic tests in multi-node configurations unreliable or something?
There's some known issues with DROP DATABASE not working in the multitenant config due to some errors with range deletes.
Fixes cockroachdb#49940. This commit adds logic to drop all types as part of a `DROP DATABASE` cascade. Release note: None
d033b25 to
b409508
Compare
thoszhang
left a comment
There was a problem hiding this comment.
Reviewed 14 of 14 files at r4.
Reviewable status:
|
bors r+ |
|
Build succeeded: |
Fixes #49940.
This commit adds logic to drop all types as part of a
DROP DATABASEcascade.
Release note: None