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.

Sign up for GitHub

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

Jump to bottom

sql/schemachanger: properly validate NOT NULL columns #81690

Closed
ajwerner opened this issue May 23, 2022 · 2 comments
Closed

sql/schemachanger: properly validate NOT NULL columns #81690

ajwerner opened this issue May 23, 2022 · 2 comments
Labels
A-schema-changer-impl Related to the implementation of the new schema changer C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@ajwerner
Copy link
Contributor

ajwerner commented May 23, 2022 •
edited by exalate-issue-sync bot
Loading

Describe the problem

At time of writing, the declarative schema changer does not validate the NOT NULL nature of newly added columns. The change in #81679 captures most of what is necessary: push the logic into the index backfiller. The remaining case that needs to be covered is validating the NOT NULL nature of virtual computed columns, which is being handled separately for the legacy schema changer.

Jira issue: CRDB-16031

Epic CRDB-43310
@ajwerner ajwerner added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label May 23, 2022
@blathers-crl blathers-crl bot added the T-sql-schema-deprecated Use T-sql-foundations instead label May 23, 2022
ajwerner added a commit to ajwerner/cockroach that referenced this issue May 23, 2022
@ajwerner
sql: enforce NOT NULL when adding a virtual computed column
ae220c1 
Before this change, we did not ever validate that newly added
virtual computed columns which were marked NOT NULL actually were
not NULL. This is because there historically, in the legacy schema changer,
we'd validate the `NULL` nature of the column when performing a column
backfill. In the declarative schema changer, we don't do a column backfill,
so we have bugs aplenty related to the general failure to check the `NULL`
disposition of the data (see cockroachdb#81679).

This change only affects the legacy schema changer so that it can be
backported. A separate issue (cockroachdb#81690) has been filed for parity in the
declarative schema changer.

Release note (bug fix): Before this change, not validation was performed
when adding a virtual computed column which was marked `NOT NULL`. This meant
that it was possible to have a virtual computed column with an active `NOT
NULL`
constraint despite having rows in the table for which the column was `NULL`.
This has now been fixed.
@ajwerner ajwerner self-assigned this May 23, 2022
@ajwerner ajwerner mentioned this issue May 23, 2022
Merged
ajwerner added a commit to ajwerner/cockroach that referenced this issue May 25, 2022
@ajwerner
sql: enforce NOT NULL when adding a virtual computed column
84bb802 
Before this change, we did not ever validate that newly added
virtual computed columns which were marked NOT NULL actually were
not NULL. This is because there historically, in the legacy schema changer,
we'd validate the `NULL` nature of the column when performing a column
backfill. In the declarative schema changer, we don't do a column backfill,
so we have bugs aplenty related to the general failure to check the `NULL`
disposition of the data (see cockroachdb#81679).

This change only affects the legacy schema changer so that it can be
backported. A separate issue (cockroachdb#81690) has been filed for parity in the
declarative schema changer.

Release note (bug fix): Before this change, not validation was performed
when adding a virtual computed column which was marked `NOT NULL`. This meant
that it was possible to have a virtual computed column with an active `NOT
NULL`
constraint despite having rows in the table for which the column was `NULL`.
This has now been fixed.
craig bot pushed a commit that referenced this issue May 31, 2022
@ajwerner
Merge #81693
2585513 
81693: sql: enforce NOT NULL when adding a virtual computed column r=ajwerner a=ajwerner

Before this change, we did not ever validate that newly added
virtual computed columns which were marked NOT NULL actually were
not NULL. This is because there historically, in the legacy schema changer,
we'd validate the `NULL` nature of the column when performing a column
backfill. In the declarative schema changer, we don't do a column backfill,
so we have bugs aplenty related to the general failure to check the `NULL`
disposition of the data (see #81679).

This change only affects the legacy schema changer so that it can be
backported. A separate issue (#81690) has been filed for parity in the
declarative schema changer.

Release note (bug fix): Before this change, not validation was performed
when adding a virtual computed column which was marked `NOT NULL`. This meant
that it was possible to have a virtual computed column with an active `NOT
NULL`
constraint despite having rows in the table for which the column was `NULL`.
This has now been fixed.

Co-authored-by: Andrew Werner <[email protected]>
@ajwerner ajwerner mentioned this issue Jun 24, 2022
Merged
@ajwerner ajwerner mentioned this issue Jun 24, 2022
Merged
ajwerner added a commit to ajwerner/cockroach that referenced this issue Jun 27, 2022
@ajwerner
sql: enforce NOT NULL when adding a virtual computed column
98020b6 
Before this change, we did not ever validate that newly added
virtual computed columns which were marked NOT NULL actually were
not NULL. This is because there historically, in the legacy schema changer,
we'd validate the `NULL` nature of the column when performing a column
backfill. In the declarative schema changer, we don't do a column backfill,
so we have bugs aplenty related to the general failure to check the `NULL`
disposition of the data (see cockroachdb#81679).

This change only affects the legacy schema changer so that it can be
backported. A separate issue (cockroachdb#81690) has been filed for parity in the
declarative schema changer.

Release note (bug fix): Before this change, not validation was performed
when adding a virtual computed column which was marked `NOT NULL`. This meant
that it was possible to have a virtual computed column with an active `NOT
NULL`
constraint despite having rows in the table for which the column was `NULL`.
This has now been fixed.
ajwerner added a commit to ajwerner/cockroach that referenced this issue Jun 27, 2022
@ajwerner
sql: enforce NOT NULL when adding a virtual computed column
2f3c84c 
Before this change, we did not ever validate that newly added
virtual computed columns which were marked NOT NULL actually were
not NULL. This is because there historically, in the legacy schema changer,
we'd validate the `NULL` nature of the column when performing a column
backfill. In the declarative schema changer, we don't do a column backfill,
so we have bugs aplenty related to the general failure to check the `NULL`
disposition of the data (see cockroachdb#81679).

This change only affects the legacy schema changer so that it can be
backported. A separate issue (cockroachdb#81690) has been filed for parity in the
declarative schema changer.

Release note (bug fix): Before this change, not validation was performed
when adding a virtual computed column which was marked `NOT NULL`. This meant
that it was possible to have a virtual computed column with an active `NOT
NULL`
constraint despite having rows in the table for which the column was `NULL`.
This has now been fixed.
@postamar
Copy link
Contributor

postamar commented Jul 5, 2022

Presently the nullness of the column is in scpb.ColumnType, which is wrong, instead it should be in some constraint element. This constraint should then be validated.

@postamar postamar assigned postamar and unassigned ajwerner Jul 5, 2022
@postamar postamar removed their assignment Aug 23, 2022
@postamar postamar added the A-schema-changer-impl Related to the implementation of the new schema changer label Nov 10, 2022
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
@rafiss
Copy link
Collaborator

rafiss commented Dec 5, 2024 •
edited
Loading

This was done as part of #117647, and it was implemented in a17ac10, first released in v24.1.0.

@rafiss rafiss closed this as completed Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-schema-changer-impl Related to the implementation of the new schema changer C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rafiss @ajwerner @postamar