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: computed columns are not validated on existing data #81698

Open
ajwerner opened this issue May 23, 2022 · 2 comments · May be fixed by #94255
Open

sql: computed columns are not validated on existing data #81698

ajwerner opened this issue May 23, 2022 · 2 comments · May be fixed by #94255
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. E-easy Easy issue to tackle, requires little or no CockroachDB experience T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@ajwerner
Copy link
Contributor

ajwerner commented May 23, 2022

Describe the problem

We don't currently evaluate computed expressions for new virtual computed columns to ensure that they
are indeed valid for all of the data in the table. This is a major oversight.

To Reproduce

CREATE TABLE t (i INT PRIMARY KEY);
INSERT INTO t VALUES (1);
ALTER TABLE t ADD COLUMN s VARCHAR(2) AS ('stored') STORED;
ALTER TABLE t ADD COLUMN v VARCHAR(2) AS ('virtual') STORED;
SELECT * FROM t;
  i |   s    |    v
----+--------+----------
  1 | stored | virtual
INSERT INTO t VALUES (2);
ERROR: value too long for type VARCHAR(2)
SQLSTATE: 22001

Expected behavior

We should fail to add these columns.

Jira issue: CRDB-16484

Epic CRDB-31282

@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
@ajwerner ajwerner added this to Triage in SQL Foundations via automation May 23, 2022
@blathers-crl blathers-crl bot added the T-sql-schema-deprecated Use T-sql-foundations instead label May 23, 2022
@ajwerner
Copy link
Contributor Author

Same problem for decimals and other things of that ilk. This goes way way back. I think the same problem applies for default values.

@ajwerner ajwerner moved this from Triage to 22.2 General in SQL Foundations May 24, 2022
@ajwerner ajwerner self-assigned this May 24, 2022
@mari-crl mari-crl added sync-me and removed sync-me labels Jun 2, 2022
@postamar postamar moved this from 22.2 General to Triage in SQL Foundations Jun 29, 2022
@ajwerner ajwerner removed their assignment Jul 5, 2022
@postamar postamar added the E-easy Easy issue to tackle, requires little or no CockroachDB experience label Jul 5, 2022
@postamar postamar moved this from Triage to Backlog in SQL Foundations Jul 5, 2022
@fqazi fqazi self-assigned this Oct 6, 2022
kivancbilen added a commit to kivancbilen/cockroach that referenced this issue Dec 23, 2022
default values while adding

Computed columns with default
values are not validated while adding them, this was causing
issues when data is inserted. Same validation mechanism as insert line
is implemented.

Resolves: cockroachdb#81698
Signed-off-by: Kivanc Bilen <kivanc_bilen@hotmail.com>
kivancbilen added a commit to kivancbilen/cockroach that referenced this issue Jan 3, 2023
default values while adding

Computed columns with default
values are not validated while adding them, this was causing
issues when data is inserted. Same validation mechanism as insert line
is implemented.

Release Note: Previously while adding computed columns with constants
they were not validated, validation is implemented.

Resolves: cockroachdb#81698
Signed-off-by: Kivanc Bilen <kivanc_bilen@hotmail.com>

enum tests are added, fixed the code accordingly

Signed-off-by: Kivanc Bilen <kivanc_bilen@hotmail.com>

error message is changed in test

Signed-off-by: Kivanc Bilen <kivanc_bilen@hotmail.com>

validation is moved from alter_table_add_column ->
computed_column,
tests are added to computed,
create table tests are added

Signed-off-by: Kivanc Bilen <kivanc_bilen@hotmail.com>

failing virtual columns are fixed

Signed-off-by: Kivanc Bilen <kivanc_bilen@hotmail.com>
kivancbilen added a commit to kivancbilen/cockroach that referenced this issue Jan 10, 2023
default values while adding

Computed columns with default
values are not validated while adding them, this was causing
issues when data is inserted. Same validation mechanism as insert line
is implemented.

Resolves: cockroachdb#81698

Release Note: Previously while adding computed columns with constants
they were not validated, validation is implemented.
Signed-off-by: Kivanc Bilen <kivanc_bilen@hotmail.com>
@mgartner
Copy link
Collaborator

