On 20 Nov 2025 at 16:27 +0100, jian he <jian.universality@gmail.com>, wrote:
https://www.postgresql.org/docs/current/ddl-priv.html#DDL-PRIV-UPDATE
says:
```
SELECT ... FOR UPDATE and SELECT ... FOR SHARE also require this privilege on at
least one column, in addition to the SELECT privilege.
```
I attached extensive permission tests for ON CONFLICT DO SELECT
I’ve added in both the tests you sent over as is Jian. I was sure I wrote some tests for the partitioning, but I must’ve forgot to commit them, so thanks for that.
+ For <literal>ON CONFLICT DO SELECT</literal>, <literal>SELECT</literal>
+ privilege is required on any column whose values are read in the
+ <replaceable>condition</replaceable>.
If you do <literal>ON CONFLICT DO SELECT FOR UPDATE</literal>
then <literal>UPDATE</literal> permission is also required (at least
one column),
can we also mention this?
I’ve update the docs in all the cases you mentioned. I’ve also grepped through the docs for “ON CONFLICT” and “DO UPDATE” and fixed upp all mentions where it made sense
+ /* Parse analysis should already have disallowed this */
+ Assert(resultRelInfo->ri_projectReturning);
+
+ /* Process RETURNING like an UPDATE that didn't change anything */
+ *rslot = ExecProcessReturning(context, resultRelInfo, CMD_UPDATE,
+ existing, existing, context->planSlot);
The above two comments seem confusing. If you look at the code
ExecProcessReturning, I think you can set the cmdType as CMD_INSERT.
Yes. I’ve clarified the comments too. Kinda itching to rewrite ExecProcessReturning so that you pass in defaultTuple(OLD, NEW) as a boolean or something - as that is all CMD_TYPE does. But in the interest of getting this committed, I’m refraining from that
+ if (!ExecOnConflictLockRow(context, existing, conflictTid,
+ resultRelInfo->ri_RelationDesc, lockmode, false))
+ return false;
isolation tests do not test the case where ExecOnConflictLockRow returns false.
actually it's reachable.
-----------------------------setup---------------------
drop table if exists tsa1;
create table tsa1(a int, b int, constraint xxidx unique (a));
insert into tsa1 values (1,2);
session1, using GDB set a breakpoint at ExecOnConflictLockRow.
session1:
insert into tsa1 values(1,3) on conflict(a) do select
for update returning *;
session2:
update tsa1 set a = 111;
session1: session 1 already reached the GDB breakpoint
(ExecOnConflictLockRow), issue
``continue`` in GDB let session1 complete.
----------------------------------------------------------------------------
I'm wondering how we can add test coverage for this.
I’ve done some minor refactoring to this code, and added some comments.
Regarding testing for it - I agree it’d be nice to have, but I have no idea how one would go about that.
Considering you tested it and the behaviour is correct, I’m hoping that we don’t consider this a blocker
Thanks for the thorough review Jian!
