On Sat, Nov 29, 2025 at 6:02 AM Viktor Holmberg <[email protected]> wrote:
>
> Attaching v18 with the above changes. Thanks your continued reviews Jian!
>
hi.
I had some minor comments with doc, comments.
+ expressions or <replaceable>condition</replaceable>. If using a
+ <literal>WHERE</literal> with <literal>ON CONFLICT DO UPDATE /
SELECT </literal>,
+ you must have <literal>SELECT</literal> privilege on the columns referenced
+ in the <literal>WHERE</literal> clause.
the extra white space is not necessary, it should be:
<literal>ON CONFLICT DO UPDATE / SELECT</literal>
- list of <command>SELECT</command>. Only rows that were successfully
+ list of <command>SELECT</command>. If an <literal>ON CONFLICT DO
SELECT</literal>
+ clause is not present, only rows that were successfully
inserted or updated will be returned. For example, if a row was
locked but not updated because an <literal>ON CONFLICT DO UPDATE
... WHERE</literal> clause <replaceable
class="parameter">condition</replaceable> was not satisfied, the
- row will not be returned.
+ row will not be returned. <literal>ON CONFLICT DO SELECT</literal>
+ works similarly, except no update takes place.
I am not sure "except no update takes place." is appropriate here.
- rows. Similarly, if a <command>DELETE</command> is turned into an
+ rows. Similarly, in an <command>INSERT</command> with an
+ <literal>ON CONFLICT DO SELECT</literal> clause, you can look at
the old values to determine if your query inserted a row or not. If a
<command>DELETE</command> is turned into an
<command>UPDATE</command> by a <link
linkend="sql-createrule">rewrite rule</link>,
the new values may be non-<literal>NULL</literal>.
182 characters, line too long, we can wrap it into several lines.
src/backend/executor/execIndexing.c top have bunch of comments for
INSERT ... ON CONFLICT DO UPDATE/NOTHING
maybe we also need to write something for INSERT ON CONFLICT DO SELECT too.
src/backend/optimizer/plan/setrefs.c, fix_join_expr comments:
* 3) ON CONFLICT UPDATE SET/WHERE clauses. Here references to EXCLUDED are
* to be replaced with INNER_VAR references, while leaving target Vars (the
* to-be-updated relation) alone. Correspondingly inner_itlist is to be
* EXCLUDED elements, outer_itlist = NULL and acceptable_rel the target
* relation.
This applies to INSERT ON CONFLICT DO SELECT.
comments need slightly adjusted?
src/test/regress/sql/triggers.sql:
--
-- Verify behavior of before and after triggers with INSERT...ON CONFLICT
-- DO UPDATE
--
the above comments need change.
we can also add a new tests like:
``insert into upsert values(9, 'orange') on conflict (key) do select
for update returning old.*, new.*, upsert.*;``
then we can see how triggers behave with or without conflict.
https://git.postgresql.org/cgit/postgresql.git/commit/?id=2bc7e886fc1baaeee3987a141bff3ac490037d12
so we also need change infer_arbiter_indexes accordingly.
seems we only need adjust:
```
else if (indexOidFromConstraint != InvalidOid)
{
/*
* In the case of "ON constraint_name DO UPDATE" we need to skip
* non-unique candidates.
*/
if (!idxForm->indisunique && onconflict->action ==
ONCONFLICT_UPDATE)
continue;
}
```
src/test/regress/sql/updatable_views.sql tests line too long too, we can make it
into separate lines.
+insert into parted_conflict_test (a, b) values (1, 'x'), (2, 'y'),
(4, 'z') on conflict (a) do select returning *;
+
we can change it to
+insert into parted_conflict_test (a, b) values (1, 'x'), (2, 'y'), (4, 'z')
+ on conflict (a) do select returning *, tableoid::regclass;
then we can see that tableoid system is computed correctly in
ExecOnConflictSelect.
+ if (lockStrength == LCS_NONE)
+ {
+ if (!table_tuple_fetch_row_version(relation, conflictTid,
SnapshotAny, existing))
+ /* The pre-existing tuple was deleted */
+ return false;
+ }
i think this part should be
```
if (!table_tuple_fetch_row_version(rel, conflictTid, SnapshotAny, existing))
elog(ERROR, "failed to fetch conflicting tuple for ON CONFLICT");
```
say we have a conflict for values (1)
``insert into tsa values (1,3) on conflict(a) do select returning *;``
set a GDB breakpoint at ExecOnConflictSelect, let another process do
``delete from tsa; vacuum tsa;``
then let GDB continue.
table_tuple_fetch_row_version can still fetch the tuple.
so I think this is an unlikely scenario.
--
jian
https://www.enterprisedb.com
