Fix cycle detection for foreign keys

[GuptaManan100]

Description

The Problem

The problem was found in a foreign key constraint referencing a column in the same table and having a delete cascade constraint. The reason we didn't catch this in our test suite, is that we have little testing for self-referencing foreign key constraints. We only have tests with on delete restrict and on update restrict constraints. We reject cyclic foreign keys anyway, but this specific case was missed.

Let's look at the schema of the table -

CREATE TABLE `employee` (
	`id` bigint unsigned NOT NULL AUTO_INCREMENT,
	`manager_id` bigint unsigned NOT NULL,
	PRIMARY KEY (`id`),
	UNIQUE KEY `idx_manager_id` (`manager_id`),
        -- The following constraint is the problematic one:
	CONSTRAINT `self_referencing_key_with_cascade` FOREIGN KEY (`manager_id`) REFERENCES `employee` (`id`) ON DELETE CASCADE
) ENGINE InnoDB,
  CHARSET utf8mb4,
  COLLATE utf8mb4_0900_ai_ci;

Now let's consider how a vtgate would plan a query like delete from employee where id = 3.

The first thing that vtgate notices is that the table has a foreign key constraint so it needs to do some cascades. So, we first create a select query that would look something like select id from employee where id = 3. This selection would give us a list of id values that we will use to run a cascaded deleted like delete from employee where manager_id in :dmvals.
But wait a minute, that delete query will also be planned and vtgate would again infer that it can lead to cascades, so we would create a selection query that would look something like select id from employee where manager_id in :dmvals. Using this selection we would plan a delete query like delete from employee where manager_id in :dmvals2
Lo and behold, we are in an infinite loop!!! This process will keep on going and this causes a stack overflow eventually!

This is very interesting because our logic for rejecting self-referenced foreign keys was to exactly prevent this kind of infinite loop in planning, because the only correct way to plan these queries is to change the plan from a tree to a cyclic directed graph. That will be an involved change because the rewriter and various other components of the planner will need to account for this paradigm change.

The next question to answer is that why didn't we fail this query. The answer to that lies in how we detect cycles in foreign keys. The way we do that is by creating a graph where a vertex is a combination of table name and a list of column names and an edge represents a foreign key contraint between them. Then we just detect cycles on the created graph.

The special thing here is that even though the foreign key is from employee.id to employee.manager_id because of the delete cascade we still end up in a planning loop even though there is no direct cycle in the graph.

The Fix to FK Cycle Detection

I have been thinking about this since yesterday, and I think I've got what changes we need to make. As I stated ☝️ the graph we create now has vertices that are a combination of a table name and a list of column names and an edge represents a foreign key constraint between them. This I think needs to change.

Instead, we should just have single table-name qualified columns as vertices. Then we can add edges between them, that represent that an update/delete operation to a certain column can potentially lead to an update/delete to the other column.

Now while adding edges, we need to be careful about the type of foreign key as well -

1. ON DELETE RESTRICT ON UPDATE RESTRICT - This is the simplest case where no update/delete is required on the child table, we only need to verify whether a value exists or not. So we don't need to add any edge for this case.
2. ON DELETE SET NULL, ON UPDATE SET NULL, ON UPDATE CASCADE - In this case having any update/delete on any of the columns in the parent side of the foreign key will make a corresponding delete/update on all the column in the child side of the foreign key. So we will add an edge from all the columns in the parent side to all the columns in the child side.
3. ON DELETE CASCADE - This is a special case wherein a deletion on the parent table will affect all the columns in the child table irrespective of the columns involved in the foreign key! This is vastly important because even if the foreign keys aren't in a cycle, this is the reason why we are seeing this bug. So, we'll add an edge from all the columns in the parent side of the foreign key to all the columns of the child table.

Once we have this graph generated, we just need to detect cycles on it. Any cycle in this graph will lead to the planning going into an infinite loop since a certain update/delete on a column (that is part of the cycle), will end up causing an update/delete cascade to itself!

Let's look at a few examples of how this works in practice.

Example 1

Let's start with the case that is reported in this issue itself -

CREATE TABLE `employee` (
	`id` bigint unsigned NOT NULL AUTO_INCREMENT,
	`manager_id` bigint unsigned NOT NULL,
	PRIMARY KEY (`id`),
	UNIQUE KEY `idx_manager_id` (`manager_id`),
        -- The following constraint is the problematic one:
	CONSTRAINT `self_referencing_key_with_cascade` FOREIGN KEY (`manager_id`) REFERENCES `employee` (`id`) ON DELETE CASCADE
) ENGINE InnoDB,
  CHARSET utf8mb4,
  COLLATE utf8mb4_0900_ai_ci;

Because of the on delete cascade in the foreign key, we'll end up with a graph like so -

flowchart TD
    A[employee.id]--> B[empoyee.manager_id]
    A --> A

Loading

Example 2

This same situation can arise in a multi table schema as well.

CREATE TABLE `One` (
	`a` bigint unsigned NOT NULL AUTO_INCREMENT,
	`b` bigint unsigned NOT NULL,
	`c` bigint unsigned NOT NULL,
	PRIMARY KEY (`a`),
	CONSTRAINT `fk_1` FOREIGN KEY (`b`) REFERENCES `Two` (`f`) ON DELETE CASCADE
) ENGINE InnoDB,
  CHARSET utf8mb4,
  COLLATE utf8mb4_0900_ai_ci;

CREATE TABLE `Two` (
	`d` bigint unsigned NOT NULL AUTO_INCREMENT,
	`e` bigint unsigned NOT NULL,
	`f` bigint unsigned NOT NULL,
	PRIMARY KEY (`d`),
	CONSTRAINT `fk_2` FOREIGN KEY (`e`) REFERENCES `One` (`a`) ON DELETE CASCADE
) ENGINE InnoDB,
  CHARSET utf8mb4,
  COLLATE utf8mb4_0900_ai_ci;

Because of the on delete cascade in the foreign key, we'll end up with a graph like so -

flowchart TD
    A[One.a]
    B[One.b]
    C[One.c]
    D[Two.d]
    E[Two.e]
    F[Two.f]
    B --> D
    B --> E
    B --> F
    E --> A
    E --> B
    E --> C

Loading

Related Issue(s)

• Fixes Bug Report: Stack Overflow in a foreign key managed query #15457

Checklist

• "Backport to:" labels have been added if this change should be back-ported to release branches
• If this change is to be back-ported to previous releases, a justification is included in the PR description
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on CI?
• Documentation was added or is not required

Deployment Notes

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[shlomi-noach]

Confirm that backport is desired as this fixes a buffer overflow issue.

[deepthi]

Confirm that backport is desired as this fixes a buffer overflow issue.

FKs are still experimental. We should only backport if someone is running in production and requests a backport for this specific bug.

[GuptaManan100]

The PR is being backported because it causes the vtgate to crash incase the user is using a schema similar to one described in the issue.

[GuptaManan100]

Okay, I'll remove the backport label

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.65%. Comparing base (85aeb34) to head (979de74).
Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #15458   +/-   ##
=======================================
  Coverage   65.64%   65.65%           
=======================================
  Files        1563     1563           
  Lines      194388   194407   +19     
=======================================
+ Hits       127610   127634   +24     
+ Misses      66778    66773    -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[GuptaManan100]

Docs PR - vitessio/website#1703