Skip to content

Commit 282a83e

Browse files
fqazifqazi
committed
catalog: add support reading metadata for leased descriptors
We recently added an option for using leased descriptors for pg_catalog / crdb_internal tables. While this option can fetch descriptor data, we presently have no way of reading metadata for leased descriptors, such as zone configs and comments. This patch introduces a new option to support reading metadata along with leased descriptors when using the GetAll.* APIs. Fixes: cockroachdb#156404 Release note: None
1 parent 1c3040a commit 282a83e

File tree

8 files changed

+207
-49
lines changed

8 files changed

+207
-49
lines changed

‎pkg/sql/catalog/descs/collection.go‎

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -367,7 +367,8 @@ func WithOnlyVersionBump() WriteDescOption {
367367
}
368368

369369
type getAllOptions struct {
370-
allowLeased bool
370+
allowLeased bool
371+
withMetaData bool
371372
}
372373

373374
// GetAllOption defines functional options for GetAll* methods.
@@ -381,11 +382,23 @@ func (c allowLeasedOption) apply(opts *getAllOptions) {
381382
opts.allowLeased = bool(c)
382383
}
383384

385+
type withMetaDataOption bool
386+
387+
func (c withMetaDataOption) apply(opts *getAllOptions) {
388+
opts.withMetaData = bool(c)
389+
}
390+
384391
// WithAllowLeased configures GetAll* methods to allow leased descriptors.
385392
func WithAllowLeased() GetAllOption {
386393
return allowLeasedOption(true)
387394
}
388395

