Re: ON CONFLICT DO SELECT (take 3)

On 28 Nov 2025 at 09:43 +0100, jian he <jian(dot)universality(at)gmail(dot)com>, wrote:
> On Tue, Nov 25, 2025 at 9:24 PM Viktor Holmberg <v(at)viktorh(dot)net> wrote:
> >
> > In conclusion:
> > Attached is v17, with:
> > - Jians latest patches minus the injection point testing
> > - Doc for MVCC
> > - ExecOnConflictSelect with a default clause for lockStrength.
> >
>
> hi.
>
> + <para>
> + Insert a new distributor if the name doesn't match, otherwise return
> + the existing row. This example uses the <varname>excluded</varname>
> + table in the WHERE clause to filter results:
> +<programlisting>
> +INSERT INTO distributors (did, dname) VALUES (12, 'Micro Devices Inc')
> + ON CONFLICT (did) DO SELECT WHERE dname = EXCLUDED.dname
> + RETURNING *;
> +</programlisting>
> + </para>
>
> "ON CONFLICT (did)":
> "Insert a new distributor if the name doesn't match",
> i think it should be
> "Insert a new distributor if the distributor id doesn't match",
> suppose "did" refer to distributor id.
You are correct, fixed
>
>
> /*
> - * If there is a WHERE clause, initialize state where it will
> - * be evaluated, mapping the attribute numbers appropriately.
> - * As with onConflictSet, we need to map partition varattnos
> - * to the partition's tupdesc.
> + * For both ON CONFLICT DO UPDATE and ON CONFLICT DO SELECT,
> + * there may be a WHERE clause. If so, initialize state where
> + * it will be evaluated, mapping the attribute numbers
> + * appropriately. As with onConflictSet, we need to map
> + * partition varattnos twice, to catch both the EXCLUDED
> + * pseudo-relation (INNER_VAR), and the main target relation
> + * (firstVarno).
> */
> if (node->onConflictWhere)
> {
> List *clause;
>
> + if (part_attmap == NULL)
> + part_attmap =
> + build_attrmap_by_name(RelationGetDescr(partrel),
> + RelationGetDescr(firstResultRel),
> + false);
> +
> we already processed onConflictSet. the above comments need change?
I’m not sure I’m following here - the comment is just saying that we’re gonna do something
similar to how we did for onConflictSet code which is above. So the comment is right I think -
But regardless I’ve rewritten it to be more clear.
If this is not what you meant, let me know.
>
> heap_lock_tuple comments:
> /*
> * This is possible, but only when locking a tuple for ON CONFLICT
> * UPDATE. We return this value here rather than throwing an error in
> * order to give that case the opportunity to throw a more specific
> * error.
> */
> +begin transaction isolation level read committed;
> +insert into selfconflict values (10,1), (10,2) on conflict(f1) do
> select for update returning *;
> +ERROR: ON CONFLICT DO SELECT command cannot affect row a second time
> +HINT: Ensure that no rows proposed for insertion within the same
> command have duplicate constrained values.
> +commit;
>
> the above tests showing TM_Invisible is possible for ON CONFLICT DO SELECT.
> so the above heap_lock_tuple comments also need change.
Right, I’ve updated the comment
>
> +--
> +-- INSERT ... ON CONFLICT DO SELECT and Row-level security
> +--
> +
> +SET SESSION AUTHORIZATION regress_rls_alice;
> +DROP POLICY p3_with_all ON document;
> +
> +CREATE POLICY p1_select_novels ON document FOR SELECT
> + USING (cid = (SELECT cid from category WHERE cname = 'novel'));
> +CREATE POLICY p2_insert_own ON document FOR INSERT
> + WITH CHECK (dauthor = current_user);
> +CREATE POLICY p3_update_novels ON document FOR UPDATE
> + USING (cid = (SELECT cid from category WHERE cname = 'novel'))
> + WITH CHECK (dauthor = current_user);
> +
> +SET SESSION AUTHORIZATION regress_rls_bob;
>
> create_policy.sgml "Policies Applied by Command Type" distinguish ON
> CONFLICT SELECT FOR UPDATE
> and ON CONFLICT SELECT is that update will invoke the UPDATE USING policy.
>
> The above tests p1_select_novels, p3_update_novels have the same using part.
> SELECT FOR UPDATE will fail just like the same reason as ON CONFLICT SELECT
> so I think the above tests do not fully test the SELECT FOR UPDATE scarenio.
> please check the attached file, which slightly changed
> p3_update_novels USING qual.
You’re right, the attached v18 includes those test changes
>
> one minor issue, ruleutils.c: get_lock_clause_strength
> I think it make more sense to remove the prefix whitespace, like change
> ``return " FOR KEY SHARE";``
> to
> ``return "FOR KEY SHARE";``
> and let caller add the whitespace itself.
I 100% agree, but this spacing behaviour seems to be a pattern in ruleutils:
{
 appendStringInfoString(buf, " ORDER BY ");
...
 appendContextKeyword(context, " OFFSET ",
...
appendContextKeyword(context, " WHERE ",

So removing the leading space for get_lock_clause_strength seems it’d cause problems by not being consistent, thus necessitating a bigger change in the code. So I’d prefer to do that as a separate change so as to not further increase the diff for this.

-------

Attaching v18 with the above changes. Thanks your continued reviews Jian!