Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add support for INSERT ... ON CONFLICT DO SELECT.

  2. doc: Fix statement about ON CONFLICT and deferrable constraints.

  3. Fix ON CONFLICT ON CONSTRAINT during REINDEX CONCURRENTLY

  1. ON CONFLICT DO SELECT (take 3)

    Viktor Holmberg <v@viktorh.net> — 2025-10-07T11:56:46Z

    Hi!

This patch implements ON CONFLICT DO SELECT.
This feature would be very handy in bunch of cases, for example idempotent APIs.
I’ve worked around the lack of this by using three statements, like: SELECT -> INSERT if not found -> SELECT again for concurrency safety. (And having to do that dance is driving me nuts)

Apart from the convenience, it’ll also have a performance boost in cases with high latency.

As evidence of that fact that this is needed, and workarounds are complicated, see this stack overflow question: https://stackoverflow.com/questions/16123944/write-a-postgres-get-or-create-sql-query or this entire podcast episode (!) https://www.youtube.com/watch?v=59CainMBjtQ

This patch is 85% the work of Andreas Karlsson and the reviewers (Dean Rasheed, Joel Jacobson, Kirill Reshke) in this thread: https://www.postgresql.org/message-id/flat/2b5db2e6-8ece-44d0-9890-f256fdca9f7e%40proxel.se, which unfortunately seems to have stalled.
I’ve fixed up all the issues mentioned in that thread (at least I think so), plus some minor extra stuff:

1. Made it work with partitioned tables
2. Added isolation test
3. Added tests for row-level security
4. Added tests for partitioning
5. Docs updated
6. Comment misspellings fixed
7. Renamed struct OnConflictSetState -> OnConflictActionState

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!

/Viktor

  2. Re: ON CONFLICT DO SELECT (take 3)

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2025-11-10T09:21:29Z

    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

  3. Re: ON CONFLICT DO SELECT (take 3)

    Viktor Holmberg <v@viktorh.net> — 2025-11-10T10:18:54Z

    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
On 10 Nov 2025 at 10:21 +0100, Dean Rasheed <dean.a.rasheed@gmail.com>, wrote:
> 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

  4. Re: ON CONFLICT DO SELECT (take 3)

    Viktor Holmberg <v@viktorh.net> — 2025-11-17T10:00:31Z

    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 15 Nov 2025, at 12:11, jian he <jian.universality@gmail.com> wrote:
> 
> On Sat, Nov 15, 2025 at 5:24 AM jian he <jian.universality@gmail.com> wrote:
>> 
>> On Fri, Nov 14, 2025 at 10:34 PM Viktor Holmberg <v@viktorh.net> wrote:
>>> 
>>> Here are some updates that needed to be done after the improvements to the RLS docs / tests in 7dc4fa & 2e8424.
>>> 
> 
> hi.
> 
> 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.
> 
> 
> 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.
> 
> 
> changing
> OnConflictSetState
> to
> OnConflictActionState
> could make it a separate patch.
> 
> all these 3 patches can be merged together, I think.
> ----------------------------------------
> typedef struct OnConflictExpr
> {
>    NodeTag        type;
>    OnConflictAction action;    /* DO NOTHING or UPDATE? */
> 
> "/* DO NOTHING or UPDATE? */"
> this comment needs to be changed?
> ----------------------------------------
> 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)
> 
> src/test/regress/sql/updatable_views.sql, there are many occurence of
> "on conflict".
> I think we also need tests for ON CONFLICT DO SELECT.
> 
> CREATE TABLE base_tbl (a int PRIMARY KEY, b text DEFAULT 'Unspecified');
> INSERT INTO base_tbl SELECT i, 'Row ' || i FROM generate_series(-2, 2) g(i);
> CREATE VIEW rw_view15 AS SELECT a, upper(b) FROM base_tbl;
> INSERT INTO rw_view15 (a) VALUES (3);
> truncate base_tbl;
> INSERT INTO rw_view15 (a) VALUES (3);
> INSERT INTO rw_view15 (a) VALUES (3) ON CONFLICT (a) DO SELECT WHERE
> excluded.upper = 'UNSPECIFIED' RETURNING *;
> INSERT INTO rw_view15 (a) VALUES (3) ON CONFLICT (a) DO UPDATE SET a =
> excluded.a WHERE excluded.upper = 'UNSPECIFIED' RETURNING *;
> INSERT INTO rw_view15 (a) VALUES (3) ON CONFLICT (a) DO SELECT WHERE
> excluded.upper = 'Unspecified' RETURNING *;
> INSERT INTO rw_view15 (a) VALUES (3) ON CONFLICT (a) DO UPDATE SET a =
> excluded.a WHERE excluded.upper = 'Unspecified' RETURNING *;
> 
> 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.
> 
> --
> jian
> https://www.enterprisedb.com/

