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