Re: INSERT ... ON CONFLICT DO SELECT [FOR ...] take 2

> > 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