catalog: schema descriptor validation reads all functions under a schema#138739
catalog: schema descriptor validation reads all functions under a schema#138739craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
f5e981d to
f95e45e
Compare
rafiss
left a comment
There was a problem hiding this comment.
nice tests! just had a nit and question
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @fqazi)
pkg/sql/catalog/descs/descriptor.go line 651 at r1 (raw file):
} requiredLevel := validate.MutableRead if !flags.isMutable {
nit: let's consolidate the logic by adding and else to the if flags.isMutable above.
pkg/sql/catalog/typedesc/type_desc.go line 554 at r1 (raw file):
// GetReferencedDescIDs returns the IDs of all descriptors referenced by // this descriptor, including itself. func (desc *immutable) GetReferencedDescIDs(
do you think we can tackle some of the issues described in #77140 ? (could be a separate PR)
f95e45e to
2485b35
Compare
fqazi
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/typedesc/type_desc.go line 554 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
do you think we can tackle some of the issues described in #77140 ? (could be a separate PR)
Let me do that as a follow on and apply the constrained look up in more places. I'll pick up the issue.
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/descs/descriptor.go line 637 at r7 (raw file):
) error { // Add the descriptors to the uncommitted layer if we want them to be mutable. if flags.isMutable {
nit: we already have an if flags.isMutable check here; let's reuse this rather than duplication an if
rafiss
left a comment
There was a problem hiding this comment.
Reviewed 10 of 11 files at r1, 3 of 3 files at r6.
Reviewable status:
2485b35 to
e1330db
Compare
fqazi
left a comment
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
e1330db to
d4639d2
Compare
rafiss
left a comment
There was a problem hiding this comment.
Reviewed 1 of 3 files at r8, 1 of 3 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status:
d4639d2 to
6960965
Compare
Previously, when fetching immutable non-leased descriptors we still did back reference validation. This could be problematic for certain CREATE statements, since they fetch non-leased schema descriptors for name resolutions, which in turns fetches every function descriptor. To avoid this, this patch adds a level parameter to GetReferencedDescIDs, which allows the caller to filter out back references. It also update the validation code to only use mutable validation on mutable descriptors. Fixes: cockroachdb#138384 Release note: None
Previously, the logic detect schema changes on schema descriptors during CREATE statements incorrectly fetched a mutable descriptor. To address this, this patch only bypasses leasing for this read operation, so that we don't run the full back reference validation during create operations on schema. This patch also adds tests to make sure concurrent CREATE statements never hit unexpected contention. Release note: None
6960965 to
f2d483e
Compare
|
bors r+ |
|
@rafiss TFTR! |
Presently, schema descriptor validation read all function descriptors under schema. This is problematic when concurrent CREATE TABLE statements referring to any function under a schema are executed will run into frequent transaction retry errors. To address this this patch will do the following:
Fixes: #138384