Skip to content

Commit b4d326b

Browse files
committed
sql/catalog/descs: consolidate uncommitted metadata into MutableCatalog
Previously, the `descs.Collection` tracked uncommitted comments and zone configs in two separate structs (`uncommittedComments` and `uncommittedZoneConfigs`), each with their own map-based storage and cache tracking. This commit consolidates both into a single `uncommittedMetadata` struct backed by `nstree.MutableCatalog`. This approach was suggested in PR #105981 because MutableCatalog already provides: - Built-in lazy initialization - Memory tracking via byteSize - Unified storage for comments and zone configs The new design uses MutableCatalog for value storage and separate "absent" sets to track entries that have been looked up and found to be deleted or non-existent. This maintains the same three-state cache semantics (not cached, cached-absent, cached-with-value) while consolidating the implementation. Note: `AddUncommittedComment` now returns an error to properly propagate validation errors from `MutableCatalog.UpsertComment`. Epic: None Release note: None
1 parent f84948a commit b4d326b

File tree

4 files changed

+140
-97
lines changed

4 files changed

+140
-97
lines changed

pkg/sql/catalog/descs/collection.go

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,9 @@ type Collection struct {
8080
// not visible to other transactions.
8181
uncommitted uncommittedDescriptors
8282

83-
uncommittedComments uncommittedComments
84-
85-
uncommittedZoneConfigs uncommittedZoneConfigs
83+
// uncommittedMetadata tracks uncommitted comments and zone configs using
84+
// a single nstree.MutableCatalog for storage.
85+
uncommittedMetadata uncommittedMetadata
8686

8787
// A cached implementation of catkv.CatalogReader used for accessing stored
8888
// descriptors, namespace entries, comments and zone configs. The cache
@@ -733,8 +733,7 @@ func (tc *Collection) WriteCommentToBatch(
733733
return err
734734
}
735735

736-
tc.AddUncommittedComment(key, cmt)
737-
return nil
736+
return tc.AddUncommittedComment(key, cmt)
738737
}
739738