> On 10 Nov 2025, at 11:18, Viktor Holmberg <v@viktorh.net> wrote:
> 
> 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
> On 10 Nov 2025 at 10:21 +0100, Dean Rasheed <dean.a.rasheed@gmail.com>, wrote:
>> 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

  5. Re: ON CONFLICT DO SELECT (take 3)

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2025-11-17T11:36:17Z

    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

  6. Re: ON CONFLICT DO SELECT (take 3)

    Viktor Holmberg <v@viktorh.net> — 2025-11-17T22:06:59Z

    >> 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!

  7. Re: ON CONFLICT DO SELECT (take 3)

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2025-11-19T14:08:14Z

    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

  8. Re: ON CONFLICT DO SELECT (take 3)

    Viktor Holmberg <v@viktorh.net> — 2025-11-19T16:51:13Z

    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

  9. Re: ON CONFLICT DO SELECT (take 3)

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2025-11-19T17:19:35Z

    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:

https://www.postgresql.org/message-id/flat/CA%2BFpmFdoa7O7yS3k7ZtqvA%2BhNWUA6YvJy6VvdYX1sGsryVQBNQ%40mail.gmail.com

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

Regards,
Dean

  10. Re: ON CONFLICT DO SELECT (take 3)

    Marko Tiikkaja <marko@joh.to> — 2025-11-19T19:06:31Z

    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: https://www.postgresql.org/message-id/flat/2b5db2e6-8ece-44d0-9890-f256fdca9f7e%40proxel.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

  11. Re: ON CONFLICT DO SELECT (take 3)

    Viktor Holmberg <v@viktorh.net> — 2025-11-19T19:54:58Z

    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:
>
> https://www.postgresql.org/message-id/flat/CA%2BFpmFdoa7O7yS3k7ZtqvA%2BhNWUA6YvJy6VvdYX1sGsryVQBNQ%40mail.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.

  12. Re: ON CONFLICT DO SELECT (take 3)

    Andreas Karlsson <andreas@proxel.se> — 2025-11-20T01:29:16Z

    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: https://www.postgresql.org/message-id/flat/2b5db2e6-8ece-44d0-9890-f256fdca9f7e%40proxel.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

  13. Re: ON CONFLICT DO SELECT (take 3)

    jian he <jian.universality@gmail.com> — 2025-11-20T06:11:27Z

    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/

  14. Re: ON CONFLICT DO SELECT (take 3)

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2025-11-20T08:49:51Z

    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: https://www.postgresql.org/message-id/flat/2b5db2e6-8ece-44d0-9890-f256fdca9f7e%40proxel.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

  15. Re: ON CONFLICT DO SELECT (take 3)

    Marko Tiikkaja <marko@joh.to> — 2025-11-20T08:50:41Z

    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

  16. Re: ON CONFLICT DO SELECT (take 3)

    Viktor Holmberg <v@viktorh.net> — 2025-11-20T10:22:25Z

    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

  17. Re: ON CONFLICT DO SELECT (take 3)

    jian he <jian.universality@gmail.com> — 2025-11-20T15:26:51Z

    >
