Re: ON CONFLICT DO SELECT (take 3)

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!