PG12 change to DO UPDATE SET column references
Hello,
I realize this is almost ancient history at this point, but I ran into
a surprising behavior change from PG11->12 with ON CONFLICT ... DO
UPDATE SET ...
Suppose I have this table:
create table foo (id int primary key);
On PG11 this works:
postgres=# insert into foo (id) values (1) on conflict (id) do update
set foo.id = 1;
INSERT 0 1
But on PG12+ this is the result:
postgres=# insert into foo (id) values (1) on conflict (id) do update
set foo.id = 1;
ERROR: column "foo" of relation "foo" does not exist
LINE 1: ...oo (id) values (1) on conflict (id) do update set foo.id = 1...
Making this more confusing is the fact that if I want to do something
like "SET bar = foo.bar + 1" the table qualification cannot be present
on the setting column but is required on the reading column.
There isn't anything in the docs that I see about this, and I don't
see anything scanning the release notes for PG12 either (though I
could have missed a keyword to search for).
Was this intended? Or a side effect? And should we explicitly document
the expectations here
The error is also pretty confusing: when you miss the required
qualification on the read column the error is more understandable:
ERROR: column reference "bar" is ambiguous
It seems to me that it'd be desirable to either allow the unnecessary
qualification or give an error that's more easily understood.
Regards,
James Coleman
On Fri, Jan 19, 2024 at 10:01 AM James Coleman <jtc331@gmail.com> wrote:
Making this more confusing is the fact that if I want to do something
like "SET bar = foo.bar + 1" the table qualification cannot be present
on the setting column but is required on the reading column.There isn't anything in the docs that I see about this, and I don't
see anything scanning the release notes for PG12 either (though I
could have missed a keyword to search for).
https://www.postgresql.org/docs/12/sql-insert.html
"When referencing a column with ON CONFLICT DO UPDATE, do not include the
table's name in the specification of a target column. For example, INSERT
INTO table_name ... ON CONFLICT DO UPDATE SET table_name.col = 1 is invalid
(this follows the general behavior for UPDATE)."
The same text exists for v11.
David J.
On Fri, Jan 19, 2024 at 1:53 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
On Fri, Jan 19, 2024 at 10:01 AM James Coleman <jtc331@gmail.com> wrote:
Making this more confusing is the fact that if I want to do something
like "SET bar = foo.bar + 1" the table qualification cannot be present
on the setting column but is required on the reading column.There isn't anything in the docs that I see about this, and I don't
see anything scanning the release notes for PG12 either (though I
could have missed a keyword to search for).https://www.postgresql.org/docs/12/sql-insert.html
"When referencing a column with ON CONFLICT DO UPDATE, do not include the table's name in the specification of a target column. For example, INSERT INTO table_name ... ON CONFLICT DO UPDATE SET table_name.col = 1 is invalid (this follows the general behavior for UPDATE)."
The same text exists for v11.
Well, egg on my face for definitely missing that in the docs.
Unfortunately that doesn't explain why it works on PG11 and not on PG12.
Regards,
James Coleman
On Saturday, January 20, 2024, James Coleman <jtc331@gmail.com> wrote:
Well, egg on my face for definitely missing that in the docs.
Unfortunately that doesn't explain why it works on PG11 and not on PG12.
It was a bug that got fixed. I’m sure a search of the mailing list
archives or Git will turn up the relevant discussion when it happened;
though I suppose it may just have been a refactoring to leverage the
consistency with update and no one realized they were fixing a bug at the
time.
David J.
James Coleman <jtc331@gmail.com> writes:
Suppose I have this table:
create table foo (id int primary key);
On PG11 this works:
postgres=# insert into foo (id) values (1) on conflict (id) do update
set foo.id = 1;
INSERT 0 1
Hmm, are you sure about that? I get
ERROR: column "foo" of relation "foo" does not exist
LINE 2: on conflict (id) do update set foo.id = 1;
^
in every branch back to 9.5 where ON CONFLICT was introduced.
I'm checking branch tip in each case, so conceivably this is
something that was changed post-11.0, but I kinda doubt we
would have back-patched it.
regards, tom lane
On Sat, Jan 20, 2024 at 11:12 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
James Coleman <jtc331@gmail.com> writes:
Suppose I have this table:
create table foo (id int primary key);On PG11 this works:
postgres=# insert into foo (id) values (1) on conflict (id) do update
set foo.id = 1;
INSERT 0 1Hmm, are you sure about that? I get
ERROR: column "foo" of relation "foo" does not exist
LINE 2: on conflict (id) do update set foo.id = 1;
^in every branch back to 9.5 where ON CONFLICT was introduced.
I'm checking branch tip in each case, so conceivably this is
something that was changed post-11.0, but I kinda doubt we
would have back-patched it.
Hmm, I just tested it on the official 11.15 docker image and couldn't
reproduce it. That leads me to believe that the difference isn't in
PG11 vs. 12, but rather in 2ndQuadrant Postgres (which we are running
for PG11, but are not using for > 11). Egg on my face twice in this
thread.
I do wonder if it's plausible (and sufficiently easy) to improve the
error message here. "column 'foo' of relation 'foo'" makes one thing
that you've written foo.foo, (in my real-world case the error message
also cut off the sql past "foo.", and so I couldn't even tell if the
sql was just malformed). At the very least it'd be nice to have a HINT
here (perhaps just when the relation and column name match).
Before I look at where it is, Is such an improvement something we'd be
interested in?
Regards,
James Coleman
James Coleman <jtc331@gmail.com> writes:
I do wonder if it's plausible (and sufficiently easy) to improve the
error message here. "column 'foo' of relation 'foo'" makes one thing
that you've written foo.foo, (in my real-world case the error message
also cut off the sql past "foo.", and so I couldn't even tell if the
sql was just malformed). At the very least it'd be nice to have a HINT
here (perhaps just when the relation and column name match).
Before I look at where it is, Is such an improvement something we'd be
interested in?
A HINT if the bogus column name (1) matches the relation name and
(2) is field-qualified seems plausible to me. Then it's pretty
likely to be a user misunderstanding about whether to write the
relation name.
regards, tom lane
On Sat, Jan 20, 2024 at 12:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
James Coleman <jtc331@gmail.com> writes:
I do wonder if it's plausible (and sufficiently easy) to improve the
error message here. "column 'foo' of relation 'foo'" makes one thing
that you've written foo.foo, (in my real-world case the error message
also cut off the sql past "foo.", and so I couldn't even tell if the
sql was just malformed). At the very least it'd be nice to have a HINT
here (perhaps just when the relation and column name match).Before I look at where it is, Is such an improvement something we'd be
interested in?A HINT if the bogus column name (1) matches the relation name and
(2) is field-qualified seems plausible to me. Then it's pretty
likely to be a user misunderstanding about whether to write the
relation name.
Attached is a patch to do just that. We could also add tests for
regular UPDATEs if you think that's useful.
Regards,
James Coleman
Attachments:
v1-0001-Helpful-hint-for-qualified-target-column-in-UPDAT.patchapplication/octet-stream; name=v1-0001-Helpful-hint-for-qualified-target-column-in-UPDAT.patchDownload
From f7cca7a65f869e988cd21d9fe52e8fd96198ada8 Mon Sep 17 00:00:00 2001
From: jcoleman <jtc331@gmail.com>
Date: Sat, 20 Jan 2024 16:40:45 -0500
Subject: [PATCH v1] Helpful hint for qualified target column in UPDATE
Target columns in UPDATE ... SET must not be qualified with the target
table. The error message when a query contains this mistake is
unnecessarily confusing: "column "foo" of relation "foo" does not
exist". In the case where the column is qualified and the qualification
matches the table name (or alias) we can help the user by reminding them
of the syntax restriction for target columns.
---
src/backend/parser/analyze.c | 25 ++++++++++++----
src/test/regress/expected/insert_conflict.out | 29 +++++++++++++++++++
src/test/regress/sql/insert_conflict.sql | 10 +++++++
3 files changed, 58 insertions(+), 6 deletions(-)
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 06fc8ce98b..a93bf9bdb2 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -2513,12 +2513,25 @@ transformUpdateTargetList(ParseState *pstate, List *origTlist)
attrno = attnameAttNum(pstate->p_target_relation,
origTarget->name, true);
if (attrno == InvalidAttrNumber)
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_COLUMN),
- errmsg("column \"%s\" of relation \"%s\" does not exist",
- origTarget->name,
- RelationGetRelationName(pstate->p_target_relation)),
- parser_errposition(pstate, origTarget->location)));
+ {
+ char *relname = RelationGetRelationName(pstate->p_target_relation);
+ char *aliasname = pstate->p_target_nsitem->p_names->aliasname;
+
+ if (origTarget->indirection != NIL &&
+ strcmp(origTarget->name, aliasname) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("column \"%s\" of relation \"%s\" does not exist",
+ origTarget->name, relname),
+ errhint("target columns cannot be qualified with the target relation name"),
+ parser_errposition(pstate, origTarget->location)));
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("column \"%s\" of relation \"%s\" does not exist",
+ origTarget->name, relname),
+ parser_errposition(pstate, origTarget->location)));
+ }
updateTargetListEntry(pstate, tle, origTarget->name,
attrno,
diff --git a/src/test/regress/expected/insert_conflict.out b/src/test/regress/expected/insert_conflict.out
index 9e9e3bd00c..a22eae4da7 100644
--- a/src/test/regress/expected/insert_conflict.out
+++ b/src/test/regress/expected/insert_conflict.out
@@ -272,6 +272,35 @@ ERROR: invalid reference to FROM-clause entry for table "insertconflicttest"
LINE 1: ...onfruit') on conflict (key) do update set fruit = insertconf...
^
HINT: Perhaps you meant to reference the table alias "ict".
+-- Helpful hint when qualifying set column with target table
+insert into insertconflicttest values (3, 'Kiwi') on conflict (key, fruit) do update set insertconflicttest.fruit = 'Mango';
+ERROR: column "insertconflicttest" of relation "insertconflicttest" does not exist
+LINE 1: ...3, 'Kiwi') on conflict (key, fruit) do update set insertconf...
+ ^
+HINT: target columns cannot be qualified with the target relation name
+insert into insertconflicttest AS ict values (3, 'Kiwi') on conflict (key, fruit) do update set ict.fruit = 'Mango';
+ERROR: column "ict" of relation "insertconflicttest" does not exist
+LINE 1: ...3, 'Kiwi') on conflict (key, fruit) do update set ict.fruit ...
+ ^
+HINT: target columns cannot be qualified with the target relation name
+-- ... but not when the column doesn't match the table name.
+insert into insertconflicttest values (3, 'Kiwi') on conflict (key, fruit) do update set insertconflicttest2.fruit = 'Mango';
+ERROR: column "insertconflicttest2" of relation "insertconflicttest" does not exist
+LINE 1: ...3, 'Kiwi') on conflict (key, fruit) do update set insertconf...
+ ^
+insert into insertconflicttest AS ict values (3, 'Kiwi') on conflict (key, fruit) do update set ict2.fruit = 'Mango';
+ERROR: column "ict2" of relation "insertconflicttest" does not exist
+LINE 1: ...3, 'Kiwi') on conflict (key, fruit) do update set ict2.fruit...
+ ^
+-- ... also not when the column isn't qualified but matches the table name.
+insert into insertconflicttest values (3, 'Kiwi') on conflict (key, fruit) do update set insertconflicttest = 'Mango';
+ERROR: column "insertconflicttest" of relation "insertconflicttest" does not exist
+LINE 1: ...3, 'Kiwi') on conflict (key, fruit) do update set insertconf...
+ ^
+insert into insertconflicttest AS ict values (3, 'Kiwi') on conflict (key, fruit) do update set ict = 'Mango';
+ERROR: column "ict" of relation "insertconflicttest" does not exist
+LINE 1: ...3, 'Kiwi') on conflict (key, fruit) do update set ict = 'Man...
+ ^
drop index key_index;
--
-- Composite key tests
diff --git a/src/test/regress/sql/insert_conflict.sql b/src/test/regress/sql/insert_conflict.sql
index 23d5778b82..435ae42024 100644
--- a/src/test/regress/sql/insert_conflict.sql
+++ b/src/test/regress/sql/insert_conflict.sql
@@ -118,6 +118,16 @@ insert into insertconflicttest AS ict values (6, 'Passionfruit') on conflict (ke
insert into insertconflicttest AS ict values (6, 'Passionfruit') on conflict (key) do update set fruit = ict.fruit; -- ok, alias
insert into insertconflicttest AS ict values (6, 'Passionfruit') on conflict (key) do update set fruit = insertconflicttest.fruit; -- error, references aliased away name
+-- Helpful hint when qualifying set column with target table
+insert into insertconflicttest values (3, 'Kiwi') on conflict (key, fruit) do update set insertconflicttest.fruit = 'Mango';
+insert into insertconflicttest AS ict values (3, 'Kiwi') on conflict (key, fruit) do update set ict.fruit = 'Mango';
+-- ... but not when the column doesn't match the table name.
+insert into insertconflicttest values (3, 'Kiwi') on conflict (key, fruit) do update set insertconflicttest2.fruit = 'Mango';
+insert into insertconflicttest AS ict values (3, 'Kiwi') on conflict (key, fruit) do update set ict2.fruit = 'Mango';
+-- ... also not when the column isn't qualified but matches the table name.
+insert into insertconflicttest values (3, 'Kiwi') on conflict (key, fruit) do update set insertconflicttest = 'Mango';
+insert into insertconflicttest AS ict values (3, 'Kiwi') on conflict (key, fruit) do update set ict = 'Mango';
+
drop index key_index;
--
--
2.39.3 (Apple Git-145)
James Coleman <jtc331@gmail.com> writes:
On Sat, Jan 20, 2024 at 12:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
A HINT if the bogus column name (1) matches the relation name and
(2) is field-qualified seems plausible to me. Then it's pretty
likely to be a user misunderstanding about whether to write the
relation name.
Attached is a patch to do just that. We could also add tests for
regular UPDATEs if you think that's useful.
Pushed with minor alterations:
1. I think our usual style for conditional hints is to use a ternary
expression within the ereport, rather than duplicating code. In this
case that way allows not touching any of the existing lines, making
review easier.
2. I thought we should test the UPDATE case as well as the ON CONFLICT
case, but I didn't think we needed quite as many tests as you had
here. I split up the responsibility so that one test covers the
alias case and the other the no-alias case.
regards, tom lane
On Sat, Jan 20, 2024 at 5:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
James Coleman <jtc331@gmail.com> writes:
On Sat, Jan 20, 2024 at 12:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
A HINT if the bogus column name (1) matches the relation name and
(2) is field-qualified seems plausible to me. Then it's pretty
likely to be a user misunderstanding about whether to write the
relation name.Attached is a patch to do just that. We could also add tests for
regular UPDATEs if you think that's useful.Pushed with minor alterations:
1. I think our usual style for conditional hints is to use a ternary
expression within the ereport, rather than duplicating code. In this
case that way allows not touching any of the existing lines, making
review easier.
Ah, I'd wondered if we had a pattern for that, but I didn't know what
I was looking for.
2. I thought we should test the UPDATE case as well as the ON CONFLICT
case, but I didn't think we needed quite as many tests as you had
here. I split up the responsibility so that one test covers the
alias case and the other the no-alias case.
That all makes sense. I figured it was better to have tests show all
the possible combinations for review and then you could whittle them
down as you saw fit.
Thanks for reviewing and committing!
Regards,
James Coleman