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
release-22.2: opt: fix ambiguous column error for computed column expressions #105024
Conversation
702b4ef
to
f51f3f4
Compare
ef0976f
to
a1a73b1
Compare
Thanks for opening a backport. Please check the backport criteria before merging:
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
Add a brief release justification to the body of your PR to justify this backport. Some other things to consider:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach, @mgartner, and @msirek)
Probably worth letting this bake for a bit. |
a1a73b1
to
7ecb92f
Compare
7ecb92f
to
78c7591
Compare
This commit fixes a rare bug that caused "ambiguous column" errors for `INSERT .. ON CONFLICT .. DO UPDATE` statements. Consider the schema and statement: CREATE TABLE t ( id INT PRIMARY KEY, a INT DEFAULT 1, b INT, c INT AS (a + b) STORED ) INSERT INTO t(id) VALUES (0) ON CONFLICT (id) DO UPDATE SET b = 1 In the `SET` clause, an explicit reference to `a` is ambiguous. It could be a reference to `t.a`, the `a` of the conflicting row(s), or it could be a reference to `excluded.a`, the `a` of the insert row(s) from the `VALUES` clause. If the `SET` clause contained `b = a`, the result would be an "ambiguous column" error, which is correct and matches Postgres's behavior. With the `SET` clause as `b = 1`, there should be no ambiguity. However, prior to this commit, ambiguity arose when building an implicit `SET` expression to generate the new value for the computed column `c`. When building `a + b`, we used the same scope used to build the `b = 1` projection: the scope of the `SET` clause. Using this scope makes `a` ambiguous, even though it always refers to `t.a` - the `a` of the conflicting row(s). Normally, this ambiguity is avoided by calling `mutationBuilder.disambiguateColumns`, which is called before computed column expressions are built. However, it's not effective in this specific edge case where the new value of `b` and the default value of `a` are the same expression, `1`. In this case, we conserve column IDs by using the same ID for both, which prevents `mutationBuilder.disambiguateColumns` from removing the ambiguity in the `SET` clause scope. The bug has been fixed by creating a new scope for resolving column references in computed column expressions. The scope contains no columns with duplicate names, so ambiguity is impossible. I believe the long-term fix is to avoid using scopes when building these synthesized expressions. I attempted to do this, but there are many tricky edge cases to iron out and the change quickly grew beyond the size of a comfortable backport. This future work is tracked by #61298. Fixes #102909 Release note (bug fix): A bug has been fixed that caused `INSERT .. ON CONFLICT .. DO UPDATE` queries to incorrectly result in an "ambiguous column" error. The bug only presented if the target table had a computed column with an expression referencing a column with a DEFAULT value.
78c7591
to
bc04436
Compare
Backport 1/1 commits from #104924 on behalf of @mgartner.
/cc @cockroachdb/release
This commit fixes a rare bug that caused "ambiguous column" errors for
INSERT .. ON CONFLICT .. DO UPDATE
statements.Consider the schema and statement:
In the
SET
clause, an explicit reference toa
is ambiguous. It couldbe a reference to
t.a
, thea
of the conflicting row(s), or it couldbe a reference to
excluded.a
, thea
of the insert row(s) from theVALUES
clause. If theSET
clause containedb = a
, the result wouldbe an "ambiguous column" error, which is correct and matches Postgres's
behavior.
With the
SET
clause asb = 1
, there should be no ambiguity. However,prior to this commit, ambiguity arose when building an implicit
SET
expression to generate the new value for the computed column
c
. Whenbuilding
a + b
, we used the same scope used to build theb = 1
projection: the scope of the
SET
clause. Using this scope makesa
ambiguous, even though it always refers to
t.a
- thea
of theconflicting row(s).
Normally, this ambiguity is avoided by calling
mutationBuilder.disambiguateColumns
, which is called before computedcolumn expressions are built. However, it's not effective in this
specific edge case where the new value of
b
and the default value ofa
are the same expression,1
. In this case, we conserve column IDsby using the same ID for both, which prevents
mutationBuilder.disambiguateColumns
from removing the ambiguity in theSET
clause scope.The bug has been fixed by creating a new scope for resolving column
references in computed column expressions. The scope contains no columns
with duplicate names, so ambiguity is impossible.
I believe the long-term fix is to avoid using scopes when building these
synthesized expressions. I attempted to do this, but there are many
tricky edge cases to iron out and the change quickly grew beyond the
size of a comfortable backport. This future work is tracked by #61298.
Fixes #102909
Release note (bug fix): A bug has been fixed that caused
INSERT .. ON CONFLICT .. DO UPDATE
queries to incorrectly result in an"ambiguous column" error. The bug only presented if the target table had
a computed column with an expression referencing a column with a DEFAULT
value.
Release justification: Fix for a bug that causes ambiguous column reference
errors for some INSERTs.