sql: use internal executor for FK validation checks

[RaduBerinde]

Switching to the internal executor instead of delegateQuery for FK
validation queries.

Release note: None

[cockroach-teamcity]

This change is 

[RaduBerinde]

Hm, it looks like sometimes logic tests time out because the internal query deadlocks (I see a MaybeWaitForPush). I can only assume it's waiting on the parent transaction. Any idea why that would be the case (we are providing the same txn to the internal executor)?

[jordanlewis]

I think this might be because delegate can use the executor's current table descriptor, but the internal executor looks up the real, persisted state of the table descriptor. Are there modifications happening to table scheams or constraints in transactions? I think @lucy-zhang might be familiar with this class of problem.

[RaduBerinde]

Indeed, schema_change_in_txn has this test:

statement ok
BEGIN

statement ok
ALTER TABLE orders2 ADD FOREIGN KEY (product) REFERENCES products

statement ok
ALTER TABLE orders2 VALIDATE CONSTRAINT fk_product_ref_products

statement ok
COMMIT

VALIDATE runs this internal query:

SELECT s.product FROM (SELECT * FROM test.public.orders2@orders2_product_idx WHERE product IS NOT NULL) AS s LEFT OUTER JOIN test.public.products@\"primary\" AS t ON s.product = t.sku WHERE t.sku IS NULL LIMIT 1

The lease code deadlocks inside sqlbase.GetTableDescFromID, waiting on our transaction.

[RaduBerinde]

Talked to Andrei, we probably want to pass more of the executor state (not just the txn) to the internal executor. Not quite sure where that state (mutated descriptors) is exactly, hoping someone can point me in the right direction.

[vivekmenezes]

this code is suffering from #34304

i don't think you want to take on fixing that issue. @lucy-zhang is working presently on moving validation out of the user transaction to another transaction. Perhaps the best course of action here is to not move forward with this PR or at least wait for that PR to land.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)

[thoszhang]

The PR that Vivek mentioned, to move the validation out of the user transaction (and do it as part of ADD CONSTRAINT), is #37433.

[RaduBerinde]

Great! Does that PR also move the checking to a separate transaction when we do VALIDATE, or just for ADD CONSTRAINT?

[thoszhang]

It does not change the behavior of VALIDATE CONSTRAINT, but that's planned for some point.

[RaduBerinde]

To move forward here, we'd need to move up the work to run the VALIDATE checks in a separate txns, or do the work to allow the internal executor to "see" in-progress descriptors. The latter might be useful in other cases as well, the internal executor is used by quite a few statements.

To give more context for this PR: I am trying to retire all code paths that rely on the heuristic planner and can't use the optimizer. One of these is delegateQuery; I was able to get rid of all users of delegateQuery except FK and scrub checks.

[thoszhang]

#37433 replaces delegateQuery for FK validation with the internal executor. We do currently have a means of adding table descriptors that the internal executor can see, by adding the table descriptor as an uncommitted table to the TableCollection: https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/backfill.go#L960-L993 is the original use of it, and it also gets used in #37433 for the constraint validation queries. (There's a TODO to improve the interface, which might be more important as it gets used in an increasing number of places.)

Is this what you had in mind? (I took a closer look at this issue today, and realized I also had the same VALIDATE CONSTRAINT deadlock on my branch, so I fixed it.)

[RaduBerinde]

Thank you Lucy! Yeah, it looks like your change contains what I tried to do here, and for that I salute you!