Skip to content

Commit

Permalink
sql/catalog: avoid making copies of table descriptors
Browse files Browse the repository at this point in the history 
Previously, we would copy table descriptors even if no
PostDeserialization changes would apply eagerly. This could lead to
extra over head when accessing a large number of tables in the
pg_catalog tables. To address this, this patch will make a copy only if
we need to modify the original descriptor because of a post
deserialization change.

Fixes: cockroachdb#121579

Release note: None
  • Loading branch information
@fqazi
fqazi committed Jul 30, 2024
1 parent 4071c6a commit 4a1537a
Show file tree
Hide file tree
Showing 2 changed files with 140 additions and 53 deletions.
192 changes: 139 additions & 53 deletions pkg/sql/catalog/tabledesc/table_desc_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,14 +146,32 @@ func (tdb *tableDescriptorBuilder) RunPostDeserializationChanges() (err error) {
tdb.maybeModified.ModificationTime = tdb.mvccTimestamp
tdb.changes.Add(catalog.SetModTimeToMVCCTimestamp)
}
c, err := maybeFillInDescriptor(tdb.maybeModified)
c, err := maybeFillInDescriptor(tdb)
if err != nil {
return err
}
c.ForEach(tdb.changes.Add)
return nil
}

// GetLatestPrivilege implements catprivilege.PrivilegeDescriptorBuilder.
func (tdb *tableDescriptorBuilder) GetLatestPrivilege() *catpb.PrivilegeDescriptor {
p := tdb.getLatestDesc().Privileges
if p == nil {
return tdb.GetMutablePrivilege()
}
return p
}

// GetMutablePrivilege implements catprivilege.PrivilegeDescriptorBuilder.
func (tdb *tableDescriptorBuilder) GetMutablePrivilege() *catpb.PrivilegeDescriptor {
d := tdb.getOrInitModifiedDesc()
if d.Privileges == nil {
d.Privileges = &catpb.PrivilegeDescriptor{}
}
return d.Privileges
}

// RunRestoreChanges implements the catalog.DescriptorBuilder interface.
func (tdb *tableDescriptorBuilder) RunRestoreChanges(
version clusterversion.ClusterVersion, descLookupFn func(id descpb.ID) catalog.Descriptor,
Expand Down Expand Up @@ -340,6 +358,27 @@ func (tdb *tableDescriptorBuilder) BuildCreatedMutable() catalog.MutableDescript
return tdb.BuildCreatedMutableTable()
}

// getLatestDesc returns the modified descriptor if it exists, or else the
// original descriptor.
func (tdb *tableDescriptorBuilder) getLatestDesc() *descpb.TableDescriptor {
desc := tdb.maybeModified
if desc == nil {
desc = tdb.original
}
return desc
}

// getOrInitModifiedDesc returns the modified descriptor, and clones it from
// the original descriptor if it is not already available. This is a helper
// function that makes it easier to lazily initialize the modified descriptor,
// since protoutil.Clone is expensive.
func (tdb *tableDescriptorBuilder) getOrInitModifiedDesc() *descpb.TableDescriptor {
if tdb.maybeModified == nil {
tdb.maybeModified = protoutil.Clone(tdb.original).(*descpb.TableDescriptor)
}
return tdb.maybeModified
}

