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: enforce NOT NULL when adding a virtual computed column #81693

Conversation

ajwerner
Copy link
Contributor

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.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner marked this pull request as ready for review May 24, 2022 01:17
@ajwerner ajwerner requested a review from a team May 24, 2022 01:17
Copy link
Contributor

@chengxiong-ruan chengxiong-ruan 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! 1 of 0 LGTMs obtained

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 force-pushed the ajwerner/enforce-nullability-for-newly-added-virtual-columns-legacy branch from ae220c1 to 84bb802 Compare May 25, 2022 16:47
@ajwerner
Copy link
Contributor Author

TFTR!

bors r=chengxiong-ruan

@craig
Copy link
Contributor

craig bot commented May 31, 2022

Build failed:

@ajwerner
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented May 31, 2022

Build succeeded:

@craig craig bot merged commit 2585513 into cockroachdb:master May 31, 2022
@blathers-crl
Copy link

blathers-crl bot commented May 31, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 84bb802 to blathers/backport-release-21.2-81693: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2.x failed. See errors above.


error creating merge commit from 84bb802 to blathers/backport-release-22.1-81693: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@rafiss
Copy link
Collaborator

rafiss commented Jun 24, 2022

Does this need a backport still?

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.

4 participants