>
> 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/

  18. Re: ON CONFLICT DO SELECT (take 3)

    Andreas Karlsson <andreas@proxel.se> — 2025-11-21T00:41:02Z

    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

  19. Re: ON CONFLICT DO SELECT (take 3)

    Viktor Holmberg <v@viktorh.net> — 2025-11-23T20:34:11Z

    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!

  20. Re: ON CONFLICT DO SELECT (take 3)

    Viktor Holmberg <v@viktorh.net> — 2025-11-24T10:39:32Z

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

  21. Re: ON CONFLICT DO SELECT (take 3)

    Viktor Holmberg <v@viktorh.net> — 2025-11-24T15:23:17Z

    On 24 Nov 2025 at 11:39 +0100, Viktor Holmberg <v@viktorh.net>, wrote:
> Got a complication warning in CI: error: ‘lockmode’ may be used uninitialized. Hopefully this fixes it.
It did not. But this will.
For some reason, in this bit:

‘''
LockTupleMode lockmode;
….
case LCS_FORUPDATE:
 lockmode = LockTupleExclusive;
 break;
 case LCS_NONE:
 elog(ERROR, "unexpected lock strength %d", lockStrength);
 }

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

GCC gives warning "error: ‘lockmode’ may be used uninitialized”. But if I switch the final exhaustive “case" to a “default” the warning goes away. Strange, if anyone know how to fix let me know. But also I don’t think it’s a big deal.

  22. Re: ON CONFLICT DO SELECT (take 3)

    jian he <jian.universality@gmail.com> — 2025-11-25T08:33:17Z

    On Mon, Nov 24, 2025 at 11:23 PM Viktor Holmberg <v@viktorh.net> wrote:
>
> It did not. But this will.
> For some reason, in this bit:
>
> ‘''
> LockTupleMode lockmode;
> ….
> case LCS_FORUPDATE:
>  lockmode = LockTupleExclusive;
>  break;
>  case LCS_NONE:
>  elog(ERROR, "unexpected lock strength %d", lockStrength);
>  }
>
>  if (!ExecOnConflictLockRow(context, existing, conflictTid,
>  resultRelInfo->ri_RelationDesc, lockmode, false))
>  return false;
> ‘''
>
> GCC gives warning "error: ‘lockmode’ may be used uninitialized”. But if I switch the final exhaustive “case" to a “default” the warning goes away. Strange, if anyone know how to fix let me know. But also I don’t think it’s a big deal.

hi.
you can search ``/* keep compiler quiet */`` within the codebase.

after
``elog(ERROR, "unexpected lock strength %d", lockStrength);``
you can add
``
lockmode = LockTupleExclusive;
break;
``

in doc/src/sgml/mvcc.sgml:
   <para>
    <command>INSERT</command> with an <literal>ON CONFLICT DO
    NOTHING</literal> clause may have insertion not proceed for a row due to
    the outcome of another transaction whose effects are not visible
    to the <command>INSERT</command> snapshot.  Again, this is only
    the case in Read Committed mode.
   </para>
I think we need to add something after the above quoted paragraph.

doc/src/sgml/ref/create_view.sgml, some places also need to be updated, I think.
see text ON CONFLICT UPDATE in there.

I added some dummy tests on src/test/regress/sql/triggers.sql.
also did some minor doc changes.

please check the attached v16.
v16-0001: rebase and combine v15-0001, v15-0002, v15-0003, v15-0004 together.
v16-0002: using INJECTION_POINT to test the case when
ExecOnConflictSelect->ExecOnConflictLockRow returns false.

v16-0002, I use
```
        if (!ExecOnConflictLockRow(context, existing, conflictTid,
                                   resultRelInfo->ri_RelationDesc,
lockmode, false))
        {
            INJECTION_POINT("exec-onconflictselect-after-lockrow", NULL);
            elog(INFO, "this part is reached");
            return false;
        }
```
to demomate that ExecOnConflictLockRow is reachable.
obviously, ``elog(INFO, "this part is reached");`` needs to be removed later.


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

  23. Re: ON CONFLICT DO SELECT (take 3)

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2025-11-25T10:04:36Z

    On Tue, 25 Nov 2025 at 08:33, jian he <jian.universality@gmail.com> wrote:
