Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: BEFORE triggers can cause spurious constraint violation errors #133784

Closed
DrewKimball opened this issue Oct 30, 2024 · 2 comments · Fixed by #133220
Closed

sql: BEFORE triggers can cause spurious constraint violation errors #133784

DrewKimball opened this issue Oct 30, 2024 · 2 comments · Fixed by #133220
Assignees
Labels
A-sql-trigger Triggers and Trigger Functions backport-24.3.x Flags PRs that need to be backported to 24.3 branch-master Failures and bugs on the master branch. branch-release-24.3 Used to mark GA and release blockers, technical advisories, and bugs for 24.3 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-queries SQL Queries Team

Comments

@DrewKimball
Copy link
Collaborator

DrewKimball commented Oct 30, 2024

Consider a diamond cascade pattern, where an update to a parent table cascades to two child tables, which in turn cascade to the same grandchild table. In the time between when the first child cascades to the grandchild, and the second child does the same, attempting to check the FK relation from the grandchild to the second child would find a constraint violation. Here's an example logic test:

statement ok
CREATE TABLE parent (k INT PRIMARY KEY);

statement ok
CREATE TABLE child (k INT PRIMARY KEY, v INT UNIQUE NOT NULL REFERENCES parent(k) ON UPDATE CASCADE ON DELETE CASCADE);
CREATE TABLE child2 (k INT PRIMARY KEY, v INT UNIQUE NOT NULL REFERENCES parent(k) ON UPDATE CASCADE ON DELETE CASCADE);

statement ok
CREATE TABLE grandchild (
  k INT PRIMARY KEY,
  v INT REFERENCES child(v) ON UPDATE CASCADE ON DELETE CASCADE,
  v2 INT REFERENCES child2(v) ON UPDATE CASCADE ON DELETE CASCADE
);

statement ok
CREATE FUNCTION g() RETURNS TRIGGER LANGUAGE PLpgSQL AS $$
  BEGIN
    RAISE NOTICE '% % ON %: % -> %', TG_WHEN, TG_OP, TG_TABLE_NAME, OLD, NEW;
    RETURN COALESCE(NEW, OLD);
  END
$$;

statement ok
CREATE TRIGGER foo BEFORE INSERT OR UPDATE OR DELETE ON grandchild FOR EACH ROW EXECUTE FUNCTION g();

statement ok
INSERT INTO parent VALUES (1), (2), (3);
INSERT INTO child VALUES (1, 1), (2, 2), (3, 3);
INSERT INTO child2 VALUES (1, 1), (2, 2), (3, 3);
INSERT INTO grandchild VALUES (1, 1, 1), (2, 2, 2), (3, 2, 2), (4, 3, 3);

# Update the parent table, which should cascade to the children and grandchild.
# Note that both child tables cascade to the grandchild.
statement ok
UPDATE parent SET k = k + 10 WHERE k < 3;

The problem is that the trigger could in principle change how the rows are modified during the cascade to the grandchild. This results in a check being queued for both child tables after the cascade for the first one completes. This results in a FK violation error, when simply waiting for the second cascade to complete would avoid the error.

We actually disallow BEFORE triggers that modify rows in cascades by default (determined by unsafe_allow_triggers_modifying_cascades). As long as this is the case, we could avoid building the extra checks, since we verify at runtime that the cascading mutation isn't modified.

Jira issue: CRDB-43749

@DrewKimball DrewKimball added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-queries SQL Queries Team A-sql-trigger Triggers and Trigger Functions labels Oct 30, 2024
Copy link

blathers-crl bot commented Oct 30, 2024

Hi @DrewKimball, please add branch-* labels to identify which branch(es) this C-bug affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@github-project-automation github-project-automation bot moved this to Triage in SQL Queries Oct 30, 2024
@DrewKimball DrewKimball moved this from Triage to Active in SQL Queries Oct 30, 2024
@DrewKimball DrewKimball self-assigned this Oct 30, 2024
@DrewKimball DrewKimball added branch-master Failures and bugs on the master branch. backport-24.3.x Flags PRs that need to be backported to 24.3 labels Oct 30, 2024
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Oct 30, 2024
This commit fixes a bug that could cause spurious constraint violation
errors when a BEFORE trigger was planned on a cascaded mutation. Since we
don't allow triggers to modify the rows of a cascaded mutation, it is safe
to avoid the extra check entirely. This commit also adds a diamond-pattern
cascade test, with triggers on each table.

Fixes cockroachdb#133784

Release note: None
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Oct 30, 2024
This commit fixes a bug that could cause spurious constraint violation
errors when a BEFORE trigger was planned on a cascaded mutation. Since we
don't allow triggers to modify the rows of a cascaded mutation, it is safe
to avoid the extra check entirely. This commit also adds a diamond-pattern
cascade test, with triggers on each table.