396+
// WithMetaData configures GetAll* methods to fetch comments and zone
397+
// configs.
398+
func WithMetaData() GetAllOption {
399+
return withMetaDataOption(true)
400+
}
401+
389402
// applyGetAllOptions applies the provided functional options to a getAllOptions struct.
390403
func applyGetAllOptions(opts []GetAllOption) getAllOptions {
391404
options := getAllOptions{}
@@ -922,7 +935,7 @@ func (tc *Collection) GetAll(
922935
if err != nil {
923936
return nstree.Catalog{}, err
924937
}
925-
ret, err := tc.aggregateAllLayers(ctx, txn, options.allowLeased, stored)
938+
ret, err := tc.aggregateAllLayers(ctx, txn, options, stored)
926939
if err != nil {
927940
return nstree.Catalog{}, err
928941
}
@@ -947,7 +960,7 @@ func (tc *Collection) GetAllComments(
947960
if err != nil {
948961
return nil, err
949962
}
950-
const allowLeased = false
963+
var allowLeased = getAllOptions{}
951964
comments, err := tc.aggregateAllLayers(ctx, txn, allowLeased, kvComments)
952965
if err != nil {
953966
return nil, err
@@ -974,7 +987,7 @@ func (tc *Collection) GetAllDatabases(
974987
if err != nil {
975988
return nstree.Catalog{}, err
976989
}
977-
ret, err := tc.aggregateAllLayers(ctx, txn, options.allowLeased, stored)
990+
ret, err := tc.aggregateAllLayers(ctx, txn, options, stored)
978991
if err != nil {
979992
return nstree.Catalog{}, err
980993
}
@@ -1001,9 +1014,9 @@ func (tc *Collection) GetAllSchemasInDatabase(
10011014
}
10021015
var ret nstree.MutableCatalog
10031016
if db.HasPublicSchemaWithDescriptor() {
1004-
ret, err = tc.aggregateAllLayers(ctx, txn, options.allowLeased, stored)
1017+
ret, err = tc.aggregateAllLayers(ctx, txn, options, stored)
10051018
} else {
1006-
ret, err = tc.aggregateAllLayers(ctx, txn, options.allowLeased, stored, schemadesc.GetPublicSchema())
1019+
ret, err = tc.aggregateAllLayers(ctx, txn, options, stored, schemadesc.GetPublicSchema())
10071020
}
10081021
if err != nil {
10091022
return nstree.Catalog{}, err
@@ -1045,7 +1058,7 @@ func (tc *Collection) GetAllObjectsInSchema(
10451058
if err != nil {
10461059
return nstree.Catalog{}, err
10471060
}
1048-
ret, err = tc.aggregateAllLayers(ctx, txn, options.allowLeased, stored, sc)
1061+
ret, err = tc.aggregateAllLayers(ctx, txn, options, stored, sc)
10491062
if err != nil {
10501063
return nstree.Catalog{}, err
10511064
}
@@ -1083,7 +1096,7 @@ func (tc *Collection) GetAllInDatabase(
10831096
}); err != nil {
10841097
return nstree.Catalog{}, err
10851098
}
1086-
ret, err := tc.aggregateAllLayers(ctx, txn, options.allowLeased, stored, schemasSlice...)
1099+
ret, err := tc.aggregateAllLayers(ctx, txn, options, stored, schemasSlice...)
10871100
if err != nil {
10881101
return nstree.Catalog{}, err
10891102
}
@@ -1120,9 +1133,9 @@ func (tc *Collection) GetAllTablesInDatabase(
11201133
}
11211134
var ret nstree.MutableCatalog
11221135
if db.HasPublicSchemaWithDescriptor() {
1123-
ret, err = tc.aggregateAllLayers(ctx, txn, options.allowLeased, stored)
1136+
ret, err = tc.aggregateAllLayers(ctx, txn, options, stored)
11241137
} else {
1125-
ret, err = tc.aggregateAllLayers(ctx, txn, options.allowLeased, stored, schemadesc.GetPublicSchema())
1138+
ret, err = tc.aggregateAllLayers(ctx, txn, options, stored, schemadesc.GetPublicSchema())
11261139
}
11271140
if err != nil {
11281141
return nstree.Catalog{}, err
@@ -1147,7 +1160,7 @@ func (tc *Collection) GetAllTablesInDatabase(
11471160
func (tc *Collection) aggregateAllLayers(
11481161
ctx context.Context,
11491162
txn *kv.Txn,
1150-
allowLeased bool,
1163+
getAllOptions getAllOptions,
11511164
stored nstree.Catalog,
11521165
schemas ...catalog.SchemaDescriptor,
11531166
) (ret nstree.MutableCatalog, _ error) {
@@ -1246,9 +1259,12 @@ func (tc *Collection) aggregateAllLayers(
12461259
tc.deletedDescs.ForEach(descIDs.Remove)
12471260
allDescs := make([]catalog.Descriptor, descIDs.Len())
12481261
flags := defaultUnleasedFlags()
1249-
if allowLeased {
1262+
if getAllOptions.allowLeased {
12501263
flags.layerFilters.withoutLeased = false
12511264
}
1265+
if getAllOptions.withMetaData {
1266+
flags.layerFilters.withMetadata = true
1267+
}
12521268
if err := getDescriptorsByID(
12531269
ctx, tc, txn, flags, allDescs, descIDs.Ordered()...,
12541270
); err != nil {

‎pkg/sql/catalog/descs/descriptor.go‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,13 @@ import (
1515
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
1616
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
1717
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
18+
"github.com/cockroachdb/cockroach/pkg/sql/catalog/internal/catkv"
1819
"github.com/cockroachdb/cockroach/pkg/sql/catalog/internal/validate"
1920
"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
2021
"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc"
2122
"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
2223
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
24+
"github.com/cockroachdb/cockroach/pkg/util/buildutil"
2325
"github.com/cockroachdb/cockroach/pkg/util/log"
2426
"github.com/cockroachdb/errors"
2527
)
@@ -29,9 +31,14 @@ func (tc *Collection) GetComment(key catalogkeys.CommentKey) (string, bool) {
2931
if cmt, hasCmt, cached := tc.uncommittedComments.getUncommitted(key); cached {
3032
return cmt, hasCmt
3133
}
32-
if tc.cr.IsIDInCache(descpb.ID(key.ObjectID)) {
34+
if tc.cr.IsCommentInCache(descpb.ID(key.ObjectID)) {
3335
return tc.cr.Cache().LookupComment(key)
3436
}
37+
if buildutil.CrdbTestBuild &&
38+
tc.leased.cache.GetByID(descpb.ID(key.ObjectID)) != nil {
39+
panic(errors.AssertionFailedf("a leased descriptor exist's but metadata data was not cached " +
40+
"ensure that GetAll* API is being used correctly"))
41+
}
3542
// TODO(chengxiong): we need to ensure descriptor if it's not in either cache
3643
// and it's not a pseudo descriptor.
3744
return "", false
@@ -187,9 +194,13 @@ func getDescriptorsByID(
187194

188195
// Read any missing descriptors from storage and add them to the slice.
189196
var readIDs catalog.DescriptorIDSet
197+
var metadataNeeded catalog.DescriptorIDSet
190198
for i, id := range ids {
191199
if descs[i] == nil {
192200
readIDs.Add(id)
201+
} else if flags.layerFilters.withMetadata {
202+
// Otherwise, we need to query metadata only.
203+
metadataNeeded.Add(id)
193204
}
194205
}
195206
if !readIDs.Empty() {
@@ -210,6 +221,13 @@ func getDescriptorsByID(
210221
}
211222
}
212223
}
224+
// If metadata needs to be cached, then execute a read only the metadata.
225+
if !metadataNeeded.Empty() {
226+
_, err := tc.cr.GetByIDs(ctx, txn, metadataNeeded.Ordered(), false, catalog.Any, catkv.WithMetaData(true))
227+
if err != nil {
228+
return err
229+
}
230+
}
213231

214232
// At this point, all descriptors are in the slice, finalize and hydrate them.
215233
if err := tc.finalizeDescriptors(ctx, txn, flags, descs, vls); err != nil {

‎pkg/sql/catalog/descs/getters.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -461,6 +461,10 @@ type layerFilters struct {
461461
// the hydration of another descriptor which depends on it.
462462
// TODO(postamar): untangle the hydration mess
463463
withoutHydration bool
464+
// withMetadata will read zone configs and comments for lease descriptors.
465+
// This is always acquired for descriptors from storage, and may need an extra
466+
// round trip.
467+
withMetadata bool
464468
}
465469

466470
type descFilters struct {

‎pkg/sql/catalog/internal/catkv/catalog_reader.go‎

Lines changed: 87 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,9 @@ type CatalogReader interface {
3535
// is known to be in the cache.
3636
IsIDInCache(id descpb.ID) bool
3737

38+
// IsCommentInCache return true when the comment is cached for this descriptor.
39+
IsCommentInCache(id descpb.ID) bool
40+
3841
// IsNameInCache return true when all the by-name catalog data for this name
3942
// key is known to be in the cache.
4043
IsNameInCache(key descpb.NameInfo) bool
@@ -85,13 +88,15 @@ type CatalogReader interface {
8588

8689
// GetByIDs reads the system.descriptor, system.comments and system.zone
8790
// entries for the desired IDs, but looks in the system database cache
88-
// first if there is one.
91+
// first if there is one. opts can be used to control which information should
92+
// be read, where the default is to WithDescriptor(true) and WithMetaData(true).
8993
GetByIDs(
9094
ctx context.Context,
9195
txn *kv.Txn,
9296
ids []descpb.ID,
9397
isDescriptorRequired bool,
9498
expectedType catalog.DescriptorType,
99+
opts ...GetByIDOption,
95100
) (nstree.Catalog, error)
96101

97102
// GetByNames reads the system.namespace entries for the given keys, but
@@ -109,6 +114,51 @@ func NewUncachedCatalogReader(codec keys.SQLCodec) CatalogReader {
109114
}
110115
}
111116

117+
// getByIDOptions options supported by GetByID.
118+
type getByIDOptions struct {
119+
withDescriptor bool
120+
withZoneConfig bool
121+
withComments bool
122+
}
123+
124+
// defaultGetByIDOptions are the default options for GetByID.
125+
var defaultGetByIDOptions = []GetByIDOption{WithDescriptor(true), WithMetaData(true)}
126+
127+
// withDefaultOptions are the default options for GetByID.
128+
func withDefaultOptions() []GetByIDOption {
129+
return defaultGetByIDOptions
130+
}
131+
132+
// GetByIDOption are options that GetByID supports.
133+
type GetByIDOption interface {
134+
apply(*getByIDOptions)
135+
}
136+
137+
func WithMetaData(b bool) GetByIDOption {
138+
return getByIDWithMetaData(b)
139+
}
140+
141+
func WithDescriptor(b bool) GetByIDOption {
142+
return getByIDWithDescriptor(b)
143+
}
144+
145+
// getByIDWithMetaData determines if metadata should be fetched.
146+
type getByIDWithMetaData bool
147+
148+
// apply implements GetByIDOption.
149+
func (g getByIDWithMetaData) apply(o *getByIDOptions) {
150+
o.withComments = bool(g)
151+
o.withZoneConfig = bool(g)
152+
}
153+
154+
// getByIDWithDescriptor determines if the descriptor should be fetched.
155+
type getByIDWithDescriptor bool
156+
157+
// apply implements GetByIDOption.
158+
func (g getByIDWithDescriptor) apply(o *getByIDOptions) {
159+
o.withDescriptor = bool(g)
160+
}
161+
112162
// catalogReader implements the CatalogReader interface by building catalogQuery
113163
// objects and running them, leveraging the SystemDatabaseCache if present.
114164
type catalogReader struct {
@@ -135,6 +185,11 @@ func (cr catalogReader) IsIDInCache(_ descpb.ID) bool {
135185
return false
136186
}
137187

188+
// IsCommentInCache is part of the CatalogReader interface.
189+
func (cr catalogReader) IsCommentInCache(_ descpb.ID) bool {
190+
return false
191+
}
192+
138193
// IsNameInCache is part of the CatalogReader interface.
139194
func (cr catalogReader) IsNameInCache(_ descpb.NameInfo) bool {
140195
return false
@@ -342,11 +397,19 @@ func (cr catalogReader) GetByIDs(
342397
ids []descpb.ID,
343398
isDescriptorRequired bool,
344399
expectedType catalog.DescriptorType,
400+
opts ...GetByIDOption,
345401
) (nstree.Catalog, error) {
346402
var mc nstree.MutableCatalog
347403
if len(ids) == 0 {
348404
return nstree.Catalog{}, nil
349405
}
406+
var options getByIDOptions
407+
if len(opts) == 0 {
408+
opts = withDefaultOptions()
409+
}
410+
for _, opt := range opts {
411+
opt.apply(&options)
412+
}
350413
cq := catalogQuery{
351414
codec: cr.codec,
352415
isDescriptorRequired: isDescriptorRequired,
@@ -359,22 +422,34 @@ func (cr catalogReader) GetByIDs(
359422
forEachDescriptorIDSpan(ids, func(startID descpb.ID, endID descpb.ID) {
360423
// Only a single descriptor run, so generate a Get request.
361424
if startID == endID {
362-
get(ctx, b, catalogkeys.MakeDescMetadataKey(codec, startID))
363-
for _, t := range catalogkeys.AllCommentTypes {
364-
scan(ctx, b, catalogkeys.MakeObjectCommentsMetadataPrefix(codec, t, startID))
425+
if options.withDescriptor {
426+
get(ctx, b, catalogkeys.MakeDescMetadataKey(codec, startID))
427+
}
428+
if options.withComments {
429+
for _, t := range catalogkeys.AllCommentTypes {
430+
scan(ctx, b, catalogkeys.MakeObjectCommentsMetadataPrefix(codec, t, startID))
431+
}
432+
}
433+
if options.withZoneConfig {
434+
get(ctx, b, config.MakeZoneKey(codec, startID))
365435
}
366-
get(ctx, b, config.MakeZoneKey(codec, startID))
367436
} else {
368437
// Otherwise, generate a Scan request instead. The end key is exclusive,
369438
// so we will need to increment the endID.
370-
scanRange(ctx, b, catalogkeys.MakeDescMetadataKey(codec, startID),
371-
catalogkeys.MakeDescMetadataKey(codec, endID+1))
372-
for _, t := range catalogkeys.AllCommentTypes {
373-
scanRange(ctx, b, catalogkeys.MakeObjectCommentsMetadataPrefix(codec, t, startID),
374-
catalogkeys.MakeObjectCommentsMetadataPrefix(codec, t, endID+1))
439+
if options.withDescriptor {
440+
scanRange(ctx, b, catalogkeys.MakeDescMetadataKey(codec, startID),
441+
catalogkeys.MakeDescMetadataKey(codec, endID+1))
442+
}
443+
if options.withComments {
444+
for _, t := range catalogkeys.AllCommentTypes {
445+
scanRange(ctx, b, catalogkeys.MakeObjectCommentsMetadataPrefix(codec, t, startID),
446+
catalogkeys.MakeObjectCommentsMetadataPrefix(codec, t, endID+1))
447+
}
448+
}
449+
if options.withZoneConfig {
450+
scanRange(ctx, b, config.MakeZoneKey(codec, startID),
451+
config.MakeZoneKey(codec, endID+1))
375452
}
376-
scanRange(ctx, b, config.MakeZoneKey(codec, startID),
377-
config.MakeZoneKey(codec, endID+1))
378453
}
379454
})
380455
})

0 commit comments

Comments
 (0)