>
> v16-0002: using INJECTION_POINT to test the case when
> ExecOnConflictSelect->ExecOnConflictLockRow returns false.
>

In general, having more tests is a good thing, but I think this is
setting a higher bar for the ON CONFLICT DO SELECT than existing code,
such as ON CONFLICT DO UPDATE. ExecOnConflictUpdate() also uses
ExecOnConflictLockRow() in the same way, and doesn't have such a test,
and there are other lock-and-retry paths in the executor not tested in
this way.

IMO, using injection points for testing a wider variety of possible
race conditions in the executor should be considered as a separate
patch.

Regards,
Dean

  24. Re: ON CONFLICT DO SELECT (take 3)

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2025-11-25T11:14:08Z

    On Sun, 23 Nov 2025 at 20:34, Viktor Holmberg <v@viktorh.net> wrote:
>
> 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
>

--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -1380,7 +1380,7 @@ WITH ( MODULUS <replaceable
class="parameter">numeric_literal</replaceable>, REM
       clause.  <literal>NOT NULL</literal> and
<literal>CHECK</literal> constraints are not
       deferrable.  Note that deferrable constraints cannot be used as
       conflict arbitrators in an <command>INSERT</command> statement that
-      includes an <literal>ON CONFLICT DO UPDATE</literal> clause.
+      includes an <literal>ON CONFLICT DO UPDATE / SELECT</literal> clause.
      </para>
     </listitem>
    </varlistentry>

Actually, a deferrable constraint cannot be used with ON CONFLICT DO
NOTHING either, which makes this a pre-existing documentation bug,
that should be fixed and back-patched separately as follows:

-      includes an <literal>ON CONFLICT DO UPDATE</literal> clause.
+      includes an <literal>ON CONFLICT</literal> clause.

In addition, I think we should change the word "arbitrators" to
"arbiters". It means pretty-much the same thing, but the latter is the
term used everywhere else.

I'll take care of that separately, so the above diff won't be needed
in this patch.

Regards,
Dean

  25. Re: ON CONFLICT DO SELECT (take 3)

    Viktor Holmberg <v@viktorh.net> — 2025-11-25T13:24:17Z

    On 25 Nov 2025 at 09:33 +0100, jian he <jian.universality@gmail.com>, wrote:
> On Mon, Nov 24, 2025 at 11:23 PM Viktor Holmberg <v@viktorh.net> wrote:
> >
> > It did not. But this will.
> > For some reason, in this bit:
> >
> > ‘''
> > LockTupleMode lockmode;
> > ….
> > case LCS_FORUPDATE:
> > lockmode = LockTupleExclusive;
> > break;
> > case LCS_NONE:
> > elog(ERROR, "unexpected lock strength %d", lockStrength);
> > }
> >
> > if (!ExecOnConflictLockRow(context, existing, conflictTid,
> > resultRelInfo->ri_RelationDesc, lockmode, false))
> > return false;
> > ‘''
> >
> > GCC gives warning "error: ‘lockmode’ may be used uninitialized”. But if I switch the final exhaustive “case" to a “default” the warning goes away. Strange, if anyone know how to fix let me know. But also I don’t think it’s a big deal.
>
> hi.
> you can search ``/* keep compiler quiet */`` within the codebase.
>
> after
> ``elog(ERROR, "unexpected lock strength %d", lockStrength);``
> you can add
> ``
> lockmode = LockTupleExclusive;
> break;
> ``
Thanks! I now realise the problem wasn’t that GCC didn’t see that elog wasn’t reachable. It was the fact that there is no default. Even though all cases are covered. This is annoying but switching it back to a default as it was seems the best option. Setting lockmode is not needed if it’s kept as a default.
My goal was to enumerate all cases so you’d get a compiler warning if one of them wasn’t handled like Dean did, but looks like I’ll have to give up on that until GCC gets improved.
>
> in doc/src/sgml/mvcc.sgml:
> <para>
> <command>INSERT</command> with an <literal>ON CONFLICT DO
> NOTHING</literal> clause may have insertion not proceed for a row due to
> the outcome of another transaction whose effects are not visible
> to the <command>INSERT</command> snapshot. Again, this is only
> the case in Read Committed mode.
> </para>
> I think we need to add something after the above quoted paragraph.
I’ve added a bit about DO SELECT now.
>
> doc/src/sgml/ref/create_view.sgml, some places also need to be updated, I think.
> see text ON CONFLICT UPDATE in there.
I checked this before and still think it’s not needed. It’s in a bit about *updatable* views, and seems obvious to me how DO SELECT would work with that? Not a hill I’m willing to die on though, but a suggested change would be appreciated.

