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

optbuilder: fix ambiguous column references for FK cascades #57153

Merged
merged 1 commit into from Nov 30, 2020

Conversation

mgartner
Copy link
Collaborator

This commit fixes an issue in optbuilder that caused "ambiguous column
reference" errors. This error would be produced during cascading updates
if a child table's reference column name was equal to the parent column
name concatenated with _new, and the child table had a check
constraint, computed column, or partial index predicate that referenced
the column.

For example, the following UPDATE statement would produce an error.
The expected behavior is a successful UPDATE. Notice that p_new of
the child table references p of the parent table.

CREATE TABLE parent (p INT PRIMARY KEY)

CREATE TABLE child (
  c INT PRIMARY KEY,
  p_new INT REFERENCES parent(p) ON UPDATE CASCADE,
  CHECK (p_new > 0)
)

UPDATE parent SET p = p * 10 WHERE p > 1

This issue was the result of incorrect scoping while building foreign
key cascading update expressions. A column with the same name and column
ID was added to the update expression's input scope. Because the
mutationBuilder.disambiguateColumns function is unable to disambiguate
columns with the same name and column ID, building any expression that
referenced the duplicated column would result in an error.

This commit fixes the issue by no longer duplicating columns in the
update expression's input scope. mutationBuilder.addUpdateCols now
detects the special case when the update expression is a *scopeColumn
and avoids duplicating it in the generated projection scope.

Fixes #57148

Release note (bug fix): A bug has been fix that caused an "ambiguous
column reference" error during foreign key cascading updates. This error
was incorrectly produced when the child table's reference column name
was equal to the concatenation of the parent's reference column name and
"_new", and when the child table had a CHECK constraint, computed
column, or partial index predicate expression that referenced the
column. This bug was introduce in version 20.2.

@mgartner mgartner requested a review from a team as a code owner November 26, 2020 00:34
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner mgartner added the backport-20.2.x Flags PRs that need to be backported to 20.2 label Nov 26, 2020
Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Thanks for getting to the bottom of this.

Please add a TODO(radu) to onUpdateCascadeBuilder.Build (above updateExprs[i].Expr = &newValScopeCols[i]) to find a cleaner way to do this, as this requires special code in addUpdateCols.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde and @rytaft)

This commit fixes an issue in optbuilder that caused "ambiguous column
reference" errors. This error would be produced during cascading updates
if a child table's reference column name was equal to the parent column
name concatenated with `_new`, and the child table had a check
constraint, computed column, or partial index predicate that referenced
the column.

For example, the following `UPDATE` statement would produce an error.
The expected behavior is a successful `UPDATE`. Notice that `p_new` of
the child table references `p` of the parent table.

    CREATE TABLE parent (p INT PRIMARY KEY)

    CREATE TABLE child (
      c INT PRIMARY KEY,
      p_new INT REFERENCES parent(p) ON UPDATE CASCADE,
      CHECK (p_new > 0)
    )

    UPDATE parent SET p = p * 10 WHERE p > 1

This issue was the result of incorrect scoping while building foreign
key cascading update expressions. A column with the same name and column
ID was added to the update expression's input scope. Because the
`mutationBuilder.disambiguateColumns` function is unable to disambiguate
columns with the same name and column ID, building any expression that
referenced the duplicated column would result in an error.

This commit fixes the issue by no longer duplicating columns in the
update expression's input scope. `mutationBuilder.addUpdateCols` now
detects the special case when the update expression is a `*scopeColumn`
and avoids duplicating it in the generated projection scope.

Fixes cockroachdb#57148

Release note (bug fix): A bug has been fix that caused an "ambiguous
column reference" error during foreign key cascading updates. This error
was incorrectly produced when the child table's reference column name
was equal to the concatenation of the parent's reference column name and
"_new", and when the child table had a CHECK constraint, computed
column, or partial index predicate expression that referenced the
column. This bug was introduce in version 20.2.
@mgartner mgartner force-pushed the 57148-fix-ambiguous-column-ref branch from ef316f4 to cefe554 Compare November 30, 2020 17:11
Copy link
Collaborator Author

@mgartner mgartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a TODO(radu) to onUpdateCascadeBuilder.Build (above updateExprs[i].Expr = &newValScopeCols[i]) to find a cleaner way to do this, as this requires special code in addUpdateCols.

