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
