diff --git a/pkg/sql/descriptor.go b/pkg/sql/descriptor.go index cac8fc079e36..a260898d1cb7 100644 --- a/pkg/sql/descriptor.go +++ b/pkg/sql/descriptor.go @@ -202,8 +202,6 @@ func getDescriptorByID( return errors.Errorf("%q is not a database", desc.String()) } - database.Privileges.MaybeFixPrivileges(id) - if err := database.Validate(); err != nil { return err } diff --git a/pkg/sql/grant_revoke.go b/pkg/sql/grant_revoke.go index 65bf6ffc9ae8..a3345de91816 100644 --- a/pkg/sql/grant_revoke.go +++ b/pkg/sql/grant_revoke.go @@ -91,6 +91,12 @@ func (p *planner) changePrivileges( changePrivilege(privileges, string(grantee)) } + // Validate privilege descriptors directly as the db/table level Validate + // may fix up the descriptor. + if err := privileges.Validate(descriptor.GetID()); err != nil { + return nil, err + } + switch d := descriptor.(type) { case *sqlbase.DatabaseDescriptor: if err := d.Validate(); err != nil { diff --git a/pkg/sql/sqlbase/structured.go b/pkg/sql/sqlbase/structured.go index edc4285b2fc8..a4e60a660f1f 100644 --- a/pkg/sql/sqlbase/structured.go +++ b/pkg/sql/sqlbase/structured.go @@ -1161,6 +1161,11 @@ func (desc *TableDescriptor) ValidateTable(st *cluster.Settings) error { } } + // Fill in any incorrect privileges that may have been missed due to mixed-versions. + // TODO(mberhault): remove this in 2.1 (maybe 2.2) when privilege-fixing migrations have been + // run again and mixed-version clusters always write "good" descriptors. + desc.Privileges.MaybeFixPrivileges(desc.GetID()) + // Validate the privilege descriptor. return desc.Privileges.Validate(desc.GetID()) } @@ -2407,6 +2412,12 @@ func (desc *DatabaseDescriptor) Validate() error { if desc.ID == 0 { return fmt.Errorf("invalid database ID %d", desc.ID) } + + // Fill in any incorrect privileges that may have been missed due to mixed-versions. + // TODO(mberhault): remove this in 2.1 (maybe 2.2) when privilege-fixing migrations have been + // run again and mixed-version clusters always write "good" descriptors. + desc.Privileges.MaybeFixPrivileges(desc.GetID()) + // Validate the privilege descriptor. return desc.Privileges.Validate(desc.GetID()) } diff --git a/pkg/sqlmigrations/migrations.go b/pkg/sqlmigrations/migrations.go index 3ec12264a913..9a6736ba880f 100644 --- a/pkg/sqlmigrations/migrations.go +++ b/pkg/sqlmigrations/migrations.go @@ -181,7 +181,7 @@ var backwardCompatibleMigrations = []migrationDescriptor{ }, { // Introduced in v2.0. - // TODO(benesch): bake this migration into v2.1, but create a new migration + // TODO(mberhault): bake this migration into v2.1, but create a new migration // with the same function to catch any tables written in a mixed-version setting. name: "ensure admin role privileges in all descriptors", workFn: ensureMaxPrivileges,