Re: ON CONFLICT DO SELECT (take 3)

On Thu, Jan 22, 2026 at 4:05 AM Viktor Holmberg <v(at)viktorh(dot)net> wrote:
>
> There are some white spaces in v19.
>
> Sorry, what do you mean "white spaces”? Is it a problem?

you can use ``git diff --check`` to find out these white spaces.
The attached file should be able to fix these issues.

+-- DO SELECT FOR UPDATE requires UPDATE rights, should fail for non-novel
+INSERT INTO document VALUES (33, (SELECT cid from category WHERE
cname = 'science fiction'), 1, 'regress_rls_bob', 'another sci-fi')
+ ON CONFLICT (did) DO SELECT FOR UPDATE RETURNING did, dauthor, dtitle;
+ERROR: new row violates row-level security policy for table "document"

The above comment is wrong. If i place

INSERT INTO document VALUES (33, (SELECT cid from category WHERE cname
= 'science fiction'), 1, 'regress_rls_bob', 'another sci-fi')
ON CONFLICT (did) DO NOTHING;

before the above tests, it will still fail, it will fail on SELECT
policy, p1_select_novels.
we can remove the above tests or rephrase the comment.

+CREATE POLICY p3_update_novels ON document FOR UPDATE
+ USING (cid = (SELECT cid from category WHERE cname = 'novel') AND dlevel = 1)
+ WITH CHECK (dauthor = current_user);

My understanding of the change in get_row_security_policies is that, ON CONFLICT
DO SELECT FOR UPDATE, the UPDATE WITH CHECK policy clause is never evaluated.
For testing purposes, we could change the above
+WITH CHECK (dauthor = current_user);
to
+WITH CHECK (dauthor = current_user AND false);

This leads to the next issue in ExecOnConflictSelect->ExecWithCheckOptions.
+ if (resultRelInfo->ri_WithCheckOptions != NIL)
+ {
+ /*
+ * Check target's existing tuple against SELECT-applicable USING
+ * security barrier quals (if any), enforced here as RLS checks/WCOs.
+ *
+ * The rewriter creates SELECT RLS checks/WCOs for SELECT security
+ * quals, and stores them as WCOs of "kind" WCO_RLS_CONFLICT_CHECK. If
+ * FOR UPDATE/SHARE was specified, UPDATE rights are required on the
+ * target table, and the rewriter also adds UPDATE RLS checks/WCOs for
+ * UPDATE security quals, using WCOs of the same kind, so this check
+ * enforces them too.
+ */
+ ExecWithCheckOptions(WCO_RLS_CONFLICT_CHECK, resultRelInfo,
+ existing,
+ mtstate->ps.state);
+ }
the comment I am not sure I fully understand, especially the last sentence.

My understanding is that
For ON CONFLICT DO SELECT FOR UPDATE, both the SELECT USING and UPDATE USING
clauses are evaluated. If either evaluates to false, the row is not silently
ignored; instead, an error is raised.
UPDATE CHECK clause was never evaluated.

Maybe we need to rephrase the above comments.

>
> it's time to squash the patchset into one, IMHO.
> you can also begin to write the draft commit message, explain what this is all
> about.
>
> Yes, done.
>
> ExecOnConflictSelect
> if (lockStrength == LCS_NONE)
> {
> /* Evem if the tuple is deleted, it must still be physically present */
> Assert(table_tuple_fetch_row_version(relation, conflictTid,
> SnapshotAny, existing));
> }
> this is wrong, i think.
> buildtype=release, the Assert macro will always be true,
> the whole Assert may be optimized out,
> and later code would have trouble using (TupleTableSlot *existing).
>
> Yes, you’re right. Nice catch. Fixed.
>

it still have problem with release branch,
see https://cirrus-ci.com/task/5061396800471040?logs=gcc_warning#L454

I intended to change it as
```
if (lockStrength == LCS_NONE)
{
if (!table_tuple_fetch_row_version(relation,
conflictTid,
SnapshotAny,
existing))
elog(ERROR, "failed to fetch conflicting tuple for ON CONFLICT");
}
```

* For ON CONFLICT .. UPDATE we just need the inner tlist to point to the
* excluded expression's tlist. (Similar to the SubqueryScan we don't want
* to reuse OUTER, it's used for RETURNING in some modify table cases,
* although not INSERT .. CONFLICT).
the above comments in set_deparse_plan need apply to ON CONFLICT DO SELECT

typedef enum WCOKind, WCOKind
WCO_RLS_CONFLICT_CHECK, /* RLS ON CONFLICT DO UPDATE USING policy */
the comments need to be adjusted.

The discussion link should be these three.
Discussion: https://postgr.es/m/5fca222d-62ae-4a2f-9fcb-0eca56277094@Spark
Discussion: https://postgr.es/m/2b5db2e6-8ece-44d0-9890-f256fdca9f7e@proxel.se
Discussion: https://postgr.es/m/d631b406-13b7-433e-8c0b-c6040c4b4663@Spark

Since there have been some discussions about the author, we can also mention
authors in the commit message.