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

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 see this:
https://commitfest.postgresql.org/patch/6109/
already mentioned all the related discussion links.

Could you also include the discussion link in the commit message?
currently this
spread across several email threads, adding the link would help others
understand the context.

----------------------------
create table x(key int4, fruit text);
create unique index x_idx on x(key);
create role alice;
grant all on schema public to alice;
grant insert on x to alice;
grant select(key) on x to alice;
set role alice;
explain (costs off, verbose) insert into x as i values (1, 'Apple') on
conflict (key) do select where i.fruit = 'Apple' returning 1;
explain (costs off, verbose) insert into x as i values (1, 'Apple') on
conflict (key) do select returning 1;

The above simple tests seem to show ON CONFLICT DO SELECT
permission working as intended, (I didn't try harder this time).

It looks like we currently lack permission-related regression tests
(no ``ERROR:  permission denied for``),
we obviously need more.


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

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/

Thanks for the review Jian. Much appreciated. Apologies for the multiple email threads - just my email client mucking
upthe 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
upagain. 
>>>
>>> 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

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
(inupdate) 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 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
somethingwith 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

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
seemsto have stalled. 

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


.m

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

>
>
> 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>—</entry>
+       <entry>Existing row</entry>
+       <entry>—</entry>
+       <entry>—</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 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/

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

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

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/