Done. It's kind of a weird chicken-and-egg problem. I played around with other options, but this seemed the least invasive for now.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rytaft)

@mgartner
Copy link
Collaborator Author

TFTR!

bors r=RaduBerinde

@craig
Copy link
Contributor

craig bot commented Nov 30, 2020

Build failed:

@mgartner
Copy link
Collaborator Author

bors r=RaduBerinde

@craig
Copy link
Contributor

craig bot commented Nov 30, 2020

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Nov 30, 2020

Build succeeded:

@craig craig bot merged commit d5222dc into cockroachdb:master Nov 30, 2020
@mgartner mgartner deleted the 57148-fix-ambiguous-column-ref branch December 1, 2020 00:22
@rafiss rafiss added this to the 20.2 milestone Apr 22, 2021
mgartner added a commit to mgartner/cockroach that referenced this pull request Nov 4, 2021
This commit anonymizes the columns in the input of a foreign key
ON UPDATE CASCADE expression. This is safe because these columns can
only be referenced by other expressions if they are update columns, and
in that case, `mutationBuilder.addUpdateCascade` will give them a
distinct name in the scope it produces. This change allows a special
case added in cockroachdb#57153 to be removed, without failing the regression test.

Release note: None
mgartner added a commit to mgartner/cockroach that referenced this pull request Nov 8, 2021
This commit anonymizes the columns in the input of a foreign key
ON UPDATE CASCADE expression. This is safe because these columns can
only be referenced by other expressions if they are update columns, and
in that case, `mutationBuilder.addUpdateCascade` will give them a
distinct name in the scope it produces. This change allows a special
case added in cockroachdb#57153 to be removed, without failing the regression test.

Release note: None
craig bot pushed a commit that referenced this pull request Nov 8, 2021
72358: ccl/server/sql: Fix failing reset index usage unit test r=lindseyjin a=lindseyjin

Resolves #72254

Previously, the unit test for resetting index usage statistics was
failing. This was happening because of two reasons:
1. The IndexUsageStatsController was not being set properly for DistSQL
server configs.
2. Noise from other tests was sometimes populating the index usage
statistics table right after it was reset, causing the check for a newly
reset table to sometimes randomly fail.

To fix these issues, we have implemented the following:
1. Created a test case to reproduce the first error, and correctly
populated IndexUsageStatsController for DistSQL server configs.
2. Updated the unit test to query for the specific test table id, to
avoid other tables polluting the test space. The test has also been
updated to check for a correct LastReset time.

Release note: none

72466: opt: clean up FK ON UPDATE CASCADE code and opt trees r=mgartner a=mgartner

#### opt: make FK ON UPDATE CASCADE input columns anonymous

This commit anonymizes the columns in the input of a foreign key
ON UPDATE CASCADE expression. This is safe because these columns can
only be referenced by other expressions if they are update columns, and
in that case, `mutationBuilder.addUpdateCascade` will give them a
distinct name in the scope it produces. This change allows a special
case added in #57153 to be removed, without failing the regression test.

Release note: None

#### opt: use more accurate FK ON UPDATE column metadata names

Columns produced by the WithScan expression in a foreign key ON UPDATE
CASCADE now have metadata names based on the column names of the child
table, rather than the parent table. This more accurately describes the
columns in the cascade plan because the plan is concerned with the child
table, not the parent. Metadata names are only used in opt expression
trees, so this is purely an aesthetic change, not a semantic one.

Release note: None


72493: sessionphase: fix service latency computation in an error case r=yuzefovich a=yuzefovich

If we encounter an error during the logical planning, then the end
execution phase is never set. Previously, we would use that unset value
in order to compute the latency of planning and execution and would get
a negative result. This is now fixed.

Release note: None

72503: protectedts/ptstorage: deflake test r=ajwerner a=ajwerner

The test asserted that the error message appeared just once. If there's a retry
the error can appear in the trace more than once. This commit cleans up that
condition.

Fixes #67972

Release note: None

Co-authored-by: Lindsey Jin <lindsey.jin@cockroachlabs.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Andrew Werner <awerner32@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-20.2.x Flags PRs that need to be backported to 20.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opt: error when an UPDATE cascades to a table with a check constraint on an ambiguous column
4 participants