sql/schemachanger: Implement comment on in declarative sc #80459

chengxiong-ruan · 2022-04-24T21:58:53Z

Split out from #78025

This commit implements COMMENT ON with the new schema changer
framework.

Release note: None

cockroach-teamcity · 2022-04-24T21:59:03Z

This change is

ajwerner

Let's add some logic tests in the new_schema_changer logictest file or a new test file that has the same config. Then, let's try adding some pernicious cases like adding a comment and then dropping the thing it references in the same transaction. Also, let's do a sequence of adding and dropping comments in the same transaction.

Reviewed 30 of 30 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan and @fqazi)

pkg/sql/schemachanger/scbuild/builder_state.go line 718 at r1 (raw file):

		}
	})
	scpb.ForEachColumn(c.ers, func(current scpb.Status, target scpb.TargetStatus, e *scpb.Column) {

one day, these are going to hurt. We'll probably want to leverage rel indexes here. It's easy to see how this could become quadratic

Code quote:

	scpb.ForEachColumnName(c.ers, func(status scpb.Status, _ scpb.TargetStatus, e *scpb.ColumnName) {
		if e.TableID == relationID && tree.Name(e.Name) == columnName {
			columnID = e.ColumnID
		}
	})
	scpb.ForEachColumn(c.ers, func(current scpb.Status, target scpb.TargetStatus, e *scpb.Column) {
		if e.TableID == relationID && e.ColumnID == columnID {
			pgAttrNum = e.PgAttributeNum
		}
	})

pkg/sql/schemachanger/scbuild/dependencies.go line 106 at r1 (raw file):

	// MayResolveIndex looks up an index using a naked index name with database
	// and schema prefix. Resolved prefix, index and the owner table are returned.
	MayResolveIndex(ctx context.Context, indexName tree.Name, prefix tree.ObjectNamePrefix) (catalog.ResolvedObjectPrefix, catalog.TableDescriptor, catalog.Index)

can you use more words to talk about the structure of this name? Namely, this seems to support the @ syntax no?

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go line 47 at r1 (raw file):

var supportedStatements = map[reflect.Type]supportedStatement{
	// Alter table will have commands individually whitelisted via the
	// supportedAlterTableStatements list, so wwe will consider it fully supported

drive-by: wwe

pkg/sql/schemachanger/scdeps/build_deps.go line 142 at r1 (raw file):

	ctx context.Context, indexName tree.Name, prefix tree.ObjectNamePrefix,
) (catalog.ResolvedObjectPrefix, catalog.TableDescriptor, catalog.Index) {
	found, resolvedPrefix, err := resolver.ResolveObjectNamePrefix(

I don't think this universally works, consider:

Setup:

CREATE DATABASE db;
USE db;

CREATE SCHEMA sc1;
CREATE SCHEMA sc2;
SET search_path = sc1, sc2;
CREATE TABLE sc2.t (i INT PRIMARY KEY, j INT);
CREATE INDEX idx ON sc2.t(j);

Things which should work and should apply to sc2.t@idx:

COMMENT ON INDEX idx IS 'a';

COMMENT ON INDEX t@idx IS 'a';

I think the first one is broken today in cockroach but the second one works.

In general, I think this method has the wrong types. I think it should use tree.TableIndexName as the parameter. Can you write some logic tests to cover the above cases?

pkg/sql/schemachanger/scdeps/helper.go line 27 at r1 (raw file):

// index. dsNames and dsIDs are parallel slices containing table names and ids,
// they must have same length.
func FindTableContainsIndex(

this belongs elsewhere. I think a reasonable place would be the resolver package, then in scdeps, just deal with error translation.

In general, a principle should be that if you're writing business logic in scdeps, you're doing something wrong. Ideally we could factor out some of the existing logic from the sql package and leverage it.

pkg/sql/schemachanger/scplan/internal/opgen/opgen_schema_comment.go line 29 at r1 (raw file):

					}
				}),
				// TODO(Chengxiong): add schema event log (need to add proto for schema

can you file an issue for this?

pkg/sql/schemachanger/scplan/testdata/comment_on line 150 at r1 (raw file):

          tableId: 107
      Statement: COMMENT ON CONSTRAINT ‹t1_amount_gt_10› ON ‹db1›.‹sc1›.‹t1› IS 'this is
        a rule'

this line wrap is suspect. How does it happen?

Code quote:


        a rule'

chengxiong-ruan

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @chengxiong-ruan, and @fqazi)

pkg/sql/schemachanger/scdeps/build_deps.go line 142 at r1 (raw file):

I don't think this universally works, consider:

Yeah, this is weird, I only tested it with one schema on the search path. So looks like we should iterate through all schema on the search path. I'm glad I'm learning something here. I guess it's a broader issue we should fix for crdb to be more compatible with postgres. Do you know if there's an issue tracking this?

In general, I think this method has the wrong types. I think it should use tree.TableIndexName as the parameter.

My thought was that this function only handle cases where a table name is not given and we try to find an index and a table has this index given whatever prefix we have, this for cases like: db_name.sc_name.index_name, db_name.index_name, sc_name.index_name, so that we can resolve table elements with the table we found, and then get index elements from there.

For cases where a table name is given, like tble_name@index we may just resolve the table first and find an index elements from it.

Does this make sense to you? I assume you're asking having this function handle both categories.

pkg/sql/schemachanger/scdeps/helper.go line 27 at r1 (raw file):

Previously, ajwerner wrote…

this belongs elsewhere. I think a reasonable place would be the resolver package, then in scdeps, just deal with error translation.

In general, a principle should be that if you're writing business logic in scdeps, you're doing something wrong. Ideally we could factor out some of the existing logic from the sql package and leverage it.

Yes, indeed. Will move it to a more common spot.

pkg/sql/schemachanger/scplan/internal/opgen/opgen_schema_comment.go line 29 at r1 (raw file):

Previously, ajwerner wrote…

can you file an issue for this?

yes, #80576

pkg/sql/schemachanger/scplan/testdata/comment_on line 150 at r1 (raw file):

Previously, ajwerner wrote…

this line wrap is suspect. How does it happen?

interesting, you absolutely have good eyes! Will figure this out.

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @chengxiong-ruan, and @fqazi)

