Thread: Re: INSERT ... ON CONFLICT DO SELECT [FOR ...] take 2
On Tue, Dec 3, 2024, at 09:52, Andreas Karlsson wrote:
> Hi,
>
> Here is an updated version of the patch which fixes a few small bugs,
> including making sure it checks the update permission plus a bug found
> by Joel Jacobsson when it was called by SPI.
+1 for this feature.
This seems especially useful when designing idempotent APIs.
Neat to only need a single statement, for what we
currently need two separate statements for.
Here is an attempt of a realistic example:
CREATE OR REPLACE FUNCTION get_or_create_license_key(_user_id bigint, _product_id bigint)
RETURNS UUID BEGIN ATOMIC
INSERT INTO licenses (user_id, product_id)
VALUES (_user_id, _product_id)
ON CONFLICT (user_id, product_id) DO NOTHING;
SELECT license_key
FROM licenses
WHERE user_id = _user_id
AND product_id = _product_id;
END;
This can be simplified into:
CREATE OR REPLACE FUNCTION get_or_create_license_key(_user_id bigint, _product_id bigint)
RETURNS UUID BEGIN ATOMIC
INSERT INTO licenses (user_id, product_id)
VALUES (_user_id, _product_id)
ON CONFLICT (user_id, product_id) DO SELECT RETURNING license_key;
END;
I've tested the patch successfully and also looked at the code briefly
and at first glance think it looks nice and clean.
/Joel
Hi, Rebased the patch to add support for OLD.* and NEW.*. Andreas
Attachment
On 3/4/25 10:24 AM, Andreas Karlsson wrote: > Rebased the patch to add support for OLD.* and NEW.*. Apparently the CI did not like that version. Andreas
Attachment
On Wed, Mar 5, 2025, at 03:32, Andreas Karlsson wrote: > On 3/4/25 10:24 AM, Andreas Karlsson wrote: >> Rebased the patch to add support for OLD.* and NEW.*. > > Apparently the CI did not like that version. > > Andreas > > Attachments: > * v6-0001-Add-support-for-ON-CONFLICT-DO-SELECT-FOR.patch +1 This patch adds a very useful feature. I looked over the patch and noted that it touches some areas and concepts that I don't feel sufficiently familiar with. For that reason, I'm removing myself as a reviewer, hoping that someone with the appropriate expertise will step in. That said, I read through the entire patch, and everything—code, comments, tests, and documentation—appears tidy and well-structured. I didn't spot any obvious errors or issues. I did notice a couple of minor nits in the comments: - The word "strength" is misspelled as "strengh" in a few places. - There's an extra "if" in the comment "Returns true if if we're done." Overall, the patch looks solid to me. /Joel
On Wed, 5 Mar 2025 at 07:33, Andreas Karlsson <andreas@proxel.se> wrote:
>
> On 3/4/25 10:24 AM, Andreas Karlsson wrote:
> > Rebased the patch to add support for OLD.* and NEW.*.
>
> Apparently the CI did not like that version.
>
> Andreas
Hi!
Applied v6.
1) Should we answer INSERT 0 10 here?
```
reshke=# table tt ;
i
---
1
2
3
4
5
(5 rows)
reshke=# insert into tt values(5) on conflict (i) do select returning *;
i
---
5
(1 row)
INSERT 0 1
```
2) PostgreSQL fails with this query:
> reshke=*# insert into tt values(5) on conflict (i) do select for update returning *;
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
!?>
```
Program received signal SIGSEGV, Segmentation fault.
0x000055960508713e in assign_collations_walker ()
(gdb) bt
#0 0x000055960508713e in assign_collations_walker ()
#1 0x00005596052653c2 in expression_tree_walker_impl ()
#2 0x0000559605087619 in assign_collations_walker ()
#3 0x0000559605086f8b in assign_expr_collations ()
#4 0x0000559605086e99 in assign_query_collations_walker ()
#5 0x0000559605265a46 in query_tree_walker_impl ()
#6 0x0000559605086e2d in assign_query_collations ()
#7 0x0000559605042c22 in transformInsertStmt ()
#8 0x000055960504192f in transformStmt ()
#9 0x000055960504186b in transformOptionalSelectInto ()
#10 0x0000559605041798 in transformTopLevelStmt ()
#11 0x000055960504131e in parse_analyze_fixedparams ()
#12 0x000055960545b908 in pg_analyze_and_rewrite_fixedparams ()
#13 0x000055960545c124 in exec_simple_query ()
#14 0x0000559605461314 in PostgresMain ()
#15 0x00005596054585cb in BackendMain ()
#16 0x000055960537dee9 in postmaster_child_launch ()
#17 0x0000559605384286 in BackendStartup ()
#18 0x0000559605381c31 in ServerLoop ()
#19 0x0000559605381531 in PostmasterMain ()
#20 0x0000559605236d2c in main ()
(gdb) Quit
(gdb) quit
```
I tried to recompile with --enable-debug and issue stop reproducing...
--
Best regards,
Kirill Reshke
On Mon, 10 Mar 2025 at 18:05, Kirill Reshke <reshkekirill@gmail.com> wrote: > > On Wed, 5 Mar 2025 at 07:33, Andreas Karlsson <andreas@proxel.se> wrote: > > > > On 3/4/25 10:24 AM, Andreas Karlsson wrote: > > > Rebased the patch to add support for OLD.* and NEW.*. > > > > Apparently the CI did not like that version. > > > > Andreas > > Hi! > Applied v6. > > 1) Should we answer INSERT 0 10 here? Sorry, i mean: INSERT 0 0 -- Best regards, Kirill Reshke
> On 3/4/25 10:24 AM, Andreas Karlsson wrote: > Rebased the patch to add support for OLD.* and NEW.*. create table t (key int primary key, val text); insert into t values (1, 'old'); insert into t values (1, 'new') on conflict (key) do select for update returning old.*, new.*; key | val | key | val -----+-----+-----+----- 1 | old | | (1 row) IMO this should cause new.* be the same as old.*. This is kind-of like a "do update" that changes nothing, with the end result being that old and new are the same, because nothing was changed. Currently, the only command that can cause new to be NULL is a DELETE, because the new state of the table is that the row no longer exists, which isn't the case here. On Mon, 10 Mar 2025 at 13:11, Kirill Reshke <reshkekirill@gmail.com> wrote: > > On Mon, 10 Mar 2025 at 18:05, Kirill Reshke <reshkekirill@gmail.com> wrote: > > > > 1) Should we answer INSERT 0 10 here? > > Sorry, i mean: INSERT 0 0 Hmm, I would say that the patch is correct -- the count should be the number of rows inserted, updated or selected for return (and the "Outputs" section of the doc page for INSERT should be updated to say that). That way, the count always matches the number of rows returned when there's a RETURNING clause, which I think is true of all other DML commands. Regards, Dean
> > On 3/4/25 10:24 AM, Andreas Karlsson wrote: > > Rebased the patch to add support for OLD.* and NEW.*. > I took a closer look at this, and I have a number of comments: * The changes for RLS look correct. However, in get_row_security_policies(), it's not necessary to add WCO_RLS_UPDATE_CHECK checks from UPDATE policies when doing ON CONFLICT DO SELECT because it's not going to do an UPDATE. Also, some code duplication can be avoided, because basically the plain DO SELECT case is the same as the other cases, but with some checks not required. So it's much simpler to leave the code structure as it was, and just disable the checks that aren't needed based on the permissions required by the command being executed, which IMO makes the code easier to follow. * In ExecOnConflictSelect(), the comment block for RLS checking seems to have been copied verbatim from ExecOnConflictUpdate(), but it needs updating, because the two cases are different. * As I mentioned before, I think that when returning OLD/NEW values, ON CONFLICT DO SELECT should behave the same as ON CONFLICT DO UPDATE would do if it didn't change anything. * Looking at the WHERE clause, I think it's a mistake to not allow excluded values in it. That makes the WHERE clause of ON CONFLICT DO SELECT inconsistent with the WHERE clause of ON CONFLICT DO UPDATE. And that inconsistency might make it tricky to add support for excluded later, since it requires the use of qualified column names. It seems to me that it might be very useful to be able to refer to excluded values -- e.g., to return just the rows that where different from what it tried to insert -- and supporting it only requires minor tweaks to transformOnConflictClause(), set_plan_refs(), and ExecOnConflictSelect(). * ruleutils.c needs support for deparsing ON CONFLICT DO SELECT. * When inserting into a table with a rule, if the rule action is INSERT ... ON CONFLICT DO SELECT ... RETURNING, then the triggering query must also have a RETURNING clause. The parser check for a RETURNING clause doesn't catch that, so there needs to also be a check in the rewriter. Attached is an update, with fixes for those issues, plus a bit of miscellaneous tidying up (as a separate patch for ease of review). There's at least one more code issue that I didn't have time to look at: create table foo (a int primary key, b text) partition by list (a); create table foo_p1 partition of foo for values in (1); create table foo_p2 partition of foo for values in (2); insert into foo values (1, 'xxx') on conflict (a) do select for update returning *; insert into foo values (1, 'xxx') on conflict (a) do select for update returning *; server closed the connection unexpectedly It looks to me like ExecInitPartitionInfo() needs updating to initialise the WHERE clause for ON CONFLICT DO SELECT. I think there is still a fair bit more to do to get this into a committable state. The docs in particular need work. For example, on the INSERT page: * The INSERT synopsis fails to mention that DO SELECT supports WHERE. * The paragraph about privileges needs updating for DO SELECT. * The final paragraph under "output_expression" should mention DO SELECT. * The "ON CONFLICT clause" section doesn't mention DO SELECT at all. * The "Outputs" section should mention select. * The "Examples" section should have at least one example of DO SELECT. The penultimate paragraph of section 6.4, "Returning Data from Modified Rows" also needs updating. There may be more places. I'd suggest a bit of grepping in the docs (and probably also in the code) for other places that need updating. It also feels like this needs more regression tests, plus some new isolation test cases. Regards, Dean