--------

Re. Infection point testing - I think this is very cool but I’d be tempted to agree with Dean on leaving it out from this patch.

--------
Re. deferrable constraints - I’ve removed that bit from the docs. Thanks for handling that Dean.

--------

In conclusion:
Attached is v17, with:
- Jians latest patches minus the injection point testing
- Doc for MVCC
- ExecOnConflictSelect with a default clause for lockStrength.

Thanks again for your help both Jian and Dean

  26. Re: ON CONFLICT DO SELECT (take 3)

    jian he <jian.universality@gmail.com> — 2025-11-28T08:42:50Z

    On Tue, Nov 25, 2025 at 9:24 PM Viktor Holmberg <v@viktorh.net> wrote:
>
> In conclusion:
> Attached is v17, with:
> - Jians latest patches minus the injection point testing
> - Doc for MVCC
> - ExecOnConflictSelect with a default clause for lockStrength.
>

hi.

+  <para>
+   Insert a new distributor if the name doesn't match, otherwise return
+   the existing row.  This example uses the <varname>excluded</varname>
+   table in the WHERE clause to filter results:
+<programlisting>
+INSERT INTO distributors (did, dname) VALUES (12, 'Micro Devices Inc')
+    ON CONFLICT (did) DO SELECT WHERE dname = EXCLUDED.dname
+    RETURNING *;
+</programlisting>
+  </para>

