Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql/schemachanger: copy ALTER TABLE .. SET TYPE validation checks from legacy to DSC #127533

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

spilchen
Copy link
Contributor

@spilchen spilchen commented Jul 19, 2024

This duplicates various validation checks from the legacy schema changer's ALTER TABLE .. SET TYPE command for the declarative schema changer, which uses states from various schema changer elements.

Additionally, it introduces more tests for validation-only type conversions. These conversions don't require data to be rewritten; instead, they validate existing rows against the new type. For example, changing from CHAR(20) to CHAR(10) requires ensuring each row is 10 characters or fewer.

The actual implementation of validation-only type conversions in the DSC will be handled in a subsequent PR.

Epic: CRDB-25314
Informs: #127516
Release note: None

@spilchen spilchen requested a review from a team as a code owner July 19, 2024 16:55
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@spilchen spilchen force-pushed the issue-127516-add-checks branch from f4341a3 to 1e89f2b Compare July 19, 2024 18:23
@spilchen spilchen self-assigned this Jul 21, 2024
…y to DSC

This duplicates various pre-checks we have in the legacy schema
changer's ALTER TABLE .. SET TYPE command to the declarative schema
changer, which uses states from various schema changer elements.

Additionally, it introduces more tests for validation-only type
conversions. These conversions don't require data to be rewritten;
instead, they validate existing rows against the new type. For example,
changing from CHAR(20) to CHAR(10) requires ensuring each row is 10
characters or fewer.

The actual implementation of validation-only type conversions in the DSC
will be handled in a subsequent PR.

Epic: CRDB-25314
Informs: cockroachdb#126516
Release note: None
@spilchen spilchen force-pushed the issue-127516-add-checks branch from 1e89f2b to 359e6ae Compare July 26, 2024 12:51
Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

Great work, :lgtm_strong: :

Just some question, but no issues with this work.

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @spilchen)


pkg/sql/alter_column_type.go line 195 at r1 (raw file):

		}
	}
	for _, fk := range tableDesc.InboundForeignKeys() {

Did we have a bug before? Or was this enforced else where?


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

# rows will not fit in the new type. The data of existing rows gets truncated,
# which isn't correct. This will get addressed when we add the validation
# only logic in #127516.

Do we know what the behaviour of Postgres for similar scenarios? Validation will definitely make this safer and remove risk of data loss.

Copy link
Contributor Author

@spilchen spilchen left a comment

Choose a reason for hiding this comment

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

TFTR @fqazi. I responded to your comments.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @fqazi)


pkg/sql/alter_column_type.go line 195 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Did we have a bug before? Or was this enforced else where?

This was a bug before. The alter_column_type logic test tried to verify this, but it was reporting the wrong error message. It was exposed when I converted this check to the DSC. This update to the test corrects that problem:

statement ok
CREATE TABLE t19 (y INT NOT NULL REFERENCES t18 (x), INDEX(y));

- statement error pq: unimplemented: ALTER COLUMN TYPE requiring rewrite of on-disk data is currently not supported for columns that are part of an index
+ statement error pq: unimplemented: ALTER COLUMN TYPE for a column that has a constraint is currently not supported
ALTER TABLE t18 ALTER COLUMN x TYPE STRING

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

Previously, fqazi (Faizan Qazi) wrote…

Do we know what the behaviour of Postgres for similar scenarios? Validation will definitely make this safer and remove risk of data loss.

Yes, postgres will validate that the existing data fits and fails when it doesn't.

postgres=# create table k1 (c1 text);
CREATE TABLE
postgres=# insert into k1 values ('looonnnnggggg string');
INSERT 0 1
postgres=# alter table k1 alter column c1 set data type char(3);
ERROR:  value too long for type character(3)

postgres=# create table k2 (c1 decimal(20,5), c2 decimal(10,5));
CREATE TABLE
postgres=# insert into k2 values (12345.6, 1.23456),(NULL,NULL),(100012.34,4563.21);
INSERT 0 3
postgres=# alter table k2 alter column c1 set data type decimal(7,2);
ERROR:  numeric field overflow
DETAIL:  A field with precision 7, scale 2 must round to an absolute value less than 10^5.

postgres=# create table k3 (c1 bit(8), c2 varbit(8), c3 text, c4 char(20), c5 varchar(30));
CREATE TABLE
postgres=# insert into k3 values (B'10101010', B'10101010', 'hello', 'world', 'worldhello');
INSERT 0 1
postgres=# alter table k3 alter column c1 set data type bit(4);
ERROR:  bit string length 8 does not match type bit(4)
postgres=# alter table k3 alter column c1 set data type varbit(4);
ERROR:  bit string too long for type bit varying(4)

@spilchen
Copy link
Contributor Author

tftr!

bors r+

@craig craig bot merged commit b97a858 into cockroachdb:master Jul 26, 2024
22 checks passed
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.

3 participants