Thread: 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).
"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 targetcolumn. For example, INSERT INTO table_name ... ON CONFLICT DO UPDATE SET table_name.col = 1 is invalid (this followsthe 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 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. 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
Attachment
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