| From: | "v(at)viktorh(dot)net" <v(at)viktorh(dot)net> |
|---|---|
| To: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, jian he <jian(dot)universality(at)gmail(dot)com> |
| Cc: | pgsql-hackers(at)postgresql(dot)org |
| Subject: | Re: ON CONFLICT DO SELECT (take 3) |
| Date: | 2025-11-17 10:00:31 |
| Message-ID: | C389FC82-8306-49DE-BCB4-AB5B742F035A@viktorh.net |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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(dot)universality(at)gmail(dot)com> wrote:
>
> On Sat, Nov 15, 2025 at 5:24 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>>
>> On Fri, Nov 14, 2025 at 10:34 PM Viktor Holmberg <v(at)viktorh(dot)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(at)viktorh(dot)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(dot)a(dot)rasheed(at)gmail(dot)com>, wrote:
>> On Tue, 7 Oct 2025 at 12:57, Viktor Holmberg <v(at)viktorh(dot)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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Anthonin Bonnefoy | 2025-11-17 10:09:24 | Fix build of llvmjit_types.bc with meson |
| Previous Message | Chao Li | 2025-11-17 09:48:43 | quoteOneName() inconsistency with quote_all_identifiers — replacement API proposed |