ON CONFLICT DO SELECT (take 3)

On Tue, 7 Oct 2025 at 12:57, Viktor Holmberg <v@viktorh.net> wrote:

This patch implements ON CONFLICT DO SELECT.
I’ve kept the patches proposed there separate, in case any of the people involved back then would like to pick it up again.

Grateful in advance to anyone who can help reviewing!

Thanks for picking this up. I haven't looked at it yet, but I'm
planning to do so.

In the meantime, I noticed that the cfbot didn't pick up your latest
patches, and is still running the v7 patches, presumably based on
their names. So here they are as v8 (rebased, plus a couple of
indentation fixes in 0003, but no other changes).

Regards,
Dean

Ah, must’ve been that I added the previous thread for referene on the commitfest entry. Thanks for sorting that out.
Looking forward to your review!

/Viktor

Thanks for the review Jian. Much appreciated. Apologies for the multiple email threads - just my email client mucking up the threads. This should hopefully bring them back to the mail thread.
I’ll go over it and make changes this week.
One question - why break out the OnConflictSet/ActionState rename to a separate commit? Previously, it only did Set (in update) so it’s naming did make sense.

On Mon, 17 Nov 2025 at 10:00, v@viktorh.net <v@viktorh.net> wrote:

One question - why break out the OnConflictSet/ActionState rename to a separate commit? Previously, it only did Set (in update) so it’s naming did make sense.

I know that some committers tend to prefer smaller commits than me,
but FWIW, I wouldn't bother splitting out something like this, since
it doesn't really provide any benefit by itself, or really make much
sense without the rest of the patch.

P.S. The convention on these mailing lists is to not top-post, but
instead reply in-line and trim, like this.

Regards,
Dean

I did some simple tests, found out that
SELECT FOR UPDATE, the lock mechanism seems to be working as intended.
We can add some tests on contrib/pgrowlocks to demonstrate that.

Test added.