"ON CONFLICT (did)":
"Insert a new distributor if the name doesn't match",
i think it should be
"Insert a new distributor if the distributor id doesn't match",
suppose "did" refer to distributor id.


  /*
- * If there is a WHERE clause, initialize state where it will
- * be evaluated, mapping the attribute numbers appropriately.
- * As with onConflictSet, we need to map partition varattnos
- * to the partition's tupdesc.
+ * For both ON CONFLICT DO UPDATE and ON CONFLICT DO SELECT,
+ * there may be a WHERE clause.  If so, initialize state where
+ * it will be evaluated, mapping the attribute numbers
+ * appropriately.  As with onConflictSet, we need to map
+ * partition varattnos twice, to catch both the EXCLUDED
+ * pseudo-relation (INNER_VAR), and the main target relation
+ * (firstVarno).
  */
  if (node->onConflictWhere)
  {
  List   *clause;

+ if (part_attmap == NULL)
+ part_attmap =
+ build_attrmap_by_name(RelationGetDescr(partrel),
+  RelationGetDescr(firstResultRel),
+  false);
+
we already processed onConflictSet. the above comments need change?


heap_lock_tuple comments:
        /*
         * This is possible, but only when locking a tuple for ON CONFLICT
         * UPDATE.  We return this value here rather than throwing an error in
         * order to give that case the opportunity to throw a more specific
         * error.
         */
+begin transaction isolation level read committed;
+insert into selfconflict values (10,1), (10,2) on conflict(f1) do
select for update returning *;
+ERROR:  ON CONFLICT DO SELECT command cannot affect row a second time
+HINT:  Ensure that no rows proposed for insertion within the same
command have duplicate constrained values.
+commit;

the above tests showing TM_Invisible is possible for ON CONFLICT DO SELECT.
so the above heap_lock_tuple comments also need change.


+--
+-- INSERT ... ON CONFLICT DO SELECT and Row-level security
+--
+
+SET SESSION AUTHORIZATION regress_rls_alice;
+DROP POLICY p3_with_all ON document;
+
+CREATE POLICY p1_select_novels ON document FOR SELECT
+  USING (cid = (SELECT cid from category WHERE cname = 'novel'));
+CREATE POLICY p2_insert_own ON document FOR INSERT
+  WITH CHECK (dauthor = current_user);
+CREATE POLICY p3_update_novels ON document FOR UPDATE
+  USING (cid = (SELECT cid from category WHERE cname = 'novel'))
+  WITH CHECK (dauthor = current_user);
+
+SET SESSION AUTHORIZATION regress_rls_bob;

create_policy.sgml "Policies Applied by Command Type" distinguish ON
CONFLICT SELECT FOR UPDATE
and ON CONFLICT SELECT is that update will invoke the UPDATE USING policy.

The above tests p1_select_novels, p3_update_novels have the same using part.
SELECT FOR UPDATE will fail just like the same reason as ON CONFLICT SELECT
so I think the above tests do not fully test the SELECT FOR UPDATE scarenio.
please check the attached file, which slightly changed
p3_update_novels USING qual.


one minor issue, ruleutils.c: get_lock_clause_strength
I think it make more sense to remove the prefix whitespace, like change
``return " FOR KEY SHARE";``
to
``return "FOR KEY SHARE";``
and let caller add the whitespace itself.


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

  27. Re: ON CONFLICT DO SELECT (take 3)

    Viktor Holmberg <v@viktorh.net> — 2025-11-28T22:02:30Z

    On 28 Nov 2025 at 09:43 +0100, jian he <jian.universality@gmail.com>, wrote:
> On Tue, Nov 25, 2025 at 9:24 PM Viktor Holmberg <v@viktorh.net> wrote:
> >
> > In conclusion:
> > Attached is v17, with:
> > - Jians latest patches minus the injection point testing
> > - Doc for MVCC
> > - ExecOnConflictSelect with a default clause for lockStrength.
> >
>
> hi.
>
> + <para>
> + Insert a new distributor if the name doesn't match, otherwise return
> + the existing row. This example uses the <varname>excluded</varname>
> + table in the WHERE clause to filter results:
> +<programlisting>
> +INSERT INTO distributors (did, dname) VALUES (12, 'Micro Devices Inc')
> + ON CONFLICT (did) DO SELECT WHERE dname = EXCLUDED.dname
> + RETURNING *;
> +</programlisting>
> + </para>
>
> "ON CONFLICT (did)":
> "Insert a new distributor if the name doesn't match",
> i think it should be
> "Insert a new distributor if the distributor id doesn't match",
> suppose "did" refer to distributor id.
You are correct, fixed
>
>
> /*
> - * If there is a WHERE clause, initialize state where it will
> - * be evaluated, mapping the attribute numbers appropriately.
> - * As with onConflictSet, we need to map partition varattnos
> - * to the partition's tupdesc.
> + * For both ON CONFLICT DO UPDATE and ON CONFLICT DO SELECT,
> + * there may be a WHERE clause. If so, initialize state where
> + * it will be evaluated, mapping the attribute numbers
> + * appropriately. As with onConflictSet, we need to map
> + * partition varattnos twice, to catch both the EXCLUDED
> + * pseudo-relation (INNER_VAR), and the main target relation
> + * (firstVarno).
> */
> if (node->onConflictWhere)
> {
> List *clause;
>
> + if (part_attmap == NULL)
> + part_attmap =
> + build_attrmap_by_name(RelationGetDescr(partrel),
> + RelationGetDescr(firstResultRel),
> + false);
> +
> we already processed onConflictSet. the above comments need change?
I’m not sure I’m following here - the comment is just saying that we’re gonna do something
similar to how we did for onConflictSet code which is above. So the comment is right I think -
But regardless I’ve rewritten it to be more clear.
If this is not what you meant, let me know.
>
> heap_lock_tuple comments:
> /*
> * This is possible, but only when locking a tuple for ON CONFLICT
> * UPDATE. We return this value here rather than throwing an error in
> * order to give that case the opportunity to throw a more specific
> * error.
> */
> +begin transaction isolation level read committed;
> +insert into selfconflict values (10,1), (10,2) on conflict(f1) do
> select for update returning *;
> +ERROR: ON CONFLICT DO SELECT command cannot affect row a second time
> +HINT: Ensure that no rows proposed for insertion within the same
> command have duplicate constrained values.
> +commit;
>
> the above tests showing TM_Invisible is possible for ON CONFLICT DO SELECT.
> so the above heap_lock_tuple comments also need change.
Right, I’ve updated the comment
>
> +--
> +-- INSERT ... ON CONFLICT DO SELECT and Row-level security
> +--
> +
> +SET SESSION AUTHORIZATION regress_rls_alice;
> +DROP POLICY p3_with_all ON document;
> +
> +CREATE POLICY p1_select_novels ON document FOR SELECT
> + USING (cid = (SELECT cid from category WHERE cname = 'novel'));
> +CREATE POLICY p2_insert_own ON document FOR INSERT
> + WITH CHECK (dauthor = current_user);
> +CREATE POLICY p3_update_novels ON document FOR UPDATE
> + USING (cid = (SELECT cid from category WHERE cname = 'novel'))
> + WITH CHECK (dauthor = current_user);
> +
> +SET SESSION AUTHORIZATION regress_rls_bob;
>
> create_policy.sgml "Policies Applied by Command Type" distinguish ON
> CONFLICT SELECT FOR UPDATE
> and ON CONFLICT SELECT is that update will invoke the UPDATE USING policy.
>
> The above tests p1_select_novels, p3_update_novels have the same using part.
> SELECT FOR UPDATE will fail just like the same reason as ON CONFLICT SELECT
> so I think the above tests do not fully test the SELECT FOR UPDATE scarenio.
> please check the attached file, which slightly changed
> p3_update_novels USING qual.
You’re right, the attached v18 includes those test changes
>
> one minor issue, ruleutils.c: get_lock_clause_strength
> I think it make more sense to remove the prefix whitespace, like change
> ``return " FOR KEY SHARE";``
> to
> ``return "FOR KEY SHARE";``
> and let caller add the whitespace itself.
I 100% agree, but this spacing behaviour seems to be a pattern in ruleutils:
{
 appendStringInfoString(buf, " ORDER BY ");
...
 appendContextKeyword(context, " OFFSET ",
...
appendContextKeyword(context, " WHERE ",

So removing the leading space for get_lock_clause_strength seems it’d cause problems by not being consistent, thus necessitating a bigger change in the code. So I’d prefer to do that as a separate change so as to not further increase the diff for this.

-------

Attaching v18 with the above changes. Thanks your continued reviews Jian!

  28. Re: ON CONFLICT DO SELECT (take 3)

    jian he <jian.universality@gmail.com> — 2025-12-12T06:15:19Z

    On Sat, Nov 29, 2025 at 6:02 AM Viktor Holmberg <v@viktorh.net> 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

  29. Re: ON CONFLICT DO SELECT (take 3)

    Viktor Holmberg <v@viktorh.net> — 2025-12-16T15:14:06Z

    On 12 Dec 2025 at 07:15 +0100, jian he <jian.universality@gmail.com>, wrote:
> On Sat, Nov 29, 2025 at 6:02 AM Viktor Holmberg <v@viktorh.net> wrote:
> >
> > Attaching v18 with the above changes. Thanks your continued reviews Jian!
> >
> hi.
> I had some minor comments with doc, comments.
Thanks, all changed now.
> + 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.
Ok, I find this change slightly scary, but I’ve now changed this to assert that table_tuple_fetch_row_version is true. You say “unlikely” but having looked at it for a while I can’t see any case where it’d happen. Hence an assert seems most appropriate. I was thinking about asserting it even before the if as I believe the tuple should always be physically present, but I didn’t dare to. If anyone can think of a case where it’d happen I’d love to hear it!

/Viktor