sql/catalog: ensure consistent versions during type hydration#162622
sql/catalog: ensure consistent versions during type hydration#162622trunk-io[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
😎 Merged successfully - details. |
|
This change is |
| // with concurrent schema changes never encounter errors. | ||
| func TestLeasedDescriptorTypeHydration(t *testing.T) { | ||
| defer leaktest.AfterTest(t)() | ||
| defer log.Scope(t).Close(t) |
There was a problem hiding this comment.
nice test! there's a lot of concurrency so just want to to double check it's stable under stress/race/deadlock?
There was a problem hiding this comment.
Done.
I ran this on engflow under stress as validation.
pkg/sql/catalog/descs/hydrate.go
Outdated
| // Note: This is a special case because type hydration cannot tolerate | ||
| // dropped descriptors. This already could occur by leasing any schema | ||
| // with type references that are being dropped. | ||
| if !witoutDropped && desc.Dropped() { |
There was a problem hiding this comment.
should it be if !witoutDropped && !withoutLeased && desc.Dropped()
There was a problem hiding this comment.
Done.
Yeah that would make it more future proof.
13a3178 to
c561f91
Compare
fqazi
left a comment
There was a problem hiding this comment.
@fqazi made 2 comments.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rafiss).
pkg/sql/catalog/descs/hydrate.go
Outdated
| // Note: This is a special case because type hydration cannot tolerate | ||
| // dropped descriptors. This already could occur by leasing any schema | ||
| // with type references that are being dropped. | ||
| if !witoutDropped && desc.Dropped() { |
There was a problem hiding this comment.
Done.
Yeah that would make it more future proof.
| // with concurrent schema changes never encounter errors. | ||
| func TestLeasedDescriptorTypeHydration(t *testing.T) { | ||
| defer leaktest.AfterTest(t)() | ||
| defer log.Scope(t).Close(t) |
There was a problem hiding this comment.
Done.
I ran this on engflow under stress as validation.
Previously, if a schema had references to types being dropped, for example due to functions that were being cleaned up, it was possible to encountered "descriptor is being dropped" when leasing the schema. This happened because the version of the schema could be behind that of other objects referenced. While locked leasing eliminates issues for non-dropped objects, it cannot alleviate it for any dropped descriptors that are being resolved. To address this, this patch will generate a retryable error, if we detect that a dropped type descriptor encountered after the read timestamp. Fixes: cockroachdb#159509 Release note: None
c561f91 to
5353e64
Compare
|
/trunk merge |
|
blathers backport 26.1 |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 5353e64 to blathers/backport-release-26.1-162622: POST https://api.github.com/repos/fqazi/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 26.1 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Previously, if a schema had references to types being dropped, for example due to functions that were being cleaned up, it was possible to encountered "descriptor is being dropped" when leasing the schema. This happened because the version of the schema could be behind that of other objects referenced. While locked leasing eliminates issues for non-dropped objects, it cannot alleviate it for any dropped descriptors that are being resolved. To address this, this patch will generate a retryable error, if we detect that a dropped type descriptor encountered after the read timestamp.
Fixes: #159509
Release note: None