Обсуждение: 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 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

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.

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

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