opt: preserve mutation rows in RETURNING subquery joins - #174790
Open
Alignyx wants to merge 2 commits into
Open
Conversation
A correlated subquery in INSERT ... ON CONFLICT ... RETURNING could lose all result rows. The self-join matching shortcut treated mutation output columns as proof that matching rows already existed in a scan of the same table. That proof is invalid for newly inserted values visible through RETURNING, and could turn the required outer join into an inner join. Apply the self-join shortcut only when neither input can mutate. Add SQL logic coverage for the original RETURNING behavior and stored table data. The added regression fails on the unpatched source and passes with this commit. An independent post-hoc review identified a related foreign-key proof that needs an additional correction, provided in the next commit. See also: cockroachdb#105640 Epic: none Release note (bug fix): Fixed a bug where INSERT ... ON CONFLICT ... RETURNING with a correlated subquery reading the target table could omit returned rows even though the write succeeded.
Guarding the self-join shortcut alone leaves another invalid proof in filtersMatchAllLeftRows. With a self-referencing primary-key foreign key, the same INSERT ... ON CONFLICT ... RETURNING query falls through to the foreign-key shortcut and still loses its result row. A constraint on stored rows does not prove that mutation output has a matching row in the snapshot read by the RETURNING subquery. Reject foreign-key preservation proofs when either input can mutate, for both equality joins and cross joins. Retain the independent cardinality proof for cross joins whose right input is guaranteed to be nonempty, and leave the existing proofs available for read-only inputs. Add a regression for the self-referencing foreign-key reproduction, with an assertion on the stored row. Also cover an uncorrelated RETURNING subquery on a self-referencing table and a read-only self-join with NULL values. The foreign-key reproduction fails with the preceding commit and passes with this correction. Fixes cockroachdb#105640 Epic: none Release note (bug fix): Fixed missing rows in INSERT ... ON CONFLICT ... RETURNING results when a subquery reads a self-referencing foreign-key table. The write could succeed while an incorrect join optimization removed the returned row.
|
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. Before a member of our team reviews your PR, I have some potential action items for you:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #105640.
INSERT ... ON CONFLICT ... RETURNINGcan successfully write a row while returning no rows when a correlated subquery reads the target table. For example, on an empty table:The expected result is one row,
(1, 10, NULL): the inserted values are available throughRETURNING, while the subquery's table scan does not see the newly inserted row. Before this change, the statement returns zero rows even though a subsequentSELECTfinds(1, 10)in the table. The issue reports this behavior in v21.1 and later.The incorrect inference is in
filtersMatchAllLeftRows. Column metadata on a mutation's output can identify the same base-table columns as a scan, but that does not establish that the scan contains a matching row. Treating that metadata as a row-preservation guarantee allows an outer join needed for the subquery to become an inner join, dropping the mutation's result when the scan is empty.The change is split into two commits:
CanMutate == false. Add regressions for insertion into an empty table and the conflict-update path, checking both returned values and stored data. The conflict case also checks that the subquery observes the previous value(1,77)whileRETURNINGexposes the updated value10.k INT PRIMARY KEY REFERENCES kv(k)and run the same INSERT. The self-join restriction alone still returns zero rows because the foreign-key shortcut reaches the same invalid conclusion. A foreign-key constraint on stored rows does not establish that a mutation's output has a match in the subquery's scan. Add the self-referencing regression, an uncorrelated RETURNING subquery on a self-referencing table, and a read-only self-join control containing NULL values.Both join inputs are checked because either may contain a mutation.
CanMutatepropagates through relational expressions, so the restriction also applies when the mutation is below another operator. Returningfalsehere declines an unsupported proof and keeps the required outer-join behavior. The independent cross-join proof based on a guaranteed nonempty right input remains valid and is retained. Read-only inputs continue to use the existing self-join and foreign-key proofs. This is conservative for expressions containing an unrelated mutation: it may forgo an optimization opportunity; no performance benchmark was run.Validation:
returningSQL logic fixture passes on the final change in the single-node local configuration. The additions contain nine result assertions, including checks of stored data.21000.opt/memo,opt/norm, andopt/optbuilderpackage test executables: 220, 46, and 108 passing test/subtest entries respectively, with no failures or skips.crlfmt -fast -tab 2andgit diff --checkpass for both commits.The package executables were built with Bazel's
--config=testand run directly with their runfiles. The SQL fixture was run throughlogictest.RunLogicTest. The./dev generate,./dev lint --short, and./dev testentry points reject the root user in this environment, so those checks and the full repository test suite are not claimed as passing.