Re: INSERT ... ON CONFLICT DO SELECT [FOR ...] take 2 - Mailing list pgsql-hackers

On 3/31/25 5:33 PM, Dean Rasheed wrote:
>>> 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:

Thanks for taking a look and improving my patch! And thanks to Kirill too.

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

Correct, though I have not yet made up my mind if removing the 
duplicated code makes things easier or harder to follow but that will 
likely be obvious in the next version of the patch. In this place it 
makes things nicer but in other places I am less certain.

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

Thanks for spotting and fixing!

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

Yeah, you are right. I made a thinko when designing this.

> * 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().

Yes, for sure! That would be really useful. Thanks!
  > * 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.

Correct.

> 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 have fixed that one and some other issues locally and will submit a 
new version in a while after I have added more tests because you are 
very correct in that a big issue with my last version of the patch was 
the big lack of tests and lack of making sure all features which 
interact with UPSERT actually worked with my changes. Plus some islation 
tests would be nice to have.

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

Yeah, the docs really need to be fixed.

Andreas