Postgres (on 14.4, at least) has a weird inconsistency: a table can be created with a column like v VARCHAR(2) GENERATED ALWAYS AS ('foo') STORED but will error when adding the same column to an existing table:

marcus=# SELECT version();
                                                      version
--------------------------------------------------------------------------------------------------------------------
 PostgreSQL 14.4 on x86_64-apple-darwin21.5.0, compiled by Apple clang version 13.1.6 (clang-1316.0.21.2.5), 64-bit
(1 row)

marcus=# CREATE TABLE t1 (i INT, v VARCHAR(2) GENERATED ALWAYS AS ('foo') STORED);
CREATE TABLE

marcus=# INSERT INTO t1(i) VALUES (1);
ERROR:  22001: value too long for type character varying(2)


marcus=# CREATE TABLE t2 (i INT);
CREATE TABLE

marcus=# ALTER TABLE t2 ADD COLUMN v VARCHAR(2) GENERATED ALWAYS AS ('foo') STORED;
ERROR:  22001: value too long for type character varying(2)

So it looks like we handle the CREATE TABLE case correctly, but need special logic for the ALTER TABLE case. I originally thought the error was the result of an assignment cast performed while backfilling the column, but the error occurs for an empty table when nothing is backfilled... unless they always evaluate the expression even if the table has no rows. Maybe we should solve this similarly?

kivancbilen added a commit to kivancbilen/cockroach that referenced this issue Jan 12, 2023
Computed columns with constant value are not validated
while adding them, this was causing issues when
data is inserted. Same validation mechanism as insert
line is implemented.

Resolves: cockroachdb#81698

Release note (bug fix): Computed columns with constant value expressions are
now validated when they are added to the table. If the constant value does not
conform to the column type, an error is returned. Previously, the column would
be successfully added and any subsequent INSERTs would fail.
Signed-off-by: Kivanc Bilen <kivanc_bilen@hotmail.com>
@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
mgartner added a commit to mgartner/cockroach that referenced this issue Jun 28, 2023
optbuilder wraps an assignment cast around a virtual computed column
expression when writing to the table if the expression does not have the
same type as the virtual column. This matches the behavior of all writes
to columns of expressions that don't match the target column type.

However, the same cast was not applied to the projection of the virtual
column expression when reading from the table. This cast is necessary in
order to produce the correct the value of the column - the projection
must produce the same value that would have been written to the table if
the column was a stored computed column. This commit corrects optbuilder
so that the cast is correctly added to the projection.

Note that this commit adds a standard cast, not an assignment cast, as a
temporary workaround until cockroachdb#81698 is addressed. This is because an
assignment cast can fail in cases when a regular cast will not. Because
we don't validate that all existing rows can successfully produce a new
virtual column expression during a backfill, adding an assignment cast
to the projection of the virtual column could cause future reads to
error. Once cockroachdb#81698 is addressed, we can change these casts to assignment
casts so that they directly match the values that would be written to
the same column if it were stored.

Fixes cockroachdb#91817
Informs cockroachdb#81698

Release note (bug fix): A bug has been fixed that could produce incorrect
values for virtual computed columns in rare cases. The bug only
presented when the virtual column expression's type did not match the
type of the virtual column.
mgartner added a commit to mgartner/cockroach that referenced this issue Jun 29, 2023
optbuilder wraps an assignment cast around a virtual computed column
expression when writing to the table if the expression does not have the
same type as the virtual column. This matches the behavior of all writes
to columns of expressions that don't match the target column type.

However, the same cast was not applied to the projection of the virtual
column expression when reading from the table. This cast is necessary in
order to produce the correct the value of the column - the projection
must produce the same value that would have been written to the table if
the column was a stored computed column. This commit corrects optbuilder
so that the cast is correctly added to the projection.

Note that this commit adds a standard cast, not an assignment cast, as a
temporary workaround until cockroachdb#81698 is addressed. This is because an
assignment cast can fail in cases when a regular cast will not. Because
we don't validate that all existing rows can successfully produce a new
virtual column expression during a backfill, adding an assignment cast
to the projection of the virtual column could cause future reads to
error. Once cockroachdb#81698 is addressed, we can change these casts to assignment
casts so that they directly match the values that would be written to
the same column if it were stored.

Fixes cockroachdb#91817
Informs cockroachdb#81698

