sql: fix unchecked constraints with cascade loops

[RaduBerinde]

sql: fix unchecked constraints with cascade loops

The cascader uses a CheckHelper when updating rows in a table. Since
we added optimizer support for mutations, we have two types of
CheckHelper (the old "eval mode" and the new "input mode").

The mutation code is passing the initial table's CheckHelper which
is in "input mode". The cascader is using it in the "eval mode" which
is a no-op. This causes CHECK constraints against the initial table to
not be verified. This can happen with self-referencing cascades, or
when there is a cascading loop.

I plan to follow up with a cleanup that removes the two modes and
separates the new "input mode" code completely. We can do this on
master where we ripped out the heuristic planner.

Fixes #42117.

Release note (bug fix): Fixed a bug in scenarios where we have UPDATE
cascades; and we are updating a table that has CHECK constraints; and
the table is self-referencing or is involved in a reference cycle. In
this case an UPDATE that cascades back in the original table was not
validated with respect to the CHECK constraints.

sql: allow check expressions with non-public columns in cascader

When adding a column with a check expression, the schema change
happens as follows:

1. The backfill starts to initialize the new column; at this time, the
   check is not "active";
2. The backfill completes; the check becomes "active" but the schema
   change is not complete and the column is still write-only;
3. The check constraint is validated in another pass;
4. The schema change is complete.

It is correct to treat the columns as readable once the check becomes
active.

This change modifies the legacy CheckHelper (used by the cascader)
to allow non-public columns in check expressions.

Release note: None

[cockroach-teamcity]

This change is 

[andy-kimball]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @RaduBerinde)

---

pkg/sql/opt_exec_factory.go, line 1407 at r1 (raw file):

	// Create a CheckHelper, used in case of cascading actions that cause changes
	// in the original table. This is only possible with UPDATE (together with

This is possible with UPSERT as well, right?

---

pkg/sql/logictest/testdata/logic_test/cascade, line 3728 at r1 (raw file):

# which violates the check.
statement error failed to satisfy CHECK constraint \(b != 1\)
UPDATE self_ab SET a = 3 WHERE a = 2

I'd add an UPSERT case as well, if it can trigger this.

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball)

---

pkg/sql/opt_exec_factory.go, line 1407 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

This is possible with UPSERT as well, right?

Added to the comment.

---

pkg/sql/logictest/testdata/logic_test/cascade, line 3728 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

I'd add an UPSERT case as well, if it can trigger this.

Done.

[RaduBerinde]

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[RaduBerinde]

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[RaduBerinde]

I thought I was hitting a flaky test (that's what teamcity showed) but this change causes TestRaceWithBackfill to fail. The test fails this upsert:

cockroach/pkg/sql/schema_changer_test.go

Line 493 in 4953f5f

t.Error(err)

with the error column "x" does not exist generated when we try to create a NewEvalCheckHelper. We are in a state where we are dropping column x but we still have a check constraint that refers to it (x >= 0). It looks like the optimizer itself is ok with this case but not the legacy code.

CC @lucy-zhang, @BramGruneir what is the expected behavior in this case? Should the check constraint be ignored? And any idea how this used to work before the optimizer? (the HP code used to create an eval check helper just like this one)

[RaduBerinde]

From what I can tell, in 19.1 the table descriptor did not have an ActiveCheck when this UPSERT was executed.. What changed?

[RaduBerinde]

Thinking more about it, I think it's a bug that the optimizer tolerates CHECK expressions that involve deletable columns. I'll try to add a check for that and maybe try to bisect to see when this started happening.

[RaduBerinde]

Used bisect to figure out when this problem started. Unfortunately it came up with 0fab1a1 which is a test improvement. I verified that the bug existed in 19.1 as well. Not quite sure how to proceed.

[RaduBerinde]

I have added a commit that modifies the CheckHelper to allow using non-public columns. The commit message contains more information.

[RaduBerinde]

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[RaduBerinde]

Failure was due to some connection problem with github.

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)