sql: ALTER COLUMN TYPE does not check partial index definitions by Dedej-Bergin · Pull Request #131590 · cockroachdb/cockroach

Dedej-Bergin · 2024-09-30T13:32:08Z

Previously we would allow ALTER COLUMN TYPE to succeed on a column even when we have a partial index on it. This would cause issues down the line for example at insert and is inconsistent with postgres. This fix now checks for partial index and errors out like postgres when your run the ALTER COLUMN TYPE command on a column with a partial index.

Fixes: #72456
Epic: CRDB-25314

Release note (bug fix): ALTER COLUMN TYPE now errors out when we have a partial index dependent on the column being altered.

blathers-crl · 2024-09-30T13:32:13Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2024-09-30T13:32:19Z

This change is

spilchen

You were on the right track. I just had a couple of suggestions for modifications.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Dedej-Bergin)

-- commits line 11 at r1:
Let's include the epic in the commit message: CRDB-25314

pkg/sql/catalog/schemaexpr/expr.go line 518 at r1 (raw file):

) error {
	for _, index := range tableDesc.AllIndexes() {
		if index.IsPartial() {

Should we verify that the column is included in the partial index?

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1218 at r1 (raw file):

DROP TABLE t_int;

subtest end

I usually aim to include a single subtest end at the bottom of the test. Since you've already added one, you can probably remove this line.

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1223 at r1 (raw file):


statement ok
set enable_experimental_alter_column_type_general='true';

I don't think we need this statement. We already call this higher up in the testcase.

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1224 at r1 (raw file):

statement ok
set enable_experimental_alter_column_type_general='true';
set use_declarative_schema_changer = off;

I think we call this test for the local-legacy-schema-changer config. So, I don't think we need this.

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1233 at r1 (raw file):


statement error pq: cannot alter type of column "b" because it is referenced by partial index "idx"
alter table a alter column b set data type timestamp  using a::timestamp;

We should expand this test and verify that we can alter the type of other columns not included in the partial index.

Dedej-Bergin

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

-- commits line 11 at r1:

Previously, spilchen wrote…

Let's include the epic in the commit message: CRDB-25314

Thanks, just updated it.

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1218 at r1 (raw file):

Previously, spilchen wrote…

I usually aim to include a single subtest end at the bottom of the test. Since you've already added one, you can probably remove this line.

Thanks, I just removed it.

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1223 at r1 (raw file):

Previously, spilchen wrote…

I don't think we need this statement. We already call this higher up in the testcase.

Okay great, I just removed it, thanks!

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1224 at r1 (raw file):

Previously, spilchen wrote…

I think we call this test for the local-legacy-schema-changer config. So, I don't think we need this.

Right, I just added onlyif config local-legacy-schema-changer just now and removed this line, thanks!

spilchen

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Dedej-Bergin)

-- commits line 11 at r1:

Previously, Dedej-Bergin (Bergin Dedej) wrote…

Thanks, just updated it.

Apologies, I meant to say that you should include this in the commit message. I thought there was a git hook in place to ensure the commit message follows the correct format.

Epic: CRDB-25314

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1224 at r1 (raw file):

Previously, Dedej-Bergin (Bergin Dedej) wrote…

Right, I just added onlyif config local-legacy-schema-changer just now and removed this line, thanks!

We should be seeing the same error when the commands are run through the DSC. Are you observing a different outcome?

Dedej-Bergin · 2024-10-01T19:58:16Z

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1224 at r1 (raw file):

Previously, spilchen wrote…

We should be seeing the same error when the commands are run through the DSC. Are you observing a different outcome?

Yes a bit different, for the new code changes I just added, with these repro steps:

create table roach\_village ( roach\_id int primary key, first\_name text, last\_name text, legs int );
create index idx on roach\_village(last\_name) where first\_name = 'papa roach';
alter table roach\_village alter column first\_name set data type timestamp  using first\_name::timestamp;
alter table roach\_village alter column last\_name set data type timestamp  using last\_name::timestamp;

I was thinking to make it work like this:

For DSC on the second one it seems to error out with unimplemented... not sure if that's the error message we want:

Dedej-Bergin

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

-- commits line 11 at r1:

Previously, spilchen wrote…

Apologies, I meant to say that you should include this in the commit message. I thought there was a git hook in place to ensure the commit message follows the correct format.
Epic: CRDB-25314

Just added this, for some reason I thought this happens automatically.

pkg/sql/catalog/schemaexpr/expr.go line 518 at r1 (raw file):

Previously, spilchen wrote…

Should we verify that the column is included in the partial index?

Yes just added this code, thanks @spilchen!

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1233 at r1 (raw file):

Previously, spilchen wrote…

We should expand this test and verify that we can alter the type of other columns not included in the partial index.

Just added the expanded test, thanks!

spilchen

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Dedej-Bergin)

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1224 at r1 (raw file):

Previously, Dedej-Bergin (Bergin Dedej) wrote…

Yes a bit different, for the new code changes I just added, with these repro steps:
create table roach\_village ( roach\_id int primary key, first\_name text, last\_name text, legs int );
create index idx on roach\_village(last\_name) where first\_name = 'papa roach';
alter table roach\_village alter column first\_name set data type timestamp  using first\_name::timestamp;
alter table roach\_village alter column last\_name set data type timestamp  using last\_name::timestamp;
I was thinking to make it work like this:

For DSC on the second one it seems to error out with unimplemented... not sure if that's the error message we want:

The unimplemented error is expected for now—that’s what #127014 will address. In the meantime, can you try using a simpler type conversion instead of a complex one? A simple conversion is one that doesn’t require a backfill. This is supported in the DSC now and should let us run the test in both modes. For example, converting from TEXT to CHAR(20) would qualify as a simple conversion.

Dedej-Bergin

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

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1224 at r1 (raw file):

Previously, spilchen wrote…

The unimplemented error is expected for now—that’s what #127014 will address. In the meantime, can you try using a simpler type conversion instead of a complex one? A simple conversion is one that doesn’t require a backfill. This is supported in the DSC now and should let us run the test in both modes. For example, converting from TEXT to CHAR(20) would qualify as a simple conversion.

So it seems that DSC doesn't block when the column is part of the predicate.

Dedej-Bergin · 2024-10-02T20:37:36Z

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1224 at r1 (raw file):

Previously, Dedej-Bergin (Bergin Dedej) wrote…

So it seems that DSC doesn't block when the column is part of the predicate.

Actually we seem to error out on the predicate, but on the column that has the index we seem to not error out on DSC.
Is this something we should change in the DSC?

spilchen

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Dedej-Bergin)

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1224 at r1 (raw file):