// BuildCreatedMutableTable returns a mutable descriptor for a table
// which is in the process of being created.
func (tdb *tableDescriptorBuilder) BuildCreatedMutableTable() *Mutable {
Expand Down Expand Up @@ -370,61 +409,76 @@ func makeImmutable(tbl *descpb.TableDescriptor) *immutable {
// This includes format upgrades and optional changes that can be handled by all version
// (for example: additional default privileges).
func maybeFillInDescriptor(
desc *descpb.TableDescriptor,
builder *tableDescriptorBuilder,
) (changes catalog.PostDeserializationChanges, err error) {
set := func(change catalog.PostDeserializationChangeType, cond bool) {
if cond {
changes.Add(change)
}
}
set(catalog.SetCreateAsOfTimeUsingModTime, maybeSetCreateAsOfTime(desc))
set(catalog.UpgradedFormatVersion, maybeUpgradeFormatVersion(desc))
set(catalog.UpgradedIndexFormatVersion, maybeUpgradePrimaryIndexFormatVersion(desc))
for i := range desc.Indexes {
idx := &desc.Indexes[i]
set(catalog.UpgradedIndexFormatVersion,
maybeUpgradeSecondaryIndexFormatVersion(idx))
// TODO(rytaft): Remove this case in 24.1.
if idx.NotVisible && idx.Invisibility == 0.0 {
set(catalog.SetIndexInvisibility, true)
idx.Invisibility = 1.0
set(catalog.SetCreateAsOfTimeUsingModTime, maybeSetCreateAsOfTime(builder))
set(catalog.UpgradedFormatVersion, maybeUpgradeFormatVersion(builder))
set(catalog.UpgradedIndexFormatVersion, maybeUpgradePrimaryIndexFormatVersion(builder))
{
orig := builder.getLatestDesc()
for i := range orig.Indexes {
origIdx := orig.Indexes[i]
if origIdx.Version < descpb.StrictIndexColumnIDGuaranteesVersion {
modifiedDesc := builder.getOrInitModifiedDesc()
maybeUpgradeSecondaryIndexFormatVersion(&modifiedDesc.Indexes[i])
}
// TODO(rytaft): Remove this case in 24.1.
if origIdx.NotVisible && origIdx.Invisibility == 0.0 {
modifiedDesc := builder.getOrInitModifiedDesc()
set(catalog.SetIndexInvisibility, true)
modifiedDesc.Indexes[i].Invisibility = 1.0
}
}
}
for i := range desc.Mutations {
if idx := desc.Mutations[i].GetIndex(); idx != nil {
set(catalog.UpgradedIndexFormatVersion,
maybeUpgradeSecondaryIndexFormatVersion(idx))
{
desc := builder.getLatestDesc()
for i := range desc.Mutations {
if idx := desc.Mutations[i].GetIndex(); idx != nil &&
idx.Version < descpb.StrictIndexColumnIDGuaranteesVersion {
idx = builder.getOrInitModifiedDesc().Mutations[i].GetIndex()
set(catalog.UpgradedIndexFormatVersion,
maybeUpgradeSecondaryIndexFormatVersion(idx))
}
}
}
parentSchemaID := desc.GetUnexposedParentSchemaID()
// TODO(richardjcai): Remove this case in 22.2.
if parentSchemaID == descpb.InvalidID {
parentSchemaID = keys.PublicSchemaID
}

var objectType privilege.ObjectType

if desc.IsSequence() {
objectType = privilege.Sequence
} else {
objectType = privilege.Table
{
desc := builder.getLatestDesc()
var objectType privilege.ObjectType
if desc.IsSequence() {
objectType = privilege.Sequence
} else {
objectType = privilege.Table
}
// Make sure the parent schema ID is valid, since during
// upgrades its possible for the user table to not have
// this set correctly.
parentSchemaID := desc.GetUnexposedParentSchemaID()
if parentSchemaID == descpb.InvalidID {
parentSchemaID = keys.PublicSchemaID
}
fixedPrivileges, err := catprivilege.MaybeFixPrivilegesWithBuilder(
builder,
desc.GetParentID(),
parentSchemaID,
objectType,
desc.GetName(),
)
if err != nil {
return catalog.PostDeserializationChanges{}, err
}
set(catalog.UpgradedPrivileges, fixedPrivileges)
}

fixedPrivileges, err := catprivilege.MaybeFixPrivileges(
&desc.Privileges,
desc.GetParentID(),
parentSchemaID,
objectType,
desc.GetName(),
)
if err != nil {
return catalog.PostDeserializationChanges{}, err
}
set(catalog.UpgradedPrivileges, fixedPrivileges)
set(catalog.RemovedDuplicateIDsInRefs, maybeRemoveDuplicateIDsInRefs(desc))
set(catalog.StrippedDanglingSelfBackReferences, maybeStripDanglingSelfBackReferences(desc))
set(catalog.FixSecondaryIndexEncodingType, maybeFixSecondaryIndexEncodingType(desc))
set(catalog.FixedIncorrectForeignKeyOrigins, maybeFixForeignKeySelfReferences(desc))
//set(catalog.UpgradedPrivileges, fixedPrivileges)
set(catalog.RemovedDuplicateIDsInRefs, maybeRemoveDuplicateIDsInRefs(builder))
set(catalog.StrippedDanglingSelfBackReferences, maybeStripDanglingSelfBackReferences(builder))
set(catalog.FixSecondaryIndexEncodingType, maybeFixSecondaryIndexEncodingType(builder))
set(catalog.FixedIncorrectForeignKeyOrigins, maybeFixForeignKeySelfReferences(builder))
return changes, nil
}

Expand Down Expand Up @@ -620,7 +674,8 @@ func maybeUpgradeForeignKeyRepOnIndex(
// FormatVersion (if it's not already there) and returns true if any changes
// were made.
// This method should be called through maybeFillInDescriptor, not directly.
func maybeUpgradeFormatVersion(desc *descpb.TableDescriptor) (wasUpgraded bool) {
func maybeUpgradeFormatVersion(builder *tableDescriptorBuilder) (wasUpgraded bool) {
desc := builder.getLatestDesc()
for _, pair := range []struct {
targetVersion descpb.FormatVersion
upgradeFn func(*descpb.TableDescriptor)
Expand All @@ -629,6 +684,7 @@ func maybeUpgradeFormatVersion(desc *descpb.TableDescriptor) (wasUpgraded bool)
{descpb.InterleavedFormatVersion, func(_ *descpb.TableDescriptor) {}},
} {
if desc.FormatVersion < pair.targetVersion {
desc = builder.getOrInitModifiedDesc()
pair.upgradeFn(desc)
desc.FormatVersion = pair.targetVersion
wasUpgraded = true
Expand Down Expand Up @@ -684,16 +740,21 @@ func upgradeToFamilyFormatVersion(desc *descpb.TableDescriptor) {

// maybeUpgradePrimaryIndexFormatVersion tries to promote a primary index to
// version PrimaryIndexWithStoredColumnsVersion whenever possible.
func maybeUpgradePrimaryIndexFormatVersion(desc *descpb.TableDescriptor) (hasChanged bool) {
func maybeUpgradePrimaryIndexFormatVersion(builder *tableDescriptorBuilder) (hasChanged bool) {
desc := builder.getLatestDesc()
// Always set the correct encoding type for the primary index.
desc.PrimaryIndex.EncodingType = catenumpb.PrimaryIndexEncoding
if desc.PrimaryIndex.EncodingType != catenumpb.PrimaryIndexEncoding {
desc = builder.getOrInitModifiedDesc()
desc.PrimaryIndex.EncodingType = catenumpb.PrimaryIndexEncoding
}
// Check if primary index needs updating.
switch desc.PrimaryIndex.Version {
case descpb.PrimaryIndexWithStoredColumnsVersion:
return false
default:
break
}
desc = builder.getOrInitModifiedDesc()
// Update primary index by populating StoreColumnIDs/Names slices.
nonVirtualCols := make([]*descpb.ColumnDescriptor, 0, len(desc.Columns)+len(desc.Mutations))
maybeAddCol := func(col *descpb.ColumnDescriptor) {
Expand Down Expand Up @@ -862,14 +923,17 @@ func maybeAddConstraintIDs(desc *descpb.TableDescriptor) (hasChanged bool) {

// maybeRemoveDuplicateIDsInRefs ensures that IDs in references to other tables
// are not duplicated.
func maybeRemoveDuplicateIDsInRefs(d *descpb.TableDescriptor) (hasChanged bool) {
func maybeRemoveDuplicateIDsInRefs(builder *tableDescriptorBuilder) (hasChanged bool) {
// Strip duplicates from DependsOn.
d := builder.getLatestDesc()
if s := cleanedIDs(d.DependsOn); len(s) < len(d.DependsOn) {
d = builder.getOrInitModifiedDesc()
d.DependsOn = s
hasChanged = true
}
// Do the same for DependsOnTypes.
if s := cleanedIDs(d.DependsOnTypes); len(s) < len(d.DependsOnTypes) {
d = builder.getOrInitModifiedDesc()
d.DependsOnTypes = s
hasChanged = true
}
Expand All @@ -878,6 +942,7 @@ func maybeRemoveDuplicateIDsInRefs(d *descpb.TableDescriptor) (hasChanged bool)
ref := &d.DependedOnBy[i]
s := catalog.MakeTableColSet(ref.ColumnIDs...).Ordered()
if len(s) < len(ref.ColumnIDs) {
d = builder.getOrInitModifiedDesc()
ref.ColumnIDs = s
hasChanged = true
}
Expand All @@ -886,10 +951,12 @@ func maybeRemoveDuplicateIDsInRefs(d *descpb.TableDescriptor) (hasChanged bool)
for i := range d.Columns {
col := &d.Columns[i]
if s := cleanedIDs(col.UsesSequenceIds); len(s) < len(col.UsesSequenceIds) {
d = builder.getOrInitModifiedDesc()
col.UsesSequenceIds = s
hasChanged = true
}
if s := cleanedIDs(col.OwnsSequenceIds); len(s) < len(col.OwnsSequenceIds) {
d = builder.getOrInitModifiedDesc()
col.OwnsSequenceIds = s
hasChanged = true
}
Expand All @@ -909,11 +976,21 @@ func cleanedIDs(input []descpb.ID) []descpb.ID {
// within the table descriptor itself which don't exist.
//
// TODO(postamar): extend as needed to column references and whatnot
func maybeStripDanglingSelfBackReferences(tbl *descpb.TableDescriptor) (hasChanged bool) {
func maybeStripDanglingSelfBackReferences(builder *tableDescriptorBuilder) (hasChanged bool) {
tbl := builder.getLatestDesc()
var mutationIDs intsets.Fast
for _, m := range tbl.Mutations {
mutationIDs.Add(int(m.MutationID))
}
for _, backref := range tbl.MutationJobs {
if !mutationIDs.Contains(int(backref.MutationID)) {
tbl = builder.getOrInitModifiedDesc()
hasChanged = true
}
}
if !hasChanged {
return
}
// Remove mutation_jobs entries which don't reference a valid mutation ID.
{
sliceIdx := 0
Expand All @@ -925,20 +1002,22 @@ func maybeStripDanglingSelfBackReferences(tbl *descpb.TableDescriptor) (hasChang
}
if sliceIdx < len(tbl.MutationJobs) {
tbl.MutationJobs = tbl.MutationJobs[:sliceIdx]
hasChanged = true
}
}
return hasChanged
}

func maybeFixSecondaryIndexEncodingType(desc *descpb.TableDescriptor) (hasChanged bool) {
func maybeFixSecondaryIndexEncodingType(builder *tableDescriptorBuilder) (hasChanged bool) {
desc := builder.getLatestDesc()
if desc.DeclarativeSchemaChangerState != nil || len(desc.Mutations) > 0 {
// Don't try to fix the encoding type if a schema change is in progress.
return false
}
for i := range desc.Indexes {
idx := &desc.Indexes[i]
if idx.EncodingType != catenumpb.SecondaryIndexEncoding {
desc = builder.getOrInitModifiedDesc()
idx = &desc.Indexes[i]
idx.EncodingType = catenumpb.SecondaryIndexEncoding
hasChanged = true
}
Expand All @@ -956,7 +1035,8 @@ func maybeFixSecondaryIndexEncodingType(desc *descpb.TableDescriptor) (hasChange
// ModificationTime fields are both unset for the first Version of a
// TableDescriptor and the code relies on the value being set based on the
// MVCC timestamp.
func maybeSetCreateAsOfTime(desc *descpb.TableDescriptor) (hasChanged bool) {
func maybeSetCreateAsOfTime(builder *tableDescriptorBuilder) (hasChanged bool) {
desc := builder.getLatestDesc()
if !desc.CreateAsOfTime.IsEmpty() || desc.Version > 1 || desc.ModificationTime.IsEmpty() {
return false
}
Expand All @@ -968,6 +1048,7 @@ func maybeSetCreateAsOfTime(desc *descpb.TableDescriptor) (hasChanged bool) {
// The expectation is that this is only set when the version is 2.
// For any version greater than that, this is not accurate but better than
// nothing at all.
desc = builder.getOrInitModifiedDesc()
desc.CreateAsOfTime = desc.ModificationTime
return true
}
Expand Down Expand Up @@ -1156,17 +1237,22 @@ func resolveTableNamesForIDs(

// maybeFixForeignKeySelfReferences fixes references that should point to the
// current descriptor inside OutboundFKs and InboundFKs.
func maybeFixForeignKeySelfReferences(tableDesc *descpb.TableDescriptor) (wasRepaired bool) {
func maybeFixForeignKeySelfReferences(builder *tableDescriptorBuilder) (wasRepaired bool) {
tableDesc := builder.getLatestDesc()
for idx := range tableDesc.OutboundFKs {
fk := &tableDesc.OutboundFKs[idx]
if fk.OriginTableID != tableDesc.ID {
tableDesc = builder.getOrInitModifiedDesc()
fk = &tableDesc.OutboundFKs[idx]
fk.OriginTableID = tableDesc.ID
wasRepaired = true
}
}
for idx := range tableDesc.InboundFKs {
fk := &tableDesc.InboundFKs[idx]
if fk.ReferencedTableID != tableDesc.ID {
tableDesc = builder.getOrInitModifiedDesc()
fk = &tableDesc.InboundFKs[idx]
fk.ReferencedTableID = tableDesc.ID
wasRepaired = true
}
Expand Down
1 change: 1 addition & 0 deletions pkg/upgrade/upgrades/descriptor_utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ func TestCreateSystemTable(t *testing.T) {
username.NodeUserName(),
),
}
fakeTable.Privileges.Version = catpb.OwnerVersion

table := tabledesc.NewBuilder(&fakeTable).BuildCreatedMutable().(catalog.TableDescriptor)

Expand Down

0 comments on commit 4a1537a

Please sign in to comment.