Skip to content

sql: fix ALTER INDEX IF EXISTS for non-existing index#42797

Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom
rafiss:rename-index-exists
Nov 27, 2019
Merged

sql: fix ALTER INDEX IF EXISTS for non-existing index#42797
craig[bot] merged 2 commits intocockroachdb:masterfrom
rafiss:rename-index-exists

Conversation

@rafiss
Copy link
Copy Markdown
Collaborator

@rafiss rafiss commented Nov 26, 2019

Previously, if ALTER INDEX IF EXISTS was used on an
unqualified index name that did not match any index, the database would
return an "index does not exist" error.

Now, the statement succeeds, but does nothing.

Also, test edge cases around ambiguous, non-existing, and/or unqualified
index names, in both alter_index and drop_index logic tests.

Fixes #42399

Release note (bug fix): ALTER INDEX IF EXISTS no longer fails
when using an unqualified index name that does not match any existing
index. Now it is a no-op.

@rafiss rafiss requested review from a team, apantel and jordanlewis November 26, 2019 19:13
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@rafiss rafiss force-pushed the rename-index-exists branch from e079377 to 94b5e79 Compare November 26, 2019 19:23
@rafiss rafiss changed the title sql: fix ALTER INDEX IF NOT EXISTS for non-existing index sql: fix ALTER INDEX IF EXISTS for non-existing index Nov 26, 2019
Copy link
Copy Markdown
Member

@jordanlewis jordanlewis left a comment

Choose a reason for hiding this comment

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

:lgtm: seems backport worthy!

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @apantel)

@@ -36,6 +36,10 @@ type renameIndexNode struct {
func (p *planner) RenameIndex(ctx context.Context, n *tree.RenameIndex) (planNode, error) {
_, tableDesc, err := expandMutableIndexName(ctx, p, n.Index, true /* requireTable */)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you accomplish the same thing by passing requireTable=false here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I actually ended up making this more similar to drop_index.go, and set requireTable=!n.IfExists

Previously, if ALTER INDEX IF EXISTS (AIIE) was used on an
unqualified index name that did not match any index, the database would
return an "index does not exist" error.

Now, the statement succeeds, but does nothing.

Release note (bug fix): ALTER INDEX IF EXISTS no longer fails
when using an unqualified index name that does not match any existing
index. Now it is a no-op.
Test edge cases around ambiguous, non-existing, and/or unqualified
index names, to reach test parity with alter_index logic test.

Release note: None
@rafiss rafiss force-pushed the rename-index-exists branch from 94b5e79 to fa4cbe5 Compare November 26, 2019 21:25
@rafiss rafiss requested a review from apantel November 26, 2019 21:45
Copy link
Copy Markdown
Contributor

@apantel apantel left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @apantel and @jordanlewis)

@rafiss
Copy link
Copy Markdown
Collaborator Author

rafiss commented Nov 26, 2019

thanks for the reviews!

bors r+

@rafiss
Copy link
Copy Markdown
Collaborator Author

rafiss commented Nov 27, 2019

bors r-

@rafiss
Copy link
Copy Markdown
Collaborator Author

rafiss commented Nov 27, 2019

bors r+

craig bot pushed a commit that referenced this pull request Nov 27, 2019
42797: sql: fix ALTER INDEX IF EXISTS for non-existing index r=rafiss a=rafiss

Previously, if ALTER INDEX IF EXISTS was used on an
unqualified index name that did not match any index, the database would
return an "index does not exist" error.

Now, the statement succeeds, but does nothing.

Also, test edge cases around ambiguous, non-existing, and/or unqualified
index names, in both alter_index and drop_index logic tests.

Fixes #42399 

Release note (bug fix): ALTER INDEX IF EXISTS no longer fails
when using an unqualified index name that does not match any existing
index. Now it is a no-op.

42831: colexec: fix a bug with top K sort not handling K > 1024 r=yuzefovich a=yuzefovich

Previously, top K sorter when emitting the data would copy always from
the beginning of the stored vectors. This is incorrect behavior when
K is greater than coldata.BatchSize(), and now this has been fixed.

Fixes: #42816.

Release note (bug fix): A bug with incorrect handling of Top K sort by
the vectorized engine when K is greater than 1024 has been fixed.

42835: workload, roachtest: skip TPCH Q4 for vec on r=yuzefovich a=yuzefovich

TPCH query 4 in some cases can hit a memory limit error, so we will skip
it for now (until the disk spilling is put in place).

Release note: None

Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Nov 27, 2019

Build succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ALTER INDEX IF EXISTS does not seem to work

4 participants