Previously, Dedej-Bergin (Bergin Dedej) wrote…

Actually we seem to error out on the predicate, but on the column that has the index we seem to not error out on DSC.
Is this something we should change in the DSC?

Thanks for running these tests. The DSC behavior is correct: it should fail only when modifying the partial expression and succeed when modifying a key column. Key columns can be changed because we’ll rewrite the index if a backfill is needed, or, if no backfill is required, the on-disk encoding of the column in the index remains unchanged (e.g., BIGINT to SMALLINT).

Could you remove the key column check in the legacy code? Sorry for not mentioning this earlier!

Previously we would allow ALTER COLUMN TYPE to succeed on a column even when we have a partial index on it. This would cause issues down the line for example at insert and is inconsistent with postgres. This fix now checks for partial index and errors out like postgres when your run the ALTER COLUMN TYPE command on a column with a partial index. Fixes: cockroachdb#72456 Epic: CRDB-25314 Release note (bug fix): ALTER COLUMN TYPE now errors out when we have a partial index dependent on the column being altered.

Dedej-Bergin · 2024-10-03T14:23:08Z

pkg/sql/logictest/testdata/logic_test/alter_column_type line 1224 at r1 (raw file):

Previously, spilchen wrote…

Thanks for running these tests. The DSC behavior is correct: it should fail only when modifying the partial expression and succeed when modifying a key column. Key columns can be changed because we’ll rewrite the index if a backfill is needed, or, if no backfill is required, the on-disk encoding of the column in the index remains unchanged (e.g., BIGINT to SMALLINT).