740739
// DeleteCommentInBatch deletes a comment with the given (objID, subID, cmtType) key in
@@ -1286,14 +1285,14 @@ func (tc *Collection) aggregateAllLayers(
12861285
})
12871286
// Add stored comments which are not shadowed.
12881287
_ = stored.ForEachComment(func(key catalogkeys.CommentKey, cmt string) error {
1289-
if _, _, isShadowed := tc.uncommittedComments.getUncommitted(key); !isShadowed {
1288+
if !tc.uncommittedMetadata.isCommentCached(key) {
12901289
return ret.UpsertComment(key, cmt)
12911290
}
12921291
return nil
12931292
})
12941293
// Add stored zone configs which are not shadowed.
12951294
_ = stored.ForEachZoneConfig(func(id descpb.ID, zc catalog.ZoneConfig) error {
1296-
if _, isShadowed := tc.uncommittedZoneConfigs.getUncommitted(id); !isShadowed {
1295+
if !tc.uncommittedMetadata.isZoneConfigCached(id) {
12971296
ret.UpsertZoneConfig(id, zc.ZoneConfigProto(), zc.GetRawBytesInStorage())
12981297
}
12991298
return nil
@@ -1330,10 +1329,10 @@ func (tc *Collection) aggregateAllLayers(
13301329
})
13311330
}
13321331
// Add uncommitted comments and zone configs.
1333-
if err := tc.uncommittedComments.addAllToCatalog(ret); err != nil {
1332+
if err := tc.uncommittedMetadata.addAllCommentsToCatalog(&ret); err != nil {
13341333
return nstree.MutableCatalog{}, err
13351334
}
1336-
tc.uncommittedZoneConfigs.addAllToCatalog(ret)
1335+
tc.uncommittedMetadata.addAllZoneConfigsToCatalog(&ret)
13371336
// Remove deleted descriptors from consideration, re-read and add the rest.
13381337
tc.deletedDescs.ForEach(descIDs.Remove)
13391338
allDescs := make([]catalog.Descriptor, descIDs.Len())
@@ -1501,8 +1500,7 @@ func (tc *Collection) ResetSyntheticDescriptors() {
15011500
// ResetUncommitted resets all uncommitted state in the Collection.
15021501
func (tc *Collection) ResetUncommitted(ctx context.Context) {
15031502
tc.uncommitted.reset(ctx)
1504-
tc.uncommittedComments.reset()
1505-
tc.uncommittedZoneConfigs.reset()
1503+
tc.uncommittedMetadata.reset()
15061504
tc.shadowedNames = nil
15071505
tc.validationLevels = nil
15081506
tc.ResetSyntheticDescriptors()

pkg/sql/catalog/descs/descriptor.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import (
2828

2929
// GetComment fetches comment from uncommitted cache if it exists, otherwise from storage.
3030
func (tc *Collection) GetComment(key catalogkeys.CommentKey) (string, bool) {
31-
if cmt, hasCmt, cached := tc.uncommittedComments.getUncommitted(key); cached {
31+
if cmt, hasCmt, cached := tc.uncommittedMetadata.getUncommittedComment(key); cached {
3232
return cmt, hasCmt
3333
}
3434
if tc.cr.IsCommentInCache(descpb.ID(key.ObjectID)) {
@@ -45,8 +45,8 @@ func (tc *Collection) GetComment(key catalogkeys.CommentKey) (string, bool) {
4545
}
4646

4747
// AddUncommittedComment adds a comment to uncommitted cache.
48-
func (tc *Collection) AddUncommittedComment(key catalogkeys.CommentKey, cmt string) {
49-
tc.uncommittedComments.upsert(key, cmt)
48+
func (tc *Collection) AddUncommittedComment(key catalogkeys.CommentKey, cmt string) error {
49+
return tc.uncommittedMetadata.upsertComment(key, cmt)
5050
}
5151

5252
// GetZoneConfig is similar to GetZoneConfigs but only
@@ -70,7 +70,7 @@ func (tc *Collection) GetZoneConfigs(
7070
ret := make(map[descpb.ID]catalog.ZoneConfig)
7171
var storageIDs catalog.DescriptorIDSet
7272
for _, id := range descIDs {
73-
if zc, cached := tc.uncommittedZoneConfigs.getUncommitted(id); cached {
73+
if zc, cached := tc.uncommittedMetadata.getUncommittedZoneConfig(id); cached {
7474
if zc != nil {
7575
ret[id] = zc.Clone()
7676
}
@@ -96,27 +96,27 @@ func (tc *Collection) GetZoneConfigs(
9696

9797
// AddUncommittedZoneConfig adds a zone config to the uncommitted cache.
9898
func (tc *Collection) AddUncommittedZoneConfig(id descpb.ID, zc *zonepb.ZoneConfig) error {
99-
return tc.uncommittedZoneConfigs.upsert(id, zc)
99+
return tc.uncommittedMetadata.upsertZoneConfig(id, zc)
100100
}
101101

102102
// MarkUncommittedZoneConfigDeleted adds the descriptor id to the uncommitted zone config layer, but indicates
103103
// that the zone config has been dropped or does not exist for this descriptor id.
104104
func (tc *Collection) MarkUncommittedZoneConfigDeleted(id descpb.ID) {
105-
tc.uncommittedZoneConfigs.markNoZoneConfig(id)
105+
tc.uncommittedMetadata.markNoZoneConfig(id)
106106
}
107107

108108
// MarkUncommittedCommentDeleted adds the key to uncommitted cache, but indicates
109109
// that the comment has been dropped, therefore the cached information is that
110110
// "there is no comment for this key".
111111
func (tc *Collection) MarkUncommittedCommentDeleted(key catalogkeys.CommentKey) {
112-
tc.uncommittedComments.markNoComment(key)
112+
tc.uncommittedMetadata.markNoComment(key)
113113
}
114114

115115
// MarkUncommittedCommentDeletedForTable is similar to
116116
// MarkUncommittedCommentDeleted, but it marks all comments on the table as
117117
// deleted.
118118
func (tc *Collection) MarkUncommittedCommentDeletedForTable(tblID descpb.ID) {
119-
tc.uncommittedComments.markTableDeleted(tblID)
119+
tc.uncommittedMetadata.markTableCommentsDeleted(tblID)
120120
}
121121

122122
// getDescriptorsByID implements the Collection method of the same name.

pkg/sql/catalog/descs/factory.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,8 +158,7 @@ func (cf *CollectionFactory) NewCollection(ctx context.Context, options ...Optio
158158
virtual: makeVirtualDescriptors(cf.virtualSchemas),
159159
leased: makeLeasedDescriptors(lm),
160160
uncommitted: makeUncommittedDescriptors(cfg.monitor),
161-
uncommittedComments: makeUncommittedComments(),
162-
uncommittedZoneConfigs: makeUncommittedZoneConfigs(),
161+
uncommittedMetadata: makeUncommittedMetadata(),
163162
cr: catkv.NewCatalogReader(cf.codec, v, cf.systemDatabase, cfg.monitor),
164163
temporarySchemaProvider: cfg.dsdp,
165164
validationModeProvider: cfg.dsdp,

pkg/sql/catalog/descs/uncommitted_metadata.go

Lines changed: 122 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -12,113 +12,145 @@ import (
1212
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
1313
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
1414
"github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree"
15-
"github.com/cockroachdb/cockroach/pkg/sql/catalog/zone"
1615
)
1716

18-
type uncommittedComments struct {
19-
uncommitted map[catalogkeys.CommentKey]string
20-
cachedKeys map[catalogkeys.CommentKey]struct{}
21-
}
17+
// uncommittedMetadata tracks uncommitted comments and zone configs using
18+
// an nstree.MutableCatalog for storage, plus separate sets for tracking
19+
// entries that have been explicitly marked as absent.
20+
type uncommittedMetadata struct {
21+
// mc stores actual uncommitted values (comments and zone configs).
22+
// This handles both storage and memory tracking via its built-in byteSize.
23+
mc nstree.MutableCatalog
2224

23-
func makeUncommittedComments() uncommittedComments {
24-
return uncommittedComments{}
25-
}
25+
// absentCommentKeys tracks comment keys that have been looked up and
26+
// found to be absent (deleted or never existed). This is separate from
27+
// mc because MutableCatalog only stores actual values.
28+
absentCommentKeys map[catalogkeys.CommentKey]struct{}
2629

27-
func (uc *uncommittedComments) reset() {
28-
*uc = uncommittedComments{}
30+
// absentZoneConfigIDs tracks descriptor IDs whose zone configs have
31+
// been looked up and found to be absent.
32+
absentZoneConfigIDs map[descpb.ID]struct{}
2933
}
3034

31-
func (uc *uncommittedComments) lazyInitMaps() {
32-
if uc.uncommitted == nil {
33-
uc.uncommitted = make(map[catalogkeys.CommentKey]string)
34-
uc.cachedKeys = make(map[catalogkeys.CommentKey]struct{})
35-
}
35+
func makeUncommittedMetadata() uncommittedMetadata {
36+
return uncommittedMetadata{}
3637
}
3738

38-
func (uc *uncommittedComments) getUncommitted(
39-
key catalogkeys.CommentKey,
40-
) (cmt string, hasCmt bool, cached bool) {
41-
if _, ok := uc.cachedKeys[key]; !ok {
42-
return "", false, false
43-
}
44-
45-
cmt, hasCmt = uc.uncommitted[key]
46-
return cmt, hasCmt, true
39+
func (um *uncommittedMetadata) reset() {
40+
um.mc.Clear()
41+
um.absentCommentKeys = nil
42+
um.absentZoneConfigIDs = nil
4743
}
4844

49-
// markNoComment lets the cache know that the comment for this key is dropped.
50-
func (uc *uncommittedComments) markNoComment(key catalogkeys.CommentKey) {
51-
uc.lazyInitMaps()
52-
delete(uc.uncommitted, key)
53-
uc.cachedKeys[key] = struct{}{}
54-
}
45+
// Comment methods
5546

56-
func (uc *uncommittedComments) markTableDeleted(tblID descpb.ID) {
57-
// NOTE: lazyInitMaps() not needed, maps can remain nil.
58-
var keysToDel []catalogkeys.CommentKey
59-
for k := range uc.uncommitted {
60-
if k.ObjectID == uint32(tblID) {
61-
keysToDel = append(keysToDel, k)
47+
// getUncommittedComment returns the uncommitted comment for the given key.
48+
// It returns (comment, hasComment, cached) where:
49+
// - cached=false means the key hasn't been looked up yet
50+
// - cached=true, hasComment=false means the key was looked up and is absent
51+
// - cached=true, hasComment=true means the key has a value
52+
func (um *uncommittedMetadata) getUncommittedComment(
53+
key catalogkeys.CommentKey,
54+
) (cmt string, hasCmt bool, cached bool) {
55+
if um.absentCommentKeys != nil {
56+
if _, absent := um.absentCommentKeys[key]; absent {
57+
return "", false, true
6258
}
6359
}
64-
for _, k := range keysToDel {
65-
delete(uc.uncommitted, k)
60+
if cmt, found := um.mc.LookupComment(key); found {
61+
return cmt, true, true
6662
}
63+
return "", false, false
6764
}
6865

69-
func (uc *uncommittedComments) upsert(key catalogkeys.CommentKey, cmt string) {
70-
uc.lazyInitMaps()
71-
uc.cachedKeys[key] = struct{}{}
72-
uc.uncommitted[key] = cmt
73-
}
74-
75-
func (uc *uncommittedComments) addAllToCatalog(mc nstree.MutableCatalog) error {
76-
for ck, cmt := range uc.uncommitted {
77-
if err := mc.UpsertComment(ck, cmt); err != nil {
78-
return err
79-
}
66+
// markNoComment marks the comment for this key as absent (deleted or never
67+
// existed). This adds it to the cache so subsequent lookups know not to
68+
// query storage.
69+
func (um *uncommittedMetadata) markNoComment(key catalogkeys.CommentKey) {
70+
um.mc.DeleteComment(key)
71+
if um.absentCommentKeys == nil {
72+
um.absentCommentKeys = make(map[catalogkeys.CommentKey]struct{})
8073
}
81-
return nil
74+
um.absentCommentKeys[key] = struct{}{}
8275
}
8376

84-
type uncommittedZoneConfigs struct {
85-
uncommitted map[descpb.ID]catalog.ZoneConfig
86-
cachedDescs map[descpb.ID]struct{}
77+
// upsertComment adds or updates a comment in the uncommitted layer.
78+
func (um *uncommittedMetadata) upsertComment(key catalogkeys.CommentKey, cmt string) error {
79+
delete(um.absentCommentKeys, key)
80+
return um.mc.UpsertComment(key, cmt)
8781
}
8882

89-
func makeUncommittedZoneConfigs() uncommittedZoneConfigs {
90-
return uncommittedZoneConfigs{}
83+
// markTableCommentsDeleted removes all comments for the given table from the
84+
// uncommitted layer. Note: this only affects comments that were previously
85+
// added to the uncommitted layer; it does not mark stored comments as deleted.
86+
func (um *uncommittedMetadata) markTableCommentsDeleted(tblID descpb.ID) {
87+
// Collect keys to delete from MutableCatalog.
88+
var keysToDelete []catalogkeys.CommentKey
89+
_ = um.mc.ForEachComment(func(key catalogkeys.CommentKey, _ string) error {
90+
if descpb.ID(key.ObjectID) == tblID {
91+
keysToDelete = append(keysToDelete, key)
92+
}
93+
return nil
94+
})
95+
for _, key := range keysToDelete {
96+
um.mc.DeleteComment(key)
97+
}
9198
}
9299

93-
func (uc *uncommittedZoneConfigs) reset() {
94-
*uc = uncommittedZoneConfigs{}
100+
// addAllCommentsToCatalog copies all uncommitted comments to the target catalog.
101+
func (um *uncommittedMetadata) addAllCommentsToCatalog(target *nstree.MutableCatalog) error {
102+
return um.mc.ForEachComment(func(key catalogkeys.CommentKey, cmt string) error {
103+
return target.UpsertComment(key, cmt)
104+
})
95105
}
96106

97-
func (uc *uncommittedZoneConfigs) lazyInitMaps() {
98-
if uc.uncommitted == nil {
99-
uc.uncommitted = make(map[descpb.ID]catalog.ZoneConfig)
100-
uc.cachedDescs = make(map[descpb.ID]struct{})
107+
// isCommentCached returns true if the comment key is in the cache (either as
108+
// a value or marked absent).
109+
func (um *uncommittedMetadata) isCommentCached(key catalogkeys.CommentKey) bool {
110+
if um.absentCommentKeys != nil {
111+
if _, absent := um.absentCommentKeys[key]; absent {
112+
return true
113+
}
101114
}
115+
_, found := um.mc.LookupComment(key)
116+
return found
102117
}
103118

104-
func (uc *uncommittedZoneConfigs) getUncommitted(
119+
// Zone config methods
120+
121+
// getUncommittedZoneConfig returns the uncommitted zone config for the given ID.
122+
// It returns (zoneConfig, cached) where:
123+
// - cached=false means the ID hasn't been looked up yet
124+
// - cached=true, zoneConfig=nil means the ID was looked up and is absent
125+
// - cached=true, zoneConfig!=nil means the ID has a value
126+
func (um *uncommittedMetadata) getUncommittedZoneConfig(
105127
id descpb.ID,
106128
) (zc catalog.ZoneConfig, cached bool) {
107-
if _, ok := uc.cachedDescs[id]; !ok {
108-
return nil, false
129+
if um.absentZoneConfigIDs != nil {
130+
if _, absent := um.absentZoneConfigIDs[id]; absent {
131+
return nil, true
132+
}
133+
}
134+
if zc := um.mc.LookupZoneConfig(id); zc != nil {
135+
return zc, true
109136
}
110-
return uc.uncommitted[id], true
137+
return nil, false
111138
}
112139

113-
func (uc *uncommittedZoneConfigs) markNoZoneConfig(id descpb.ID) {
114-
uc.lazyInitMaps()
115-
delete(uc.uncommitted, id)
116-
uc.cachedDescs[id] = struct{}{}
140+
// markNoZoneConfig marks the zone config for this ID as absent (deleted or
141+
// never existed). This adds it to the cache so subsequent lookups know not
142+
// to query storage.
143+
func (um *uncommittedMetadata) markNoZoneConfig(id descpb.ID) {
144+
um.mc.DeleteZoneConfig(id)
145+
if um.absentZoneConfigIDs == nil {
146+
um.absentZoneConfigIDs = make(map[descpb.ID]struct{})
147+
}
148+
um.absentZoneConfigIDs[id] = struct{}{}
117149
}
118150

119-
func (uc *uncommittedZoneConfigs) upsert(id descpb.ID, zc *zonepb.ZoneConfig) error {
120-
uc.lazyInitMaps()
121-
uc.cachedDescs[id] = struct{}{}
151+
// upsertZoneConfig adds or updates a zone config in the uncommitted layer.
152+
func (um *uncommittedMetadata) upsertZoneConfig(id descpb.ID, zc *zonepb.ZoneConfig) error {
153+
delete(um.absentZoneConfigIDs, id)
122154
var val roachpb.Value
123155
if err := val.SetProto(zc); err != nil {
124156
return err
@@ -127,12 +159,26 @@ func (uc *uncommittedZoneConfigs) upsert(id descpb.ID, zc *zonepb.ZoneConfig) er
127159
if err != nil {
128160
return err
129161
}
130-
uc.uncommitted[id] = zone.NewZoneConfigWithRawBytes(zc, rawBytes)
162+
um.mc.UpsertZoneConfig(id, zc, rawBytes)
131163
return nil
132164
}
133165

134-
func (uc *uncommittedZoneConfigs) addAllToCatalog(mc nstree.MutableCatalog) {
135-
for id, zc := range uc.uncommitted {
136-
mc.UpsertZoneConfig(id, zc.ZoneConfigProto(), zc.GetRawBytesInStorage())
166+
// addAllZoneConfigsToCatalog copies all uncommitted zone configs to the target
167+
// catalog.
168+
func (um *uncommittedMetadata) addAllZoneConfigsToCatalog(target *nstree.MutableCatalog) {
169+
_ = um.mc.ForEachZoneConfig(func(id descpb.ID, zc catalog.ZoneConfig) error {
170+
target.UpsertZoneConfig(id, zc.ZoneConfigProto(), zc.GetRawBytesInStorage())
171+
return nil
172+
})
173+
}
174+
175+
// isZoneConfigCached returns true if the zone config ID is in the cache
176+
// (either as a value or marked absent).
177+
func (um *uncommittedMetadata) isZoneConfigCached(id descpb.ID) bool {
178+
if um.absentZoneConfigIDs != nil {
179+
if _, absent := um.absentZoneConfigIDs[id]; absent {
180+
return true
181+
}
137182
}
183+
return um.mc.LookupZoneConfig(id) != nil
138184
}

0 commit comments

Comments
 (0)