Re: ON CONFLICT DO SELECT (take 3)

On Wed, 4 Feb 2026 at 12:52, Viktor Holmberg <v(at)viktorh(dot)net> wrote:
>
> On 4 Feb 2026 at 11:23 +0100, jian he <jian(dot)universality(at)gmail(dot)com>, wrote:
>
> I have marked this as "Ready for Committer".
>

I have been going over this, with an eye to committing it.

I'm not convinced that the pgrowlocks tests are worth it. It's quite a
lot of new code to test something quite trivial, and it doesn't seem
worth carrying that code going forward. I think there is sufficient
coverage in other tests, so I have removed these.

Looking at the first change to create_policy.sgml, it's not quite
right, but on closer inspection neither was the original text. The
issue is that in the case of an INSERT ... ON CONFLICT DO NOTHING,
SELECT policies are only applied if an arbiter clause is present.
Since that's a pre-existing bug, it should be fixed and back-patched
separately -- see 0001, attached.

Looking at the paragraph on insert.sgml describing column privileges
required, I realised that there is another pre-existing bug there --
it fails to mention privileges required by columns referred to by the
arbiter index/constraint, and this applies to all forms of ON
CONFLICT, not just DO UPDATE. Again, I think that should be fixed and
back-patched separately, which I've done in 0002, in a way intended to
need no update for ON CONFLICT DO SELECT.

I don't really like that ExecOnConflictSelect() passes CMD_INSERT to
ExecProcessReturning(), because that's inconsistent with
ExecOnConflictUpdate(). We could pass CMD_SELECT, adding a new case to
the switch statement, but I think a neater solution is to change the
cmdType parameter to a bool "isDelete" parameter, which makes the code
slightly simpler, because only the DELETE case behaves differently
from other cases. I considered pulling this out as a separate commit,
but I don't think that it's really worth it.

In rewriteRuleAction(), when the rule action is INSERT .. ON CONFLICT
DO SELECT but the triggering query lacks a RETURNING clause, I think
that the errdetail() that an earlier version of the patch had was
useful. The reason is that the triggering query will have been a plain
INSERT, with no ON CONFLICT clause, so the primary error message might
be confusing without the additional information that a rule rewrote
the query, turning it into an INSERT ... ON CONFLICT DO SELECT. So I
have put that errdetail() back.

In insert_conflict.sql, the permission tests looked out of place, and
a little long-winded, so I moved them to privileges.sql, which already
had suitable test tables, and I cut them down a bit, because it
doesn't really seem necessary to test every lock strength (we don't
for other commands). I also cut down some of the other tests, which
seemed a bit repetitive.

In rowsecurity.sql, I tweaked the first DO SELECT test to confirm that
the SELECT policy is applied to the existing row, in the event of
conflict. Also, note that DO SELECT FOR UPDATE is not exactly the same
as DO UPDATE, because it only applies UPDATE USING policy clauses, not
UPDATE CHECKs.

In triggers.sql, the new tests didn't really seem to be testing
anything new, since only INSERT triggers fire for DO SELECT, and for
the values used in the test cases, the triggers didn't change
anything. So I've replaced them with a single new test that confirms
that DO SELECT sees the values updated by the BEFORE INSERT trigger.

In updatable_views.sql, I've simplified a few of the tests which
seemed overly repetitive, and expanded some of the others.

Aside from those changes, I made a few other cosmetic changes. If
you're happy with those, I think it's pretty-much good-to-go.

(Though I need to wait for the release freeze to expire before I can
push and back-patch 0001 and 0002).

Regards,
Dean