sql,descs: add & adopt descriptor_validation session var#90488
sql,descs: add & adopt descriptor_validation session var#90488craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
08d1469 to
a15b492
Compare
|
@ajwerner this is ready for review. This commit only covers part of the work required for #50651 but I'd like to merge it ahead of the rest because it has enough merit of its own and offers a strict improvement over the use of a cluster setting for controlling descriptor validation behaviour. For the actual DROP story I'm not quite sure what to do yet and I'll want to explore. Contrary to initial impressions perhaps the declarative schema changer will prove more robust after all. This would involve making |
ajwerner
left a comment
There was a problem hiding this comment.
package descs
type SessionData interface {
TemporarySchemaProvider
ValidationMode() sessiondatapb.ValidationMode
}package catsessiondata
type SessionData sessiondata.Stack
func MakeSessionData(sds *sessiondata.Stack) *SesssionData { return (*SessionData)(sds) }
func (sd *SessionData) toStack() *sessiondata.Stack { return (*sessiondata.Stack)(sd) }
func (sd *SessionData) ValidationMode() sessiondatapb.ValidationMode {
return sd.toStack().Top().ValidationMode()
}Reviewed 35 of 45 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @postamar)
pkg/sql/catalog/descs/collection.go line 51 at r1 (raw file):
systemDatabase *catkv.SystemDatabaseCache, virtualSchemas catalog.VirtualSchemas, sds *sessiondata.Stack,
nit: how would you feel about narrowing the scope of the interface you need here to just the relevant parts of the sessiondata.Stack? Time has shown that narrowing the dependencies prevents sprawl and increases clarity.
|
Thanks for the suggestion. I hesitated to do that, worrying it might be overkill, but now I'll do it. I might have to implement that interface both on SessionData and on Stack, because AFAIK the stack has a different lifecycle than the collection: the collection gets reset every txn but the stack may change within the txn. That doesn't bother me, though. |
b0eb698 to
3ab5918
Compare
|
Assuming that CI is happy, this is ready for another look. I think your suggested changes were worth it. |
This commit removes the `sql.catalog.descs.validate_on_write.enabled` cluster setting and replaces it with the `descriptor_validation` session variable. The default value is 'on' which indicates that catalog descriptors are to be validated when both read from- and written to the system.descriptor table. Other possible values are 'off' which disables validation entirely and 'read_only' which disables it for writes. This commit therefore plumbs the session data into the descs.Collection object and for this purpose defines a new DescriptorSessionDataProvider interface, which is implemented in a new catsessiondata package. Informs cockroachdb#50651. Release note (sql change): added a new 'descriptor_validation' session variable which can be set to 'read_only' or 'off' to disable descriptor validation, which may be useful when mitigating or recovering from catalog corruption.
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 42 of 48 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:
|
Thanks for the reviews! bors r+ |
|
Build succeeded: |
This commit removes the
sql.catalog.descs.validate_on_write.enabledcluster setting and replaces it with thedescriptor_validationsession variable. The default value is 'on' which indicates that catalog descriptors are to be validated when both read from- and written to the system.descriptor table. Other possible values are 'off' which disables validation entirely and 'read_only' which disables it for writes.Informs #50651.
Release note (sql change): added a new 'descriptor_validation' session variable which can be set to 'read_only' or 'off' to disable descriptor validation, which may be useful when mitigating or recovering from catalog corruption.