Could you remove the key column check in the legacy code? Sorry for not mentioning this earlier!

No worries, just made the changes. The test-case results are now coming back the same for both DSC and legacy, thanks!

spilchen

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Dedej-Bergin)

Dedej-Bergin · 2024-10-03T14:57:25Z

bors r+

craig · 2024-10-03T15:27:55Z

Build succeeded:

Dedej-Bergin requested a review from spilchen September 30, 2024 13:32

Dedej-Bergin force-pushed the act-check-partial branch 3 times, most recently from 12e2476 to 1ac07e7 Compare September 30, 2024 15:44

Dedej-Bergin marked this pull request as ready for review September 30, 2024 15:44

Dedej-Bergin requested a review from a team as a code owner September 30, 2024 15:44

Dedej-Bergin force-pushed the act-check-partial branch from 1ac07e7 to 464bddd Compare September 30, 2024 17:10

spilchen requested changes Oct 1, 2024

View reviewed changes

Dedej-Bergin force-pushed the act-check-partial branch from 464bddd to 499a23e Compare October 1, 2024 12:59

Dedej-Bergin commented Oct 1, 2024

View reviewed changes

spilchen requested changes Oct 1, 2024

View reviewed changes

Dedej-Bergin force-pushed the act-check-partial branch from 499a23e to 1bc6f42 Compare October 1, 2024 21:17

Dedej-Bergin commented Oct 1, 2024

View reviewed changes

Dedej-Bergin requested a review from spilchen October 1, 2024 22:31

spilchen requested changes Oct 2, 2024

View reviewed changes

Dedej-Bergin commented Oct 2, 2024

View reviewed changes

Dedej-Bergin requested a review from spilchen October 2, 2024 20:38

spilchen requested changes Oct 2, 2024

View reviewed changes

Dedej-Bergin force-pushed the act-check-partial branch from 1bc6f42 to 5047d8b Compare October 3, 2024 14:21

Dedej-Bergin requested a review from spilchen October 3, 2024 14:23

spilchen approved these changes Oct 3, 2024

View reviewed changes

craig bot merged commit 1922797 into cockroachdb:master Oct 3, 2024

Dedej-Bergin deleted the act-check-partial branch October 3, 2024 19:01

Conversation

Dedej-Bergin commented Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blathers-crl bot commented Sep 30, 2024

Uh oh!

cockroach-teamcity commented Sep 30, 2024

Uh oh!

spilchen left a comment

Choose a reason for hiding this comment

Uh oh!

Dedej-Bergin left a comment

Choose a reason for hiding this comment

Uh oh!

spilchen left a comment

Choose a reason for hiding this comment

Uh oh!

Dedej-Bergin commented Oct 1, 2024

Uh oh!

Dedej-Bergin left a comment

Choose a reason for hiding this comment

Uh oh!

spilchen left a comment

Choose a reason for hiding this comment

Uh oh!

Dedej-Bergin left a comment

Choose a reason for hiding this comment

Uh oh!

Dedej-Bergin commented Oct 2, 2024

Uh oh!

spilchen left a comment

Choose a reason for hiding this comment

Uh oh!

Dedej-Bergin commented Oct 3, 2024

Uh oh!

spilchen left a comment

Choose a reason for hiding this comment

Uh oh!

Dedej-Bergin commented Oct 3, 2024

Uh oh!

craig bot commented Oct 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Dedej-Bergin commented Sep 30, 2024 •

edited

Loading