sql: check constraint and partial index corruption due to enum value renames

[ajwerner]

Describe the problem

Cockroach seems to permit the corruption of check constraints and of partial indexes when renaming enum values. Only the former problem appears on postgres. The latter problem seems to be that string to enum casts are (properly) not marked as immutable. The postgres folks seem to have documented their way out of the breaking of the check constraint with the rename; they don't say that an expression is necessarily stable. I have a feeling we may not be honoring that properly.

The CHECK clause specifies an expression producing a Boolean result which
new or updated rows must satisfy for an insert or update operation to
succeed.

I think the following steps would get us in line with postgres.

• Mark string to enum and enum to string as Stable instead of Immutable (https://github.com/cockroachdb/cockroach/issues/74470 and https://github.com/cockroachdb/cockroach/pull/74553)
• Reject partial index constraints which are not immutable
• Do not push check constraints which are not Immutable or LeakProof down into queries as predicates.

To Reproduce

CREATE TYPE typ AS ENUM ('a', 'b');
CREATE TABLE invalid_check (k VARCHAR, CHECK (k::typ = 'a'));
CREATE TABLE invalid_partial (k VARCHAR, INDEX invalid (k) WHERE (k::typ = 'a'));
CREATE INDEX invalid_idx ON invalid_partial (k) WHERE (k::typ = 'a');
INSERT INTO invalid_check VALUES ('a');
INSERT INTO invalid_partial VALUES ('a');
ALTER TYPE typ RENAME VALUE 'a' TO 'c';
SELECT * FROM invalid_partial@invalid;
INSERT INTO invalid_check VALUES ('a');
SELECT * FROM invalid_check;

This sequence of statements will corrupt both the partial index and the check constraint.

CREATE TYPE
CREATE TABLE
CREATE TABLE
INSERT 1
INSERT 1
ALTER TYPE
ERROR: index "invalid" is a partial index that does not contain all the rows needed to execute this query
ERROR: invalid input value for enum typ: "a"
SQLSTATE: 22P02
  k
-----
  a
(1 row)

Expected behavior

Postgres has the same problem with the check constraint but seems to side-step the partial index problem:

test=# CREATE INDEX invalid_idx ON invalid_partial (k) WHERE (k::typ = 'a');
ERROR:  functions in index predicate must be marked IMMUTABLE

Jira issue: CRDB-9159

[vy-ton]

Mark string to enum and enum to string as Stable instead of Immutable Reject partial index constraints which are not immutable

What about existing partial index constraints that use non-immutable expressions?

Is there a way to audit our casts for Postgres-compatibility, i.e. volatile, stable, immutable? Feels like this release we've touched this in multiple ways.

[rytaft]

Is this needed for 21.2?

[ajwerner]

Is this needed for 21.2?

I don't think it qualifies as any sort of release blocker if that's what you mean. This bug is present in all versions since 20.2.

[mgartner]

It's certainly odd that Postgres allows stable (and maybe volatile) operators in CHECK constraint expressions. This means that rows are guaranteed to conform to the constraint only at INSERT- and UPDATE-time, not always. Some relevant disccusions on the matter:

• https://www.postgresql.org/message-id/flat/0C3A1AEC-6BE4-11D8-9224-000A95C88220@myrealbox.com#0C3A1AEC-6BE4-11D8-9224-000A95C88220@myrealbox.com
• https://dba.stackexchange.com/a/75635/113903

This puts us in an awkward position. In order to be consistent with Postgres we'll need to also allow stable and volatile operators in CHECK constraints. If we wanted to ensure data in tables always conforms to CHECK constraints, we'd have to diverge from Postgres. Finally, choosing to be consistent with Postgres has some major implications that we need to be aware of. As one example, the optimizer considers CHECK constraints as truths of the table, sometimes using these truths to optimize queries. There may be cases where such optimizations cause incorrect results if the truths don't actually hold.

[rytaft]

the optimizer considers CHECK constraints as truths of the table

Could we change the logic for considering CHECK constraints as "truths" to check if the expression is immutable?

[mgartner]

Could we change the logic for considering CHECK constraints as "truths" to check if the expression is immutable?

Ahh, we already do this: https://github.com/cockroachdb/cockroach/blob/0ba84555cb36470686e9e9947608cf76a16a7fdc/pkg/sql/opt/optbuilder/select.go#L705-L710

Phew!