infer_arbiter_indexes
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("ON CONFLICT DO UPDATE not supported
with exclusion constraints")));
I guess this works for ON CONFLICT SELECT?
we can leave some comments on the function infer_arbiter_indexes,
and also add some tests on src/test/regress/sql/constraints.sql after line 570.

Good catch. In fact it did “work” as in "not crash" - but I think it shouldn’t.
With exclusion constraints, a single insert can conflict with multiple rows.
I assumed that’s why DO UPDATE doesn’t work with it - because it’d update multiple rows.
For the same reason, I think DO SELECT shouldn’t work either, as you could then
get multiple rows back for a single insert.
I guess in both cases you could make it so it updates/selects all conflicting rows - but I’d
really prefer to leave it as an error. If someone actually wants this to work with exclusion
constraints the error can always be removed in a future version. But if we add a multi-row-return
then we are locked in forever.

changing
OnConflictSetState
to
OnConflictActionState
could make it a separate patch.

Skipping this for now, let me know if you strongly object.

all these 3 patches can be merged together, I think.

Ok done. But these review fixes are separate for ease of review. Before merging they should
be folded in to the main/first commit.

"/* DO NOTHING or UPDATE? */"
this comment needs to be changed?

Done

----------------------------------------
src/backend/rewrite/rewriteHandler.c
parsetree->onConflict->action == ONCONFLICT_UPDATE
maybe we also need to do some logic to the ONCONFLICT_SELECT
(I didn't check this part deeply)

Yes, this needed to be fixed to make updatable views work. Done.

If you compare it with the result above, it seems the updatable view behaves
inconsistent with ON CONFLICT DO SELECT versus ON CONFLICT DO UPDATE.

Yes, it was wrong. Nice catch. Fixed now I think, and test added.

I believe this new patch addresses all the issues found by Jian. I hope another review won’t find quite so much dirt!

On Mon, 17 Nov 2025 at 22:07, v@viktorh.net <v@viktorh.net> wrote:

I believe this new patch addresses all the issues found by Jian. I hope another review won’t find quite so much dirt!

The latest set of changes look reasonable to me, so I've squashed
those 2 commits together and made an initial stab at writing a more
complete commit message.

I made a quick pass over the code, and I'm attaching a few more
suggested updates. This is mostly cosmetic stuff (e.g., fixing a few
code comments that were overlooked), plus some minor refactoring to
reduce code duplication.

Regards,
Dean

On 19 Nov 2025 at 15:08 +0100, Dean Rasheed <dean.a.rasheed@gmail.com>, wrote:

I made a quick pass over the code, and I'm attaching a few more
suggested updates. This is mostly cosmetic stuff (e.g., fixing a few
code comments that were overlooked), plus some minor refactoring to
reduce code duplication.

Neat!
For the CASE default, elog(ERROR, "unrecognized LockClauseStrength %d” that was removed.
Would this now trigger a compile time error/warning? And are you supposed to get 0 warnings when compiling?
(I get a large amount of warnings "warning: 'pg_restrict' macro redefined" on master, but that could just be something with my environment)
More of a question, the changes are an improvement.

/Viktor

On Wed, 19 Nov 2025 at 16:51, Viktor Holmberg <v@viktorh.net> wrote:

For the CASE default, elog(ERROR, "unrecognized LockClauseStrength %d” that was removed.
Would this now trigger a compile time error/warning? And are you supposed to get 0 warnings when compiling?

That shouldn't trigger a warning, because there is a case block for
every enum element, and yes there should be 0 compiler warnings.

(I get a large amount of warnings "warning: 'pg_restrict' macro redefined" on master, but that could just be something with my environment)

I haven't seen that before, but there's this thread:

/messages/by-id/CA+FpmFdoa7O7yS3k7ZtqvA+hNWUA6YvJy6VvdYX1sGsryVQBNQ@mail.gmail.com

If you re-run configure, does it go away?

Regards,
Dean

On Tue, Oct 7, 2025 at 2:57 PM Viktor Holmberg <v@viktorh.net> wrote:

This patch is 85% the work of Andreas Karlsson and the reviewers (Dean Rasheed, Joel Jacobson, Kirill Reshke) in this thread: /messages/by-id/2b5db2e6-8ece-44d0-9890-f256fdca9f7e@proxel.se, which unfortunately seems to have stalled.

This was also based on my work, and according to Andreas the first
version was "very similar" to mine.

.m

On 19 Nov 2025 at 18:19 +0100, Dean Rasheed <dean.a.rasheed@gmail.com>, wrote:

On Wed, 19 Nov 2025 at 16:51, Viktor Holmberg <v@viktorh.net> wrote:

For the CASE default, elog(ERROR, "unrecognized LockClauseStrength %d” that was removed.
Would this now trigger a compile time error/warning? And are you supposed to get 0 warnings when compiling?

That shouldn't trigger a warning, because there is a case block for
every enum element, and yes there should be 0 compiler warnings.

Yes sorry, that’s what I meant! In that case, nice that those potential future errors are moved from runtime to compile time.

(I get a large amount of warnings "warning: 'pg_restrict' macro redefined" on master, but that could just be something with my environment)

I haven't seen that before, but there's this thread:

/messages/by-id/CA+FpmFdoa7O7yS3k7ZtqvA+hNWUA6YvJy6VvdYX1sGsryVQBNQ@mail.gmail.com

If you re-run configure, does it go away?

Regards,
Dean

Yes, re-configuring made the warning go away. Thanks for pointing me in the right direction.

On 11/19/25 8:06 PM, Marko Tiikkaja wrote:

On Tue, Oct 7, 2025 at 2:57 PM Viktor Holmberg <v@viktorh.net> wrote:

This patch is 85% the work of Andreas Karlsson and the reviewers (Dean Rasheed, Joel Jacobson, Kirill Reshke) in this thread: /messages/by-id/2b5db2e6-8ece-44d0-9890-f256fdca9f7e@proxel.se, which unfortunately seems to have stalled.

This was also based on my work, and according to Andreas the first
version was "very similar" to mine.

Yup! "My" patch was just Marko's patch rebased on PG17/PG18 and with
support for new PG features added, more tests and a couple of bugs
fixed. The original idea and huge parts of the patch are indeed Marko's.

I hope I never gave the impression otherwise. :)

Andreas

On Wed, Nov 19, 2025 at 10:08 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

The latest set of changes look reasonable to me, so I've squashed
those 2 commits together and made an initial stab at writing a more
complete commit message.

hi.

"Note that exclusion constraints are not supported as arbiters with ON
CONFLICT DO UPDATE."
this sentence need update?

"""
If the INSERT command contains a RETURNING clause, the result will be similar to
that of a SELECT statement containing the columns and values defined in the
RETURNING list, computed over the row(s) inserted or updated by the command.
"""
this sentence need update?

 HINT:  Perhaps you meant to reference the column "excluded.fruit".
 -- inference fails:
-insert into insertconflicttest values (3, 'Kiwi') on conflict (key,
fruit) do update set fruit = excluded.fruit;
+insert into insertconflicttest values (4, 'Kiwi') on conflict (key,
fruit) do update set fruit = excluded.fruit;
 ERROR:  there is no unique or exclusion constraint matching the ON
CONFLICT specification
-insert into insertconflicttest values (4, 'Mango') on conflict
(fruit, key) do update set fruit = excluded.fruit;
+insert into insertconflicttest values (5, 'Mango') on conflict
(fruit, key) do update set fruit = excluded.fruit;
 ERROR:  there is no unique or exclusion constraint matching the ON
CONFLICT specification
-insert into insertconflicttest values (5, 'Lemon') on conflict
(fruit) do update set fruit = excluded.fruit;
+insert into insertconflicttest values (6, 'Lemon') on conflict
(fruit) do update set fruit = excluded.fruit;
 ERROR:  there is no unique or exclusion constraint matching the ON
CONFLICT specification
-insert into insertconflicttest values (6, 'Passionfruit') on conflict
(lower(fruit)) do update set fruit = excluded.fruit;
+insert into insertconflicttest values (7, 'Passionfruit') on conflict
(lower(fruit)) do update set fruit = excluded.fruit;

all these changes is not necessary, we can just add
+truncate insertconflicttest;
+insert into insertconflicttest values (1, 'Apple'), (2, 'Orange');
after line
explain (costs off) insert into insertconflicttest values (1, 'Apple')
on conflict (key) do select for key share returning *;
to make table insertconflicttest have the same content as before ON
CONFLICT DO SELECT.

it seems we didn't test ExecInitPartitionInfo related changes,
I've attached a simple test for it.

https://www.postgresql.org/docs/current/ddl-priv.html#DDL-PRIV-UPDATE
says:
```
SELECT ... FOR UPDATE and SELECT ... FOR SHARE also require this privilege on at
least one column, in addition to the SELECT privilege.
```
I attached extensive permission tests for ON CONFLICT DO SELECT

in ExecOnConflictSelect, i change it to:
```
if (!table_tuple_fetch_row_version(relation, conflictTid,
SnapshotAny, existing))
{
elog(INFO, "this part reached");
return false;
}
```
all isolation tests passed, this seems unlikely to be reachable.

--
jian
https://www.enterprisedb.com/

On 11/19/25 8:06 PM, Marko Tiikkaja wrote:

On Tue, Oct 7, 2025 at 2:57 PM Viktor Holmberg <v@viktorh.net> wrote:

This patch is 85% the work of Andreas Karlsson and the reviewers (Dean Rasheed, Joel Jacobson, Kirill Reshke) in this thread: /messages/by-id/2b5db2e6-8ece-44d0-9890-f256fdca9f7e@proxel.se, which unfortunately seems to have stalled.

This was also based on my work, and according to Andreas the first
version was "very similar" to mine.

Ah, sorry about that. I didn't go back through the old threads carefully enough.

On Thu, 20 Nov 2025 at 01:29, Andreas Karlsson <andreas@proxel.se> wrote:

Yup! "My" patch was just Marko's patch rebased on PG17/PG18 and with
support for new PG features added, more tests and a couple of bugs
fixed. The original idea and huge parts of the patch are indeed Marko's.

OK, got it. So author credit for this patch should go to Marko,
Andreas, and Viktor? In that order? And I should add the original
thread to the commit message.

Regards,
Dean

On Thu, Nov 20, 2025 at 10:50 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

On Thu, 20 Nov 2025 at 01:29, Andreas Karlsson <andreas@proxel.se> wrote:

Yup! "My" patch was just Marko's patch rebased on PG17/PG18 and with
support for new PG features added, more tests and a couple of bugs
fixed. The original idea and huge parts of the patch are indeed Marko's.

OK, got it. So author credit for this patch should go to Marko,
Andreas, and Viktor? In that order? And I should add the original
thread to the commit message.

I think Andreas gets first author on this one.

Thanks!

.m

On 20 Nov 2025 at 09:50 +0100, Marko Tiikkaja <marko@joh.to>, wrote:

On Thu, Nov 20, 2025 at 10:50 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

On Thu, 20 Nov 2025 at 01:29, Andreas Karlsson <andreas@proxel.se> wrote:

Yup! "My" patch was just Marko's patch rebased on PG17/PG18 and with
support for new PG features added, more tests and a couple of bugs
fixed. The original idea and huge parts of the patch are indeed Marko's.

OK, got it. So author credit for this patch should go to Marko,
Andreas, and Viktor? In that order? And I should add the original
thread to the commit message.

I think Andreas gets first author on this one.

Thanks!

.m

I don’t mind the author credit order, I just want to delete the
SELECT INSERT SELECT dances from my code. (in 2 years or
 however long it will take for PG19 to trickle down to heroku).
So all fine by me!
/Viktor

https://www.postgresql.org/docs/current/ddl-priv.html#DDL-PRIV-UPDATE
says:
```
SELECT ... FOR UPDATE and SELECT ... FOR SHARE also require this privilege on at
least one column, in addition to the SELECT privilege.
```
I attached extensive permission tests for ON CONFLICT DO SELECT

+   For <literal>ON CONFLICT DO SELECT</literal>, <literal>SELECT</literal>
+   privilege is required on any column whose values are read in the
+   <replaceable>condition</replaceable>.
If you do <literal>ON CONFLICT DO SELECT FOR UPDATE</literal>
then <literal>UPDATE</literal> permission is also required  (at least
one column),
can we also mention this?

+ /* Parse analysis should already have disallowed this */
+ Assert(resultRelInfo->ri_projectReturning);
+
+ /* Process RETURNING like an UPDATE that didn't change anything */
+ *rslot = ExecProcessReturning(context, resultRelInfo, CMD_UPDATE,
+  existing, existing, context->planSlot);

The above two comments seem confusing. If you look at the code
ExecProcessReturning, I think you can set the cmdType as CMD_INSERT.

+ if (lockstrength != LCS_NONE)
+ {
+ LockTupleMode lockmode;
+
+ switch (lockstrength)
+ {
+ case LCS_FORKEYSHARE:
+ lockmode = LockTupleKeyShare;
+ break;
+ case LCS_FORSHARE:
+ lockmode = LockTupleShare;
+ break;
+ case LCS_FORNOKEYUPDATE:
+ lockmode = LockTupleNoKeyExclusive;
+ break;
+ case LCS_FORUPDATE:
+ lockmode = LockTupleExclusive;
+ break;
+ default:
+ elog(ERROR, "unexpected lock strength %d", lockstrength);
+ }
+
+ if (!ExecOnConflictLockRow(context, existing, conflictTid,
+   resultRelInfo->ri_RelationDesc, lockmode, false))
+ return false;

isolation tests do not test the case where ExecOnConflictLockRow returns false.
actually it's reachable.

-----------------------------setup---------------------
drop table if exists tsa1;
create table tsa1(a int, b int, constraint xxidx unique (a));
insert into tsa1 values (1,2);

session1, using GDB set a breakpoint at ExecOnConflictLockRow.
session1:
insert into tsa1 values(1,3) on conflict(a) do select
for update returning *;
session2:
update tsa1 set a = 111;

session1: session 1 already reached the GDB breakpoint
(ExecOnConflictLockRow), issue
``continue`` in GDB let session1 complete.
----------------------------------------------------------------------------
I'm wondering how we can add test coverage for this.

+      <row>
+       <entry><command>ON CONFLICT DO SELECT FOR UPDATE/SHARE</command></entry>
+       <entry>Check existing rows</entry>
+       <entry>&mdash;</entry>
+       <entry>Existing row</entry>
+       <entry>&mdash;</entry>
+       <entry>&mdash;</entry>
       </row>

Here, it should be "
<entry>Check existing row</entry>
"?

If you search 'ON CONFLICT', it appears on many sgml files, currently we only
made change to:
doc/src/sgml/dml.sgml
doc/src/sgml/ref/create_policy.sgml
doc/src/sgml/ref/insert.sgml

seems other sgml files also need to be updated.

--
jian
https://www.enterprisedb.com/

On 11/20/25 9:49 AM, Dean Rasheed wrote:

OK, got it. So author credit for this patch should go to Marko,
Andreas, and Viktor? In that order? And I should add the original
thread to the commit message.

Sounds good!

Andreas

On 20 Nov 2025 at 16:27 +0100, jian he <jian.universality@gmail.com>, wrote:

https://www.postgresql.org/docs/current/ddl-priv.html#DDL-PRIV-UPDATE
says:
```
SELECT ... FOR UPDATE and SELECT ... FOR SHARE also require this privilege on at
least one column, in addition to the SELECT privilege.
```
I attached extensive permission tests for ON CONFLICT DO SELECT

I’ve added in both the tests you sent over as is Jian. I was sure I wrote some tests for the partitioning, but I must’ve forgot to commit them, so thanks for that.

+ For <literal>ON CONFLICT DO SELECT</literal>, <literal>SELECT</literal>
+ privilege is required on any column whose values are read in the
+ <replaceable>condition</replaceable>.
If you do <literal>ON CONFLICT DO SELECT FOR UPDATE</literal>
then <literal>UPDATE</literal> permission is also required (at least
one column),
can we also mention this?

I’ve update the docs in all the cases you mentioned. I’ve also grepped through the docs for “ON CONFLICT” and “DO UPDATE” and fixed upp all mentions where it made sense

+ /* Parse analysis should already have disallowed this */
+ Assert(resultRelInfo->ri_projectReturning);
+
+ /* Process RETURNING like an UPDATE that didn't change anything */
+ *rslot = ExecProcessReturning(context, resultRelInfo, CMD_UPDATE,
+ existing, existing, context->planSlot);

The above two comments seem confusing. If you look at the code
ExecProcessReturning, I think you can set the cmdType as CMD_INSERT.

Yes. I’ve clarified the comments too. Kinda itching to rewrite ExecProcessReturning so that you pass in defaultTuple(OLD, NEW) as a boolean or something - as that is all CMD_TYPE does. But in the interest of getting this committed, I’m refraining from that

+ if (!ExecOnConflictLockRow(context, existing, conflictTid,
+ resultRelInfo->ri_RelationDesc, lockmode, false))
+ return false;

isolation tests do not test the case where ExecOnConflictLockRow returns false.
actually it's reachable.

-----------------------------setup---------------------
drop table if exists tsa1;
create table tsa1(a int, b int, constraint xxidx unique (a));
insert into tsa1 values (1,2);

session1, using GDB set a breakpoint at ExecOnConflictLockRow.
session1:
insert into tsa1 values(1,3) on conflict(a) do select
for update returning *;
session2:
update tsa1 set a = 111;

session1: session 1 already reached the GDB breakpoint
(ExecOnConflictLockRow), issue
``continue`` in GDB let session1 complete.
----------------------------------------------------------------------------
I'm wondering how we can add test coverage for this.

I’ve done some minor refactoring to this code, and added some comments.
Regarding testing for it - I agree it’d be nice to have, but I have no idea how one would go about that.
Considering you tested it and the behaviour is correct, I’m hoping that we don’t consider this a blocker

Thanks for the thorough review Jian!

Got a complication warning in CI: error: ‘lockmode’ may be used uninitialized. Hopefully this fixes it.