Release note (bug fix): A bug has been fixed that could produce incorrect
values for virtual computed columns in rare cases. The bug only
presented when the virtual column expression's type did not match the
type of the virtual column.
mgartner added a commit to mgartner/cockroach that referenced this issue Jul 5, 2023
optbuilder wraps an assignment cast around a virtual computed column
expression when writing to the table if the expression does not have the
same type as the virtual column. This matches the behavior of all writes
to columns of expressions that don't match the target column type.

However, the same cast was not applied to the projection of the virtual
column expression when reading from the table. This cast is necessary in
order to produce the correct value of the column - the projection must
produce the same value that would have been written to the table if the
column was a stored computed column. This commit corrects optbuilder so
that the cast is correctly added to the projection.

Note that this commit adds a standard cast, not an assignment cast, as a
temporary workaround until cockroachdb#81698 is addressed. This is because an
assignment cast can fail in cases when a regular cast will not. Because
we don't validate that all existing rows can successfully produce a new
virtual column expression during a backfill, adding an assignment cast
to the projection of the virtual column could cause future reads to
error. Once cockroachdb#81698 is addressed, we can change these casts to assignment
casts so that they directly match the values that would be written to
the same column if it were stored.

Fixes cockroachdb#91817
Informs cockroachdb#81698

Release note (bug fix): A bug has been fixed that could produce incorrect
values for virtual computed columns in rare cases. The bug only
presented when the virtual column expression's type did not match the
type of the virtual column.
craig bot pushed a commit that referenced this issue Jul 5, 2023
105736: opt: wrap virtual computed column projections in a cast r=mgartner a=mgartner

#### sql/logictest: remove assignment cast TODO

This commit removes a TODO that was partially addressed by #82022.

Informs #73743

Release note: None

#### sql/types: fix T.Identical

This commit fixes a bug in `types.T.Identical` which caused it to return
false for collated string types with a different string-representation
of locales that represents the same locale logically. For example,
collated string types with locales `en_US` and `en_us` would not be
identical, even though these are both valid representations of the same
locale.

There is no release note because this has not caused any known bugs.

Release note: None

#### opt: wrap virtual computed column projections in a cast

optbuilder wraps an assignment cast around a virtual computed column
expression when writing to the table if the expression does not have the
same type as the virtual column. This matches the behavior of all writes
to columns of expressions that don't match the target column type.

However, the same cast was not applied to the projection of the virtual
column expression when reading from the table. This cast is necessary in
order to produce the correct value of the column - the projection must
produce the same value that would have been written to the table if the
column was a stored computed column. This commit corrects optbuilder so
that the cast is correctly added to the projection.

Note that this commit adds a standard cast, not an assignment cast, as a
temporary workaround until #81698 is addressed. This is because an
assignment cast can fail in cases when a regular cast will not. Because
we don't validate that all existing rows can successfully produce a new
virtual column expression during a backfill, adding an assignment cast
to the projection of the virtual column could cause future reads to
error. Once #81698 is addressed, we can change these casts to assignment
casts so that they directly match the values that would be written to
the same column if it were stored.

Fixes #91817
Informs #81698

Release note (bug fix): A bug has been fixed that could produce incorrect
values for virtual computed columns in rare cases. The bug only
presented when the virtual column expression's type did not match the
type of the virtual column.


105932: opt/exec: add passthrough cols to DELETE USING result cols in explain r=yuzefovich,DrewKimball a=michae2

Now that we support `DELETE USING`, delete nodes can have passthrough columns. Add these to the result columns used by `EXPLAIN (TYPES)`.

Fixes: #105803

Release note (sql change): Fix an internal error when using `EXPLAIN (TYPES)` on a `DELETE FROM ... USING ... RETURNING` statement.

Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
Co-authored-by: Michael Erickson <michae2@cockroachlabs.com>
@rafiss rafiss assigned Xiang-Gu and unassigned Xiang-Gu Jul 18, 2023
@rafiss rafiss removed their assignment Aug 1, 2023
@rafiss rafiss removed the db-cy-23 label Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. E-easy Easy issue to tackle, requires little or no CockroachDB experience T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
SQL Foundations
  
Backlog
Development

Successfully merging a pull request may close this issue.

8 participants