sql: support ALTER INDEX … NOT VISIBLE by wenyihu6 · Pull Request #86032 · cockroachdb/cockroach

wenyihu6 · 2022-08-12T11:44:06Z

This commit adds support to execute ALTER INDEX … [VISIBLE | NOT VISIBLE].

See also: #85473

Release note (sql change): Altering an index to visible or not visible using
ALTER INDEX … VISIBLE | NOT VISIBLE is now supported.

cockroach-teamcity · 2022-08-12T11:44:15Z

This change is

wenyihu6 · 2022-08-12T11:58:49Z

The first two commits come from #85794, #85473.

michae2

Nice! This Let's see if we can get someone from @cockroachdb/sql-schema to take a look as well.

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2)

michae2

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @wenyihu6)

pkg/sql/alter_index_visible.go line 1 at r3 (raw file):

package sql

Above the package declaration, we need a copyright header:

// Copyright 2022 The Cockroach Authors.
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

Also, it looks like the crlfmt linter wants us to make the following changes:

    lint_test.go:1389:
        diff -u old/sql/alter_index_visible.go new/sql/alter_index_visible.go
        --- old/sql/alter_index_visible.go  2022-08-12 12:26:48.062773361 +0000
        +++ new/sql/alter_index_visible.go  2022-08-12 12:26:48.062773361 +0000
        @@ -2,6 +2,7 @@
         import (
           "context"
        +
           "github.com/cockroachdb/cockroach/pkg/sql/catalog"
           "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
           "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
        @@ -19,7 +20,9 @@
           index     catalog.Index
         }
        -func (p *planner) AlterIndexVisible(ctx context.Context, n *tree.AlterIndexVisible) (planNode, error) {
        +func (p *planner) AlterIndexVisible(
        +  ctx context.Context, n *tree.AlterIndexVisible,
        +) (planNode, error) {
           if err := checkSchemaChangeEnabled(
             ctx,
             p.ExecCfg(),

postamar

3rd commit LGTM, only nits

postamar · 2022-08-12T18:22:36Z

pkg/sql/alter_index_visible.go

+	if err := checkSchemaChangeEnabled(
+		ctx,
+		p.ExecCfg(),
+		"ALTER INDEX VISIBILITY",


nit: define a constant

Relates to #85473 (comment)

postamar · 2022-08-12T18:24:06Z

pkg/sql/alter_index_visible.go

+	// returned is nil. 2. No IfExists, error is returned.
+	_, tableDesc, err := expandMutableIndexName(ctx, p, &n.Index, !n.IfExists /* requireTable */)
+	if err != nil {
+		// Error if no table is found and IfExists is false.


nit: these kinds of comments add little value but increase clutter, remove them.

Same comment applies below.

postamar · 2022-08-12T18:26:21Z

pkg/sql/alter_index_visible.go

+			return newZeroNode(nil /* columns */), nil
+		}
+		// Error if no index exists and IfExists is not specified.
+		return nil, pgerror.WithCandidateCode(err, pgcode.UndefinedObject)


I suggest wrapping this in an assertion error here, expandMutableIndexName promises that if tableDesc is not nil, then the index exists, so an error here should never happen. Mentioning that in the comments would be useful.

postamar · 2022-08-12T18:29:30Z

pkg/sql/alter_index_visible.go

+		if n.n.NotVisible {
+			str = " not"
+		}
+		// TODO (wenyihu6): should we log notices if index is already what they want or is that too noisy?


cc @cockroachdb/sql-experience but I don't think so, if you do

CREATE TABLE t (i INT NOT NULL); ALTER TABLE t ALTER COLUMN i SET NOT NULL;

that ALTER is entirely redundant yet there's no notice.

wenyihu6 · 2022-08-12T18:35:42Z

pkg/sql/alter_index_visible.go line 1 at r3 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Above the package declaration, we need a copyright header:

// Copyright 2022 The Cockroach Authors.
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

Also, it looks like the crlfmt linter wants us to make the following changes:

    lint_test.go:1389:
        diff -u old/sql/alter_index_visible.go new/sql/alter_index_visible.go
        --- old/sql/alter_index_visible.go  2022-08-12 12:26:48.062773361 +0000
        +++ new/sql/alter_index_visible.go  2022-08-12 12:26:48.062773361 +0000
        @@ -2,6 +2,7 @@
         import (
           "context"
        +
           "github.com/cockroachdb/cockroach/pkg/sql/catalog"
           "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
           "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
        @@ -19,7 +20,9 @@
           index     catalog.Index
         }
        -func (p *planner) AlterIndexVisible(ctx context.Context, n *tree.AlterIndexVisible) (planNode, error) {
        +func (p *planner) AlterIndexVisible(
        +  ctx context.Context, n *tree.AlterIndexVisible,
        +) (planNode, error) {
           if err := checkSchemaChangeEnabled(
             ctx,
             p.ExecCfg(),

Done.

wenyihu6 · 2022-08-12T18:36:40Z

pkg/sql/alter_index_visible.go line 71 at r5 (raw file):

Previously, postamar (Marius Posta) wrote…

I suggest wrapping this in an assertion error here, expandMutableIndexName promises that if tableDesc is not nil, then the index exists, so an error here should never happen. Mentioning that in the comments would be useful.

I was also curious about this. When I tested it out, this errors when you specify the index name with a table that exists but the index does not exists. For example, t@idx returns an error if t exists but idx doesn't exist.

wenyihu6 · 2022-08-12T18:36:55Z

pkg/sql/alter_index_visible.go line 98 at r5 (raw file):

Previously, postamar (Marius Posta) wrote…

cc @cockroachdb/sql-experience but I don't think so, if you do
CREATE TABLE t (i INT NOT NULL);
ALTER TABLE t ALTER COLUMN i SET NOT NULL;
that ALTER is entirely redundant yet there's no notice.

That's true. I will remove this notice.

wenyihu6 · 2022-08-12T18:54:01Z

pkg/sql/alter_index_visible.go line 98 at r5 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

That's true. I will remove this notice.

Done.

wenyihu6 · 2022-08-12T19:00:44Z

pkg/sql/alter_index_visible.go line 39 at r5 (raw file):

Previously, postamar (Marius Posta) wrote…

nit: define a constant

Relates to #85473 (comment)

I'm not sure if I'm understanding correctly. Do you mean something like const feature = "ALTER INDEX VISIBILITY"? Other functions that call checkSchemaChangeEnabled don't seem to have this convention.

This commit adds support to execute ALTER INDEX … [VISIBLE | NOT VISIBLE]. Fixes: cockroachdb#72576 See also: cockroachdb#85473 Release note (sql change): Altering an index to visible or not visible using ALTER INDEX … VISIBLE | NOT VISIBLE is now supported.

wenyihu6 · 2022-08-13T13:56:07Z

TFTRs!!

bors r+

craig · 2022-08-13T15:15:15Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-13T16:42:04Z

Build succeeded:

Bazel Essential CI (Cockroach)

wenyihu6 requested a review from a team August 12, 2022 11:44

wenyihu6 requested a review from a team as a code owner August 12, 2022 11:44

wenyihu6 requested review from a team August 12, 2022 11:44

wenyihu6 requested a review from a team as a code owner August 12, 2022 11:44

wenyihu6 force-pushed the 5.5-alter-index-planner branch from e39f174 to 42f934f Compare August 12, 2022 11:56

wenyihu6 requested a review from michae2 August 12, 2022 11:57

michae2 approved these changes Aug 12, 2022

View reviewed changes

michae2 requested changes Aug 12, 2022

View reviewed changes

wenyihu6 force-pushed the 5.5-alter-index-planner branch from 42f934f to 45275a1 Compare August 12, 2022 18:21

postamar mentioned this pull request Aug 12, 2022

sql: add ALTER INDEX … NOT VISIBLE to parser #85473

Merged

postamar approved these changes Aug 12, 2022

View reviewed changes

wenyihu6 force-pushed the 5.5-alter-index-planner branch from 45275a1 to 045f1fc Compare August 12, 2022 18:52

michae2 approved these changes Aug 12, 2022

View reviewed changes

wenyihu6 force-pushed the 5.5-alter-index-planner branch from 045f1fc to 17ecc2f Compare August 13, 2022 13:54

wenyihu6 force-pushed the 5.5-alter-index-planner branch from 17ecc2f to 15613bb Compare August 13, 2022 13:55

craig bot merged commit 384a281 into cockroachdb:master Aug 13, 2022

cockroach-teamcity mentioned this pull request Aug 13, 2022

sql: support ALTER INDEX … NOT VISIBLE cockroachdb/docs#14815

Closed

wenyihu6 deleted the 5.5-alter-index-planner branch September 1, 2022 20:48

Conversation

wenyihu6 commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Aug 12, 2022

Uh oh!

wenyihu6 commented Aug 12, 2022

Uh oh!

michae2 left a comment

Choose a reason for hiding this comment

Uh oh!

michae2 left a comment

Choose a reason for hiding this comment

Uh oh!

postamar left a comment

Choose a reason for hiding this comment

Uh oh!

postamar Aug 12, 2022

Choose a reason for hiding this comment

Uh oh!

postamar Aug 12, 2022

Choose a reason for hiding this comment

Uh oh!

postamar Aug 12, 2022

Choose a reason for hiding this comment

Uh oh!

postamar Aug 12, 2022

Choose a reason for hiding this comment

Uh oh!

wenyihu6 commented Aug 12, 2022

Uh oh!

wenyihu6 commented Aug 12, 2022

Uh oh!

wenyihu6 commented Aug 12, 2022

Uh oh!

wenyihu6 commented Aug 12, 2022

Uh oh!

wenyihu6 commented Aug 12, 2022

Uh oh!

wenyihu6 commented Aug 13, 2022

Uh oh!

craig bot commented Aug 13, 2022

Uh oh!

craig bot commented Aug 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wenyihu6 commented Aug 12, 2022 •

edited

Loading