sql: correctly identify and cleanup dependent objects in drop database #51813
Conversation
|
This change is |
e2e92d6 to
5843f85
Compare
There was a problem hiding this comment.
I like this change, but it's a bit far of my expertise by now. I would defer to @lucy-zhang for the technical review.
-
I am surprised that the code is not shared with what's implemented at the bottom of
drop_table.go. I was expecting that the analysis of table dependencies be the same when a table drop comes from a database drop, or when it's listed explicitly. -
A random nit would be that the logic for analyzing the dependencies recursively would be best moved to a new Go source file (
ownership.goor something?) since it's common to multiple operations.
5843f85 to
3399f46
Compare
There was a problem hiding this comment.
I am surprised that the code is not shared with what's implemented at the bottom of drop_table.go. I was expecting that the analysis of table dependencies be the same when a table drop comes from a database drop, or when it's listed explicitly.
It's sort of interesting, this code is accounting for logic that already exists, implicitly, somewhere else. I do suspect that this points to a need to refactor. It might be great if the drop table logic were explicit about the resources it was going to delete and could share this code. That'd make the possibility of future bugs in this space less common. We'll probably also, eventually, want to extend this to types. However, for this release, we're not implementing DROP TYPE CASCADE so it doesn't matter as much.
What do you think about shouldering such a refactor. I'm relatively interested in the idea of backporting the change in this form with an eye towards taking on the larger refactor to unify the ownership tracking in a follow-up.
Reviewed 3 of 4 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, and @lucy-zhang)
pkg/sql/drop_database.go, line 315 at r1 (raw file):
// exist. if errors.Is(err, sqlbase.ErrDescriptorNotFound) { log.Eventf(ctx,
This feels rare enough that I'd want at least log.Infof
pkg/sql/drop_database.go, line 317 at r1 (raw file):
Quoted 17 lines of code…
for colID := range toDel.desc.GetColumns() { for _, seqID := range toDel.desc.GetColumns()[colID].OwnsSequenceIds { ownedSeqDesc, err := p.Descriptors().GetMutableTableVersionByID(ctx, seqID, p.txn) if err != nil { // Special case error swallowing for #50711 and #50781, which can // cause columns to own sequences that have been dropped/do not // exist. if errors.Is(err, sqlbase.ErrDescriptorNotFound) { log.Eventf(ctx, "swallowing error for owned sequence that was not found %s", err.Error()) continue } return nil, nil, err } implicitDeleteObjects[seqID] = ownedSeqDesc } }
nit: This feels like it could be a helper function.
pkg/sql/sequence.go, line 533 at r1 (raw file):
return err } // If the sequence descriptor has been dropped, we do not need to unlink the
How does this happen?
pkg/sql/logictest/testdata/logic_test/drop_database, line 282 at r1 (raw file):
0 # Similar to the test above, except now the relationship is cross database. In
Why do we even support having a table own a sequence in a different database (or schema for that matter). Postgres doesn't:
postgres=# ALTER SEQUENCE baz.asdf OWNED BY foo.t.i
ERROR: sequence must be in same schema as table it is linked to
I'd love to see a PR that prevented it moving forward. This is a thing we should put into a consistency checker eventually. I think we'll want to have an easy way to determine whether there are cross-database relationships that we want to kill.
3399f46 to
2a48e40
Compare
There was a problem hiding this comment.
I am surprised that the code is not shared with what's implemented at the bottom of drop_table.go.
Are you referring to removeMatchingReferences so that we don't have to worry about this filtration step here? I do agree there is a need to unify this stuff. If you could expand what you have in mind it might be helpful for me to get an idea for what direction you see this stuff going.
I support the idea of having this backported in its current form and then come back to refactor this to prevent bugs like this in the future. I'll open up a follow-up issue for this.
What do you think about shouldering such a refactor.
I'd love to. I have a few ideas of how this could be made better w.r.t sequences/views that we currently have, but I haven't spent enough time thinking about types. Let me open up a followup issue and write down some thoughts there. It would be nice to get some feedback there and I can take it from there.
Reviewable status:
pkg/sql/drop_database.go, line 317 at r1 (raw file):
Previously, ajwerner wrote…
for colID := range toDel.desc.GetColumns() { for _, seqID := range toDel.desc.GetColumns()[colID].OwnsSequenceIds { ownedSeqDesc, err := p.Descriptors().GetMutableTableVersionByID(ctx, seqID, p.txn) if err != nil { // Special case error swallowing for #50711 and #50781, which can // cause columns to own sequences that have been dropped/do not // exist. if errors.Is(err, sqlbase.ErrDescriptorNotFound) { log.Eventf(ctx, "swallowing error for owned sequence that was not found %s", err.Error()) continue } return nil, nil, err } implicitDeleteObjects[seqID] = ownedSeqDesc } }nit: This feels like it could be a helper function.
Done
pkg/sql/sequence.go, line 533 at r1 (raw file):
Previously, ajwerner wrote…
How does this happen?
If a sequence is used by a table in the database, and the dropImpl on the table is processed before the sequence. This is one of the cases where dependedOnBy works differently for sequences and views -- even though a table is dependedOnBy a sequence, the sequence isn't automatically dropped when the table is dropped. Before my change, we were erroneously filtering the sequence out from the list of objects, so the scenario I described above couldn't arise, and this wasn't possible.
Thanks for drawing my attention to this though, it prompted me to add a test where the dropImpl on the sequence is called before the table, which we were missing earlier.
pkg/sql/logictest/testdata/logic_test/drop_database, line 282 at r1 (raw file):
Previously, ajwerner wrote…
Why do we even support having a table own a sequence in a different database (or schema for that matter). Postgres doesn't:
postgres=# ALTER SEQUENCE baz.asdf OWNED BY foo.t.i ERROR: sequence must be in same schema as table it is linked toI'd love to see a PR that prevented it moving forward. This is a thing we should put into a consistency checker eventually. I think we'll want to have an easy way to determine whether there are cross-database relationships that we want to kill.
The relationship here isn't an ownership relationship -- it's just a "uses" relationship. Postgres doesn't allow cross database sequence usage, but it does seem to allow cross schema sequence use. I do agree it would be nice to have a PR that prevents this though.
One thing that I want to think of here is how this interacts with SERIAL and how we decide to solve #51090. I wanna think about this a bit more, but I'll create an issue for this (as well as disallowing cross-database sequence ownership, which is also a separate problem).
There was a problem hiding this comment.
I'm good on this if you add a bit more commentary in some spots and fix the linter problems.
Reviewable status:
pkg/sql/drop_database.go, line 326 at r2 (raw file):
} func (p *planner) accumulateOwnedSequences(
nit: comment
pkg/sql/sequence.go, line 533 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
If a sequence is used by a table in the database, and the dropImpl on the table is processed before the sequence. This is one of the cases where
dependedOnByworks differently for sequences and views -- even though a table isdependedOnBya sequence, the sequence isn't automatically dropped when the table is dropped. Before my change, we were erroneously filtering the sequence out from the list of objects, so the scenario I described above couldn't arise, and this wasn't possible.Thanks for drawing my attention to this though, it prompted me to add a test where the dropImpl on the sequence is called before the table, which we were missing earlier.
Can you add more commentary about this here.
pkg/sql/logictest/testdata/logic_test/drop_database, line 285 at r2 (raw file):
query I SELECT COUNT(*) FROM system.namespace WHERE name = 'seq50997' ----
the linter is complaining about the COUNT needing to be count
2a48e40 to
1c7761d
Compare
There was a problem hiding this comment.
TFTR! Haven't finished writing the follow up issue with suggestions on how to refactor this going forward yet, but will have that done soon.
Reviewable status:
pkg/sql/sequence.go, line 533 at r1 (raw file):
Previously, ajwerner wrote…
Can you add more commentary about this here.
Added a comment. You might find it interesting that this wouldn't have happened if we implement DROP CASCADES for sequences. When a sequence is dropped explicitly, we error out if the sequence has dependencies. During a DROP DATABASE we skip that check and therefore drop a sequence descriptor which still has active dependencies.
In the ideal world, if DROP DATABASE CASCADE calls drop on the sequence before the table, the sequence would be dropped as a CASCADE and therefore unlink itself from the table descriptor and we wouldn't need this ugly check. This also manifests itself in #51889 that I just filed.
pkg/sql/logictest/testdata/logic_test/drop_database, line 285 at r2 (raw file):
Previously, ajwerner wrote…
the linter is complaining about the
COUNTneeding to becount
🤦
There were a few things going on here because of the way we filtered tables during `DROP DATABASE CASCADE`. The intention was to filter out objects that depended on other objects also being deleted (and would therefore be deleted by the CASCADE nature of object drops). This assumption is correct for table-view and view-view dependencies -- but not for sequences. We also switched from individual schema change jobs during a database drop to a single job which doesn't play well with this filtering -- as no new jobs are queued, all objects that were filtered would leave orphaned namespace/descriptor entries behind. It's interesting to note that the filtering didn't directly cause this issue, it just made the underlying issue more visible -- the single drop database schema change job relies on knowing about all object descriptors that need to be dropped upfront. Orphaned entries which would have only occured for cross-database dependencies can occur in the same database because of the filtering. This leads to why the fix is what it is. As part of this patch, I've tried to make the preperation steps for dropping databases more explicit. First, we accumalate all objects that need to be deleted. This includes objects that will be implicitly deleted, such as owned sequences, dependent views (both in the database being deleted and other databases that aren't being deleted). The schema change job uses this new list to ensure entries are appropriately cleaned up. We still perform the filter step as before to identify objects which are the "root" of a drop and only call drop on these objects. The only change here is that instead of accumulating dependent objects, we explicitly accumulate cascading views. Fixes cockroachdb#51782 Fixes cockroachdb#50997 Release note (bug fix): Before this change, we would leave orphaned system.namespace/system.descriptor entries if we ran a `DROP DATABASE CASCADE` and the database contained "dependency" relations. For example, if the database included a view which depended on a table in the database, dropping the database would result in an orphaned entry for the view. Same thing for a sequence that was used by a table in the database. (See cockroachdb#51782 for repro steps). This bug is now fixed, and cleanup happens as expected.
1c7761d to
5063b61
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/drop_database, line 282 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
The relationship here isn't an ownership relationship -- it's just a "uses" relationship. Postgres doesn't allow cross database sequence usage, but it does seem to allow cross schema sequence use. I do agree it would be nice to have a PR that prevents this though.
One thing that I want to think of here is how this interacts with SERIAL and how we decide to solve #51090. I wanna think about this a bit more, but I'll create an issue for this (as well as disallowing cross-database sequence ownership, which is also a separate problem).
Ack.
|
Err I'm not sure why CI is failing, it doesn't seem to be referring to any actual test. I wonder if it's the pebble/kv flake we saw earlier in the week, so I'll try to bors. Thanks for helping push this through with a quick turnaround in reviews @ajwerner bors r=ajwerner |
|
Build succeeded: |
There were a few things going on here because of the way we filtered
tables during
DROP DATABASE CASCADE. The intention was to filterout objects that depended on other objects also being deleted (and would
therefore be deleted by the CASCADE nature of object drops). This
assumption is correct for table-view and view-view dependencies -- but
not for sequences. We also switched from individual schema change jobs
during a database drop to a single job which doesn't play well with
this filtering -- as no new jobs are queued, all objects that were
filtered would leave orphaned namespace/descriptor entries behind.
It's interesting to note that the filtering didn't directly cause this
issue, it just made the underlying issue more visible -- the single drop
database schema change job relies on knowing about all object
descriptors that need to be dropped upfront. Orphaned entries which
would have only occured for cross-database dependencies can occur in
the same database because of the filtering. This leads to why the fix
is what it is.
As part of this patch, I've tried to make the preperation steps for
dropping databases more explicit. First, we accumalate all objects
that need to be deleted. This includes objects that will be implicitly
deleted, such as owned sequences, dependent views (both in the database
being deleted and other databases that aren't being deleted). The
schema change job uses this new list to ensure entries are appropriately
cleaned up. We still perform the filter step as before to identify
objects which are the "root" of a drop and only call drop on these
objects. The only change here is that instead of accumulating dependent
objects, we explicitly accumulate cascading views.
Fixes #51782
Fixes #50997
Release note (bug fix): Before this change, we would leave orphaned
system.namespace/system.descriptor entries if we ran a
DROP DATABASE CASCADEand the database contained "dependency"relations. For example, if the database included a view which
depended on a table in the database, dropping the database would result
in an orphaned entry for the view. Same thing for a sequence that was
used by a table in the database. (See #51782 for repro steps). This bug
is now fixed, and cleanup happens as expected.