sql: enforce nullability when evaluating expressions in index backfills#81679
Conversation
|
This change is |
8d7dac8 to
4abe444
Compare
|
|
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
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan and @mgartner)
pkg/sql/logictest/testdata/logic_test/alter_table line 2521 at r1 (raw file):
Previously, chengxiong-ruan (Chengxiong Ruan) wrote…
NULL:::INTbeing a valid casting surprised me actually.... Any other use cases outside of this test case? just curious :)
But thanks for fixing this as later on...I guess UDF could returnNULLas well...
you can cast or type annotate NULL as anything. It's not needed except that without it I was tickling #81699.
chengxiong-ruan
left a comment
There was a problem hiding this comment.
Reviewable status:
fqazi
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:
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.
Prior to this change, we never enforced nullability of values when performing index backfills. Up until cockroachdb#76983, all column additions used the legacy schema changer and its column backfill protocol. The column backfiller indeed checks on nullability violations for columns [here](https://github.com/cockroachdb/cockroach/blob/d5313438aaa61a71a5a66c5718963bbb0fd7268b/pkg/sql/backfill/backfill.go#L355-L357). Release note (bug fix): Prior to this change, virtual computed columns which were marked as NOT NULL could be added to new secondary index. After this change, attempts to add such columns to a secondary index will result in an error. Note that such invalid columns can still be added to tables. Work to resolve that bug is tracked in cockroachdb#81675.
4abe444 to
30fa449
Compare
|
TFTR! bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 30fa449 to blathers/backport-release-21.2-81679: 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 30fa449 to blathers/backport-release-22.1-81679: 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. |
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 <awerner32@gmail.com>
|
Does this need a backport still? |
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.
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.
|
Thanks for doing that. Just to confirm, it looks like #81693 was backported, but this PR was not. (I only did a cursory check, but maybe the manual backport you did actually combined both.) |
|
Darn, wrong backport :( |
Prior to this change, we never enforced nullability of values when performing
index backfills. Up until #76983, all column additions used the legacy schema
changer and its column backfill protocol. The column backfiller indeed checks
on nullability violations for columns here.
Release note (bug fix): Prior to this change, virtual computed columns which
were marked as NOT NULL could be added to new secondary index. After this
change, attempts to add such columns to a secondary index will result in an
error. Note that such invalid columns can still be added to tables. Work to
resolve that bug is tracked in #81675.