pkg/sql/schemachanger/scdeps/build_deps.go line 142 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

I don't think this universally works, consider:

Yeah, this is weird, I only tested it with one schema on the search path. So looks like we should iterate through all schema on the search path. I'm glad I'm learning something here. I guess it's a broader issue we should fix for crdb to be more compatible with postgres. Do you know if there's an issue tracking this?

In general, I think this method has the wrong types. I think it should use tree.TableIndexName as the parameter.

My thought was that this function only handle cases where a table name is not given and we try to find an index and a table has this index given whatever prefix we have, this for cases like: db_name.sc_name.index_name, db_name.index_name, sc_name.index_name, so that we can resolve table elements with the table we found, and then get index elements from there.

For cases where a table name is given, like tble_name@index we may just resolve the table first and find an index elements from it.

Does this make sense to you? I assume you're asking having this function handle both categories.

I'm not sure. It feels like tree.TableIndexName can store all of the cases. It might be poorly named though. Perhaps we should have some structure which is a combination of the more general ObjectName and the index specification. Can you imagine a scenario where we'd ever want to resolve an index and not support all of these syntaxes?

chengxiong-ruan · 2022-04-26T19:24:47Z

pkg/sql/schemachanger/scdeps/build_deps.go line 142 at r1 (raw file):

Previously, ajwerner wrote…

I'm not sure. It feels like tree.TableIndexName can store all of the cases. It might be poorly named though. Perhaps we should have some structure which is a combination of the more general ObjectName and the index specification. Can you imagine a scenario where we'd ever want to resolve an index and not support all of these syntaxes?

Ha I'm terrible at naming as well. I do want to support all of the cases and I suppose you're talking about something like this I did in pr (I just copied the code to quote..) , I can push it to a lower layer though:

func CommentOnIndex(b BuildCtx, n *tree.CommentOnIndex) {
	var tableID catid.DescID
	var tableElements ElementResultSet
	if len(n.Index.Table.ObjectName) > 0 {
		tableElements = b.ResolveTable(n.Index.Table.ToUnresolvedObjectName(), commentResolveParams)
	} else {
		tableElements = b.ResolveTableWithIndexBestEffort(n.Index.Table.ObjectNamePrefix, tree.Name(n.Index.Index), commentResolveParams)
	}

chengxiong-ruan · 2022-04-26T19:25:18Z

pkg/sql/schemachanger/scdeps/build_deps.go line 142 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Ha I'm terrible at naming as well. I do want to support all of the cases and I suppose you're talking about something like this I did in pr (I just copied the code to quote..) , I can push it to a lower layer though:
func CommentOnIndex(b BuildCtx, n *tree.CommentOnIndex) {
	var tableID catid.DescID
	var tableElements ElementResultSet
	if len(n.Index.Table.ObjectName) > 0 {
		tableElements = b.ResolveTable(n.Index.Table.ToUnresolvedObjectName(), commentResolveParams)
	} else {
		tableElements = b.ResolveTableWithIndexBestEffort(n.Index.Table.ObjectNamePrefix, tree.Name(n.Index.Index), commentResolveParams)
	}

where n.Index is a TableIndexName

chengxiong-ruan

Good callouts, I'll add them in a separate pr in which the fullysupported will be turned on and this implementation will be guarded behind a version gate.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @fqazi)

pkg/sql/schemachanger/scbuild/builder_state.go line 718 at r1 (raw file):

Previously, ajwerner wrote…

one day, these are going to hurt. We'll probably want to leverage rel indexes here. It's easy to see how this could become quadratic

yeah, I've decided to make the ColumnComment in memory to use column ID to identify it, but use PgAttributeNum for reading/updating/deleting. I kinda shoot myself on the feet by using PgAttributeNum as an identifier. With that said, all these logic checking PgAttributeNum is gone.

pkg/sql/schemachanger/scbuild/dependencies.go line 106 at r1 (raw file):

Previously, ajwerner wrote…

can you use more words to talk about the structure of this name? Namely, this seems to support the @ syntax no?

this is done after the refactoring.

chengxiong-ruan · 2022-05-05T04:21:24Z

pkg/sql/schemachanger/scplan/testdata/comment_on line 150 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

interesting, you absolutely have good eyes! Will figure this out.

as discussed before, this is a yaml bug: go-yaml/yaml#166

fqazi

Great work, just minor comments, they're not functional, just clean up.

Reviewed 32 of 32 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @chengxiong-ruan)

pkg/sql/alter_table.go line 1383 at r2 (raw file):

		keys.ColumnCommentType,
		tableID,
		pgAttributeNum)

As a separate issue do we want a migration to clean up orphan entries?

pkg/sql/catalog/tabledesc/table.go line 313 at r2 (raw file):

		}
	}
	return descpb.ConstraintDetail{}, nil

Tempted to say we should return a pointer and use nil as the canary value, just feels safer.

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

	return c.ers.Filter(func(_ scpb.Status, _ scpb.TargetStatus, e scpb.Element) bool {
		idI, _ := screl.Schema.GetAttribute(screl.ConstraintID, e)

Can we just simplify this down to a filter? On the TableID and ConstraintId attributes? Then check if the element set is empty after only?

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/comment_on.go line 81 at r2 (raw file):

	if n.Comment == nil {
		tableElements.ForEachElementStatus(func(_ scpb.Status, _ scpb.TargetStatus, e scpb.Element) {

Replace this with FindTableComment

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/comment_on.go line 91 at r2 (raw file):

			Comment: *n.Comment,
		}
		tableElements.ForEachElementStatus(func(_ scpb.Status, _ scpb.TargetStatus, e scpb.Element) {

Replace this with FindTable (similar in other spots where there is only one possibility)

pkg/sql/logictest/testdata/logic_test/new_schema_changer line 430 at r2 (raw file):

DROP TABLE foo

subtest multi_table

Wait, were these tests nuked or am I reading the diff wrong?

chengxiong-ruan

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @fqazi)

pkg/sql/alter_table.go line 1383 at r2 (raw file):