Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: add framework for testing only descriptor validation #54188

Merged
merged 1 commit into from
Sep 17, 2020

Conversation

arulajmani
Copy link
Collaborator

@arulajmani arulajmani commented Sep 10, 2020

This patch adds a new field on the ExecutorTestingKnobs called
TestingDescriptorValidation. This creates the structure to perform
certain table descriptor validation only during testing scenarios, i.e,
when this knob is turned on. This is achieved by intercepting the
context at the conn_executor level to include a value that indicates
to the validation functions if extra validation should be performed.
By intercepting the context at this level, instead of somewhere closer
to the Validate calls, we avoid having to find and intercept the context
in all possible codepaths -- which was getting quite messy.

On the validation side, this patch adds two to be filled functions --
validateCrossReferencesIfTesting and validateTableIfTesting. We don't
validate cross references for mutable descriptors, so providing these
two functions allows us to preserve this pattern and cover both
mutable/immutable descriptors in testing only validation. The functions
are currently no-ops and will be filled in in upcoming patches.

Currently, the testing knob is turned on by default for all test
servers. If you're writing a test and don't want to opt into this
validation for some reason, there's a
DisableTestingDescriptorValidation on server args that can be set.

Release note: None

@arulajmani arulajmani requested review from ajwerner, a team and pbardea and removed request for a team September 10, 2020 15:48
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@arulajmani arulajmani force-pushed the validate branch 3 times, most recently from c7a6b95 to d29c669 Compare September 11, 2020 01:03
@pbardea pbardea removed their request for review September 14, 2020 13:50
Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 32 of 33 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @arulajmani)


pkg/sql/catalog/tabledesc/structured.go, line 1434 at r1 (raw file):

// PerformTestingDescriptorValidation can be set as a value on a context to
// ensure testing specific descriptor validation happens.
var PerformTestingDescriptorValidation = testingDescriptorValidation(true)

super nit: i think this is nicer to read

var PerformTestingDescriptorValidation testingDescriptorValidation = true

pkg/sql/logictest/logic.go, line 1262 at r1 (raw file):

				},
				//SQLExecutor: &sql.ExecutorTestingKnobs{
				//	TestingDescriptorValidation: true,

why is this commented out?

This patch adds a new field on the `ExecutorTestingKnobs` called
`TestingDescriptorValidation`. This creates the structure to perform
certain table descriptor validation only during testing scenarios, i.e,
when this knob is turned on. This is achieved by intercepting the
context at the `conn_executor` level to include a value that indicates
to the validation functions if extra validation should be performed.
By intercepting the context at this level, instead of somewhere closer
to the Validate calls, we avoid having to find and intercept the context
in all possible codepaths -- which was getting quite messy.

On the validation side, this patch adds two to be filled functions --
validateCrossReferencesIfTesting and validateTableIfTesting. We don't
validate cross references for mutable descriptors, so providing these
two functions allows us to preserve this pattern and cover both
mutable/immutable descriptors in testing only validation. The functions
are currently no-ops and will be filled in in upcoming patches.

Currently, the testing knob is turned on by default for all test
servers. If you're writing a test and don't want to opt into this
validation for some reason, there's a
`DisableTestingDescriptorValidation` on server args that can be set.

Release note: None
Copy link
Collaborator Author

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @otan)


pkg/sql/catalog/tabledesc/structured.go, line 1434 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

super nit: i think this is nicer to read

var PerformTestingDescriptorValidation testingDescriptorValidation = true

Done.


pkg/sql/logictest/logic.go, line 1262 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

why is this commented out?

My bad, I intended to remove this but instead left it commented out. This was from a previous approach where this descirptor validation was only for logictests, but in the new approach it's for all tests unless you opt out, making this knob here useless.

@arulajmani
Copy link
Collaborator Author

bors r=otan

@craig
Copy link
Contributor

craig bot commented Sep 17, 2020

Build succeeded:

@craig craig bot merged commit 6bf81ee into cockroachdb:master Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants