sql/schemachanger: enable declarative schema changer to read comments from db #80380

chengxiong-ruan · 2022-04-22T14:28:27Z

This commit adds a metadata fetcher interface to read comments
from db during descriptor elements walking. An implementation
of the interface is also added and plumbed into the descriptor
decomposition.

Release note: None

cockroach-teamcity · 2022-04-22T14:28:35Z

This change is

chengxiong-ruan · 2022-04-22T14:31:50Z

@postamar I decided to break this out from #78025 which I think it's hard enough to review even it was in a separate commit, I found it's really hard to make sure all test pass at each commit because the commits all kinda touching the same tests....

postamar

Thanks for breaking this out. I think you did the right thing, there's a lot here, and it will make the COMMENT ON pull request a lot more straightforward to review.

I have a few objections, nothing too deep so don't wait for me to get back from vacation to merge this after addressing them.

Reviewed 33 of 40 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan)

pkg/sql/descmetadata/comment_cache.go, line 35 at r2 (raw file):

	ie            sqlutil.InternalExecutor
	comments      map[scdecomp.CommentKey]string
	objIDsChecked map[catid.DescID]struct{}

nit: consider using a catalog.DescriptorIDSet instead?

pkg/sql/descmetadata/comment_cache.go, line 110 at r2 (raw file):

		"mf-get-table-comments",
		mf.txn,
		sessiondata.InternalExecutorOverride{User: security.RootUserName()},

Just curious, why do we need to query this table as root? Is this how it's done in the legacy schema changer? (I haven't checked)

pkg/sql/schemachanger/scbuild/builder_state.go, line 432 at r2 (raw file):

			}
		case catalog.Schema:
			if err := b.commentCache.LoadCommentsForObjects(b.ctx, catalog.Table, c.backrefs.Ordered()); err != nil {

Schemas also contain types, not just tables. Why does LoadCommentsForObjects require a descriptor type anyway? I don't understand the purpose of this filtering. Let's just warm the cache for all the back-referenced things, no?

pkg/sql/schemachanger/scdecomp/decomp.go, line 63 at r2 (raw file):

	desc catalog.Descriptor,
	lookupFn func(id catid.DescID) catalog.Descriptor,
	ev ElementVisitor,

Consider moving the ElementVisitor definition to this new dependencies.go file, it seems like it would belong there.

pkg/sql/schemachanger/scdecomp/dependencies.go, line 26 at r2 (raw file):

type CommentKey struct {
	ObjectID    catid.DescID
	SubID       descpb.ID

Shouldn't this also be a catid.DescID?

pkg/sql/schemachanger/scdecomp/dependencies.go, line 28 at r2 (raw file):

	SubID       descpb.ID
	CommentType keys.CommentType
}

Why is this type defined here in scdecomp at all?

pkg/sql/schemachanger/scdecomp/dependencies.go, line 32 at r2 (raw file):

// DescriptorCommentCache represent an interface to fetch metadata like
// comment for descriptors.
type DescriptorCommentCache interface {

The descriptor decomposition logic does not care that this is a cache, as far as it's concerned that's purely an implementation detail. I'm aware that's not the case for scbuild though.

What do you think of having an interface here that just does Gets and rename it CommentGetter or something. Then in the builder define another named CommentCache with the LoadCommentsForObjects method and which embeds CommentGetter?

pkg/sql/schemachanger/scdecomp/dependencies.go, line 35 at r2 (raw file):

	// Get returns comment for a (objID, subID, commentType) tuple if exists. "ok"
	// returned indicates if the comment actually exists or not.
	Get(ctx context.Context, objID catid.DescID, subID descpb.ID, commentType keys.CommentType) (comment string, ok bool, err error)

How about replacing this one method with several, one for each comment type?GetDatabaseComment(ctx, dbID) etc.

No need to have the system.comment table schema leak everywhere.

pkg/sql/schemachanger/scdeps/sctestdeps/database_state.go, line 200 at r2 (raw file):

	}
	return comments
}

This is fine but have you considered instead having a comment cache constructor in descmetadata which warms the cache by doing a SELECT * FROM system.comments LIMIT 1000 or something like that?

This way you could re-use it in the test deps (no way we'd ever have more than 1k comments) plus you would amortize a lot of round trips in prod for the common case where there are at most very few comments.

pkg/sql/schemachanger/scplan/plan_test.go, line 60 at r2 (raw file):

		run := func(t *testing.T, d *datadriven.TestData) string {
			// TODO (Chengxiong): make this switch block to have only "build" and
			// "ops/deps" sections.

I suggest you either follow up with a PR shortly or file an issue to track this, otherwise we'll forget about it.

pkg/sql/schemachanger/scplan/plan_test.go, line 106 at r2 (raw file):

					default:
						t.Fatal("not a supported descriptor metadata statement")
					}

Why have a switch block at all? Instead perhaps just tdb.Exec(t, stmt.SQL)?

Same comment applies elsewhere in this PR.

chengxiong-ruan

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

pkg/sql/descmetadata/comment_cache.go, line 35 at r2 (raw file):