I've read through all the error messages in the patch. Some comments:
====
postgres=# create table foo (id int4);
CREATE TABLE
postgres=# create unique index foo_y on foo (id) where id > 0;
CREATE INDEX
postgres=# insert into foo values (-1) on conflict (id) where id > 0 do
nothing;
ERROR: inferred arbiter partial unique index's predicate does not cover
tuple proposed for insertion
DETAIL: ON CONFLICT inference clause implies that the tuple proposed
for insertion must be covered by the predicate of partial index "foo_y".
I'm surprised. If the inserted value doesn't match the WHERE clause of
the constraint, there is clearly no conflict, so I would assume the
above to work without error.
====
postgres=# create table foo (id int4 primary key, col2 int4, constraint
col2_unique unique (col2) deferrable);
CREATE TABLE
postgres=# insert into foo values (2) on conflict on foo_pkey do nothing;
ERROR: ON CONFLICT is not supported on relations with deferred unique
constraints/exclusion constraints
Why not? I would understand if the specified arbiter constraint is
deferred, but why does it matter if one of the other constraints is?
====
postgres=# create table foo (id int4 primary key);
CREATE TABLE
postgres=# begin isolation level serializable;
BEGIN
postgres=# select 1; -- to get the snapshot
?column?
----------
1
(1 row)
<in another session, insert 1>
postgres=# insert into foo values (1) on conflict on foo_pkey do nothing;
ERROR: could not serialize access due to concurrent insert or update
directing alternative ON CONFLICT path
What about a delete? Don't you get a serialization error, if another
backend deletes the conflicting tuple, and you perform an INSERT based
on the effects of the deleting transaction, which is not supposed to be
visible to you?
The error message is quite long. How about just giving the default
"could not serialize access due to concurrent update" message? I don't
think the "directing alternative ON CONFLICT path" really helps the user.
====
postgres=# insert into foo values (1), (2) on conflict on foo_pkey do
update set id = 2;
ERROR: ON CONFLICT DO UPDATE command could not lock/update
self-inserted tuple
HINT: Ensure that no rows proposed for insertion within the same
command have duplicate constrained values.
"lock/update" doesn't sound good. If the system knows which it is,
should say so. But in this case, locking the row just an implementation
detail. We're performing the UPDATE when that error happens, not locking.
How about:
ERROR: could not update tuple that was inserted in the same command
HINT: Ensure that the rows being inserted do not conflict with each other.
BTW, the tuple might be an updated version of an existing tuple, i.e.
the ON CONFLICT DO UPDATE ... was fired on an earlier row. Not
necessarily a row that was inserted in the same command. The message is
technically correct, but you need to understand the difference between
tuple and a row. Actually, should we avoid using the term "tuple"
altogether in user-facing errors?
====
postgres=# insert into foo values (1) on conflict (xmin) do nothing;
ERROR: system columns may not appear in unique index inference
specification
"may" is usually not the right word to use in error messages, as it
implies permission to do something (see style guide). It's not totally
inappropriate in this case, but how about:
ERROR: system columns cannot be used in an ON CONFLICT clause
====
postgres=# create table foo (id int4, constraint xcheck CHECK (id >
0));CREATE TABLE
postgres=# insert into foo values (1) on conflict on xcheck do nothing;
ERROR: constraint in ON CONFLICT clause has no associated index
How about:
ERROR: "xcheck" is a CHECK constraint
DETAIL: Only unique or exclusion constraints can be used in ON CONFLICT
ON clause.
That's assuming that "xcheck" actually is a CHECK constraint. Similarly
for a foreign key constraint.
====
postgres=# create table foo (id int4, constraint foo_x exclude using
gist (id with =) );
CREATE TABLE
postgres=# insert into foo values (1) on conflict on foo_x do update
set id=-1;
ERROR: "foo_x" is not a unique index
DETAIL: ON CONFLICT DO UPDATE requires unique index as arbiter.
I think it would be better to refer to the constraint here, rather than
the index. The user specified the constraint, the fact that it's backed
by an index is just an implementation detail.
Actually, the user specified an exclusion constraint, and expected DO
UPDATE to work with that. But we don't support that. So we should say:
ERROR: ON CONFLICT ... DO UPDATE not supported with exclusion constraints
====
postgres=# create table foo (id int4 primary key, t text);
CREATE TABLE
postgres=# insert into foo values (1) on conflict (t) do nothing;
ERROR: could not infer which unique index to use from
expressions/columns and predicate provided for ON CONFLICT
I think we should assume that the user listed the columns correctly, but
there is no constraint on them. The current message implies that the
list of columns was wrong. Either case is possible, of course, but I'd
suggest:
ERROR: there is no unique or exclusion constraint matching the ON
CONFLICT specification
====
postgres=# insert into foo values (1) on conflict (t nulls first) do
nothing;
ERROR: ON CONFLICT does not accept ordering or NULLS FIRST/LAST
specifications
LINE 1: insert into foo values (1) on conflict (t nulls first) do n...
^
HINT: These factors do not affect uniqueness of indexed datums.
I'd suggest:
ERROR: NULLS FIRST/LAST is not allowed in ON CONFLICT clause
and just leave out the hint. Or say something along the lines "You can
just leave it out".
Any chance the errlocation could point to the actual NULLS FIRST/LAST
clause, not the paren starting the column list? And ERRCODE_SYNTAX_ERROR
would be more appropriate for this.
====
postgres=# insert into foo values (1) on conflict do update set count =
excluded.count + 1;
ERROR: ON CONFLICT DO UPDATE requires explicit arbiter index
LINE 1: insert into foo values (1) on conflict do update set count ...
^
"Hmm, so where do I stick the index then?" or "But I just created the
index! Why can't the system find it?"
Can we avoid exposing the term "arbiter index" to the user? From a
user's point of view, he cannot specify an index directly. He can
specify a list of columns and an optional WHERE clause, or a constraint
name.
How about:
ERROR: DO UPDATE requires a constraint name or index specification
HINT: For example, ON CONFLICT ON <constraint> or ON CONFLICT (<column>)
====
postgres=# insert into pg_roles values ('myrole') on conflict do nothing;
ERROR: ON CONFLICT not supported with catalog relations
LINE 1: insert into pg_roles values ('myrole') on conflict do nothi...
^
I think the more common term used in error messages is "system catalog
table"
====
postgres=# create table foo (id int4 primary key);
CREATE TABLE
postgres=# create rule insert_foo as on insert to foo do also insert
into bar values (-1);
CREATE RULE
postgres=# insert into foo values (1) on conflict on foo_pkey do update
set id=1;
ERROR: INSERT with ON CONFLICT clause may not target relation with
INSERT or UPDATE rules
That's not strictly true. A "DO INSTEAD NOTHING" rule doesn't prevent ON
CONFLICT from working, for example. Not sure how to phrase that into the
message without making it too long.
"may" is again not very appropriate here. Should be "cannot". And the
term "target relation" might not be very clear to an average user.
====
Phew, that was quite a list..
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
