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 wrote:
>
> On Sat, Nov 15, 2025 at 5:24 AM jian he wrote:
>>
>> On Fri, Nov 14, 2025 at 10:34 PM Viktor Holmberg 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
> {
>NodeTagtype;
>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 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 , wrote:
>> On Tue, 7 Oct 2025 at 12:57, Viktor Holmberg 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
