sql: still more routine dependency improvements

[DrewKimball]

sql: still more routine dependency improvements

I missed in #158935 that check constraints and partial index predicates
are additional places where we unnecessarily track transitive column
dependencies when creating a routine. The fix is gated behind the same
session setting as #158935.

Fixes #158154

Release note (sql change): Fixed two cases where creation of a routine
resolves unnecessary column dependencies, which can prevent drop of the
column without first dropping the routine. Here, the unnecessary
dependencies are due to references within CHECK constraints, including
those for RLS policies and hash-sharded indexes, as well as those in
partial index predicates. The fix is gated behind the session setting
use_improved_routine_deps_triggers_and_computed_cols, which is off by
default before 26.1.

optbuilder: don't track deps for virtual tables

A previous fix in #146250 added dependency tracking for tables in
appendOrdinaryColumnsFromTable. This fix missed that the table could
be a virtual table, in which case we don't need (and in some places
can't handle) tracking dependencies. This commit adds a filter to
avoid tracking virtual tables.

The only place I've found where this mattered was in the cross-DB
check for views, which led to a test flake after some changes in
the previous commit.

Informs #158154

Release note: None

[cockroach-teamcity]

This change is 

[mgartner]

Nice catch!

Does this fix the case for enums, which also synthesize check constraints? Is it worth adding this test:

CREATE TYPE typ AS ENUM ('foo', 'bar');

CREATE TABLE t (a INT, b typ);

CREATE PROCEDURE p() LANGUAGE SQL AS $$
  INSERT INTO t (a) VALUES (1);
  UPDATE t SET a = a + 1;
$$;

CALL p();

ALTER TABLE t DROP COLUMN b;

CALL p();

We don't have to worry about partial indexes for now, because of #97813. I'll add a note to that issue pointing this out. If you think it's worth a TODO in the code where we build partial index predicates, then you can add one.

@mgartner reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ZhouXing19)

[ZhouXing19]

Nice find!!

@ZhouXing19 reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @DrewKimball)

[DrewKimball]

@mgartner good point about enums and partial indexes. I added an enum test (already fixed), but noticed that there were several more cases we didn't handle for partial indexes. Those are now fixed, with test cases added.

[mgartner]

@mgartner reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball and @ZhouXing19)

---

pkg/sql/opt/optbuilder/partial_index.go line 128 at r2 (raw file):

		// dependencies on any UDTs or columns referenced by the predicate. The
		// partial index will already prevent dropping such UDTs or columns.
		defer b.DisableSchemaDepTracking()()

I'm wondering if this is the only addition you need for partial indexes. It looks like the three others you added all lead down to this method - the metadata and two arbiter cases. Did you try making just this one addition?

[DrewKimball]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner and @ZhouXing19)

---

pkg/sql/opt/optbuilder/partial_index.go line 128 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I'm wondering if this is the only addition you need for partial indexes. It looks like the three others you added all lead down to this method - the metadata and two arbiter cases. Did you try making just this one addition?

I believe each of those cases ends up building the predicate directly with buildScalar, so it's necessary. I did try removing some of them individually to make sure the tests fail as expected, though it wasn't exhaustive.

[DrewKimball]

Unit tests flaked, just kicked off a retry.

[DrewKimball]

Hm, the flake in TestCreateAsVTable is because I made the addPartialIndexPredicatesForTable conditional as a "legacy" code path. For some reason, the schema changer is unable to tell that the dep added by appendOrdinaryColumnsFromTable is from the system database. Rather than fix that here, I've just made the disabled dependency checking unconditional for that function again.

[DrewKimball]

I looked further into that test flake, and decided it was worth fixing in the backport. The second commit handles that by adding a check for virtual tables in appendOrdinaryColumnsFromTable before deciding to track column dependencies.

[DrewKimball]

TFTRs!

bors r+

[craig]

Build succeeded:

• linux_amd64_fips_build
• local_roachtest_fips
• check_generated_code
• local_roachtest
• linux_amd64_build
• acceptance
• lint
• docker_image_amd64
• unit_tests
• examples_orms

[DrewKimball]

blathers backport 26.1