Fixes cockroachdb#133784

Release note: None
craig bot pushed a commit that referenced this issue Nov 6, 2024
133220: opt: plan row-level BEFORE triggers for cascading mutations r=DrewKimball a=DrewKimball

#### opt: mechanical changes for BEFORE triggers on cascades

This commit consists of the following changes to prepare for adding
support for row-level BEFORE triggers to cascading mutations:
* Added `unsafe_allow_triggers_modifying_cascades` which will determine
  whether a trigger can modify the rows mutated by a cascade, default off.
* Added a "cascade" argument to `buildRowLevelBeforeTriggers` to provide
  the context when building a row-level BEFORE trigger.
* Refactoring the logic for invoking `crdb_internal.plpgsql_raise` to be
  re-used for triggers.

Informs #132971

Release note: None

#### opt: plan row-level BEFORE triggers for cascading mutations

This commit adds logic to invoke the functions for row-level BEFORE
triggers that are fired by a cascade's mutation. This is mostly
straightforward, except for the fact that a row-level BEFORE trigger
can actually modify or filter rows-to-be-mutated. In Postgres, it's
possible to cause constraint violations via this behavior. This commit
adds a runtime check to ensure that trigger functions do not modify
cascading updates/deletes. The check is gated behind
`unsafe_allow_triggers_modifying_cascades`, default off.

Fixes #132971

Release note (sql change): Cascades can now fire row-level BEFORE triggers.
By default, attempting to modify or eliminate the cascading update/delete
will result in a `Triggered Data Change Violation` error. Users that wish
to do this anyway can set `unsafe_allow_triggers_modifying_cascades`.
Note that doing so could result in constraint violations, similar to Postgres.

#### sql: fix queuing behavior for checks and triggers

This commit fixes a bug in `PlanAndRunPostQueries` that could cause
check queries to be run more than once. This would cause problems with
unclosed resources after query completion. This commit also fixes a
minor bug that could cause triggers queued by triggers to be run
before newly-queued checks and cascades, instead of after. The following
commit includes a regression test.

Fixes #133792

Release note: None

#### opt: don't add spurious check queries after BEFORE trigger

This commit fixes a bug that could cause spurious constraint violation
errors when a BEFORE trigger was planned on a cascaded mutation. Since we
don't allow triggers to modify the rows of a cascaded mutation, it is safe
to avoid the extra check entirely. This commit also adds a diamond-pattern
cascade test, with triggers on each table.

Fixes #133784

Release note: None

134005: ui: delete old db pages r=xinhaoz a=xinhaoz

Delete legacy db pages and related functionality.
These pages are now replaced by the v2 db pages.
Deleted
- Database overview page and related apis
- Database details page and related apis
- Database table page and related apis

Epic: none

Release note (ui change): As of 25.1 the legacy db page which was previously available via Advanced Debug is no longer available.

134313: opt: make `max-stack` opt tester option more reliable r=mgartner a=mgartner

The `max-stack` opt tester option now runs the test command in a
separate goroutine. A fresh stack makes tests using this setting more
reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see #132701) to 100KB, between 65KB in which the
test fails after the fix in #132701 and 135KB in which the test fails
before the fix.

Finally, the test has been disabled under `race` builds which increase
the size of stack frames and would cause this test to fail.

Epic: None

Release note: None


134353: mixedversion: update skip upgrades logic r=RaduBerinde a=RaduBerinde

This commit updates the logic that determines which versions support
skipping the previous major release during upgrade.

Epic: REL-1292
Release note: None

Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
Co-authored-by: Xin Hao Zhang <xzhang@cockroachlabs.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
@craig craig bot closed this as completed in ffa99f9 Nov 6, 2024
@github-project-automation github-project-automation bot moved this from Active to Done in SQL Queries Nov 6, 2024
Copy link

blathers-crl bot commented Nov 6, 2024

Based on the specified backports for linked PR #133220, I applied the following new label(s) to this issue: branch-release-24.3. Please adjust the labels as needed to match the branches actually affected by this issue, including adding any known older branches.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl blathers-crl bot added the branch-release-24.3 Used to mark GA and release blockers, technical advisories, and bugs for 24.3 label Nov 6, 2024
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Nov 6, 2024
This commit fixes a bug that could cause spurious constraint violation
errors when a BEFORE trigger was planned on a cascaded mutation. Since we
don't allow triggers to modify the rows of a cascaded mutation, it is safe
to avoid the extra check entirely. This commit also adds a diamond-pattern
cascade test, with triggers on each table.

Fixes cockroachdb#133784

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-trigger Triggers and Trigger Functions backport-24.3.x Flags PRs that need to be backported to 24.3 branch-master Failures and bugs on the master branch. branch-release-24.3 Used to mark GA and release blockers, technical advisories, and bugs for 24.3 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-queries SQL Queries Team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant