Skip to content

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

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
wenyihu6:5-alter-index
Aug 13, 2022
Merged

sql: add ALTER INDEX … NOT VISIBLE to parser#85473
craig[bot] merged 1 commit intocockroachdb:masterfrom
wenyihu6:5-alter-index

Conversation

@wenyihu6
Copy link
Copy Markdown
Contributor

@wenyihu6 wenyihu6 commented Aug 2, 2022 •
edited
Loading

This commit adds parsing support for ALTER INDEX … [VISIBLE | NOT VISIBLE].
Executing the command returns an unimplemented error.

Assists: #72576

See also: #85794

Release note (sql change): Parser now supports altering an index to visible or
not visible. But no implementation has done yet, and executing it returns an
“unimplemented” error immediately.

@wenyihu6 wenyihu6 changed the title 5 alter index sql: support ALTER INDEX … NOT VISIBLE Aug 2, 2022
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Aug 2, 2022

This change is Reviewable

@wenyihu6 wenyihu6 force-pushed the 5-alter-index branch 17 times, most recently from 5d66fc0 to 144073e Compare August 5, 2022 16:23
@wenyihu6 wenyihu6 marked this pull request as ready for review August 5, 2022 16:24
@wenyihu6 wenyihu6 requested a review from a team August 5, 2022 16:24
@wenyihu6 wenyihu6 requested a review from a team as a code owner August 5, 2022 16:24
@wenyihu6 wenyihu6 requested review from a team August 5, 2022 16:24
@wenyihu6 wenyihu6 requested a review from a team as a code owner August 5, 2022 16:24
@wenyihu6 wenyihu6 requested a review from msbutler August 5, 2022 16:24
@wenyihu6
Copy link
Copy Markdown
Contributor Author

wenyihu6 commented Aug 5, 2022

The first commit comes from #85239. The second commit comes from #85354.

@wenyihu6 wenyihu6 requested review from mgartner and michae2 August 5, 2022 16:25
@wenyihu6 wenyihu6 changed the title sql: support ALTER INDEX … NOT VISIBLE sql: add ALTER INDEX … NOT VISIBLE to parser Aug 9, 2022
@wenyihu6 wenyihu6 force-pushed the 5-alter-index branch 3 times, most recently from 816ef6c to e624798 Compare August 11, 2022 15:29
michae2
michae2 approved these changes Aug 12, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 18 of 18 files at r9, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @wenyihu6)

pkg/sql/alter_index_visible.go line 28 at r9 (raw file):

// and expects to see its own writes. But I'm not certain since renameIndexNode
// is also implementing this interface method.
// func (n *alterIndexVisibleNode) ReadingOwnWrites() {}

It might be necessary for something like this to work:

BEGIN;
CREATE INDEX myidx ON t (a);
ALTER INDEX myidx NOT VISIBLE;
COMMIT;

Or maybe something like this:

BEGIN;
ALTER INDEX myidx NOT VISIBLE;
ALTER INDEX myidx VISIBLE;
COMMIT;

@michae2 michae2 requested a review from a team August 12, 2022 00:25
@wenyihu6 wenyihu6 mentioned this pull request Aug 12, 2022
Merged
wenyihu6 added a commit to wenyihu6/cockroach that referenced this pull request Aug 12, 2022
@wenyihu6
sql: support ALTER INDEX … NOT VISIBLE
42f934f 
This commit adds support to execute ALTER INDEX … [VISIBLE | NOT VISIBLE].

Assists: 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.
postamar
postamar approved these changes Aug 12, 2022
View reviewed changes
Copy link
Copy Markdown

@postamar postamar left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2nd commit LGTM

wenyihu6 added a commit to wenyihu6/cockroach that referenced this pull request Aug 12, 2022
@wenyihu6
sql: support ALTER INDEX … NOT VISIBLE
45275a1 
This commit adds support to execute ALTER INDEX … [VISIBLE | NOT VISIBLE].

Assists: 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
Copy link
Copy Markdown
Contributor Author

wenyihu6 commented Aug 12, 2022

pkg/sql/alter_index_visible.go line 28 at r9 (raw file):

Previously, michae2 (Michael Erickson) wrote…

It might be necessary for something like this to work:

BEGIN;
CREATE INDEX myidx ON t (a);
ALTER INDEX myidx NOT VISIBLE;
COMMIT;

Or maybe something like this:

BEGIN;
ALTER INDEX myidx NOT VISIBLE;
ALTER INDEX myidx VISIBLE;
COMMIT;

Thank you! That makes sense.

I found a bit more context here.

// If the planNode also implements the nodeReadingOwnWrites interface,
// the txn is temporarily reconfigured to use read-your-own-writes for
// the duration of the call to startExec. This is used e.g. by
// DDL statements.

I wasn't sure if ReadingOwnWrites refers to two statements within one transaction or reading what have been changed within one statement (for example, alter table can have a list of commands). Looks like it is the first one based on the comment here.

@wenyihu6
Copy link
Copy Markdown
Contributor Author

wenyihu6 commented Aug 12, 2022

pkg/sql/opt/exec/execbuilder/testdata/not_visible_index line 1159 at r11 (raw file):

Previously, postamar (Marius Posta) wrote…

nit: define a subtest here?

Done.

wenyihu6 added a commit to wenyihu6/cockroach that referenced this pull request Aug 12, 2022
@wenyihu6
sql: support ALTER INDEX … NOT VISIBLE
045f1fc 
This commit adds support to execute ALTER INDEX … [VISIBLE | NOT VISIBLE].

Assists: 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
sql: add ALTER INDEX … NOT VISIBLE to parser
70fa8cf 
This commit adds parsing support for ALTER INDEX … [VISIBLE | NOT VISIBLE].
Executing the command returns an `unimplemented error`.

Assists: cockroachdb#72576

See also: cockroachdb#85794

Release note (sql change): Parser now supports altering an index to visible or
not visible. But no implementation has done yet, and executing it returns an
“unimplemented” error immediately.

# Conflicts:
#	pkg/sql/sem/tree/stmt.go
@wenyihu6
Copy link
Copy Markdown
Contributor Author

wenyihu6 commented Aug 13, 2022

TFTRs!!

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 13, 2022

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 13, 2022

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 13, 2022

Build succeeded:

@craig craig bot merged commit 7726a96 into cockroachdb:master Aug 13, 2022
wenyihu6 added a commit to wenyihu6/cockroach that referenced this pull request Aug 13, 2022
@wenyihu6
sql: support ALTER INDEX … NOT VISIBLE
15613bb 
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.
@cockroach-teamcity cockroach-teamcity mentioned this pull request Aug 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michae2 michae2 michae2 approved these changes

@mgartner mgartner Awaiting requested review from mgartner

+1 more reviewer

@postamar postamar postamar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wenyihu6 @cockroach-teamcity @postamar @michae2