On Tue, Jan 13, 2026 at 5:50 PM [email protected] <[email protected]> wrote: > > Jian, am I ok to change the commitfest entry to “waiting for committer” or > did you want to make another pass? > > /Viktor
hi.
There are some white spaces in v19.
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.
+ /*
+ * If rule_action is INSERT .. ON CONFLICT DO SELECT, the parser should
+ * have verified that it has a RETURNING clause, but we must also check
+ * that the triggering query has a RETURNING clause.
+ */
+ if (rule_action->onConflict &&
+ rule_action->onConflict->action == ONCONFLICT_SELECT &&
+ (!rule_action->returningList || !parsetree->returningList))
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("ON CONFLICT DO SELECT requires a RETURNING clause"),
+ errdetail("A rule action is INSERT ... ON CONFLICT DO SELECT, which
requires a RETURNING clause."));
+
the errmsg, errdetail conveyed information seems duplicated somehow,
maybe we can merge it into one errmsg.
infer_arbiter_indexes
/*
* Quickly return NIL for ON CONFLICT DO NOTHING without an inference
* specification or named constraint. ON CONFLICT DO UPDATE statements
* must always provide one or the other (but parser ought to have caught
* that already).
*/
if (onconflict->arbiterElems == NIL &&
onconflict->constraint == InvalidOid)
return NIL;
comments section, "ON CONFLICT DO UPDATE"
should be "ON CONFLICT DO SELECT/UPDATE" ?
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).
* In the special case of an UPDATE arising from an
* INSERT ... ON CONFLICT DO UPDATE, existing records are first checked using
* a WCO_RLS_CONFLICT_CHECK WithCheckOption, which always uses the USING
* clauses from RLS policies.
the above comments in function add_with_check_options also need to be adjusted
for ON CONFLICT DO SELECT?
doc/src/sgml/ref/insert.sgml:
If <literal>ON CONFLICT DO SELECT</literal> is used with
<literal>FOR UPDATE</literal> or <literal>FOR SHARE</literal>,
<literal>UPDATE</literal> privilege is also required on at least one
column, in addition to <literal>SELECT</literal> privilege.
I am not sure this is accurate, may we can rephrase it as the following:
If <literal>ON CONFLICT DO SELECT</literal> is present,
<literal>SELECT</literal> privilege on the table is required.
If <literal>ON CONFLICT DO SELECT</literal> is used with a specified lock
strength, the <literal>UPDATE</literal> privilege is required on at least one
column, in addition to <literal>SELECT</literal> privilege.
dml.sgml
can still be useful for those commands. For example, in an
<command>INSERT</command> with an
<link linkend="sql-on-conflict"><literal>ON CONFLICT DO UPDATE</literal></link>
clause, the old values will be non-<literal>NULL</literal> for conflicting
rows. Similarly, in an <command>INSERT</command> with an
<literal>ON CONFLICT DO SELECT</literal> clause, you can look at the old
values to determine if your query inserted a row or not.
for the above last sentence, maybe we can rephrase it as the following:
"""
Similarly, in an <command>INSERT</command> with an
<literal>ON CONFLICT DO SELECT</literal> clause,
the old values will be non-NULL for conflicting rows.
"""
create_policy.sgml
<para>
If a data-modifying query has a <literal>RETURNING</literal> clause,
<literal>SELECT</literal> permissions are required on the relation,
and any newly inserted or updated rows from the relation must satisfy
the relation's <literal>SELECT</literal> policies in order to be
available to the <literal>RETURNING</literal> clause. If a newly
inserted or updated row does not satisfy the relation's
<literal>SELECT</literal> policies, an error will be thrown (inserted
or updated rows to be returned are <emphasis>never</emphasis>
silently ignored).
</para>
we also need to update the above paragraph for ON CONFLICT DO SELECT?
create_policy.sgml
If an <literal>INSERT</literal> has an <literal>ON CONFLICT DO
NOTHING/UPDATE</literal> clause, <literal>SELECT</literal>
permissions are required on the relation, and the rows proposed for
insertion are checked using the relation's <literal>SELECT</literal>
policies.
this part also needs to change for ON CONFLICT DO SELECT?
create_policy.sgml
<varlistentry id="sql-createpolicy-update">
<term><literal>UPDATE</literal></term>
This section needs change for ON CONFLICT DO SELECT.
create_view.sgml
<command>INSERT</command> statements that have an
<literal>ON CONFLICT DO UPDATE</literal> clause are fully supported.
The above para needs change, since ON CONFLICT DO SELECT is also supported.
create_view.sgml
(<literal>ON CONFLICT DO UPDATE</literal> may
similarly affect an existing row not visible through the view).
the above sentence also needs change for ON CONFLICT DO SELECT?
ExecProcessReturning
* newSlot: slot holding new tuple inserted or updated
* planSlot: slot holding tuple returned by top subplan node
Do we need to refactor the above comments for the ON CONFLICT DO SELECT?
maybe we can change the comment
"ON CONFLICT SELECT" to "ON CONFLICT DO SELECT"
+ default:
+ elog(ERROR, "unexpected lock strength %d", lockStrength);
minor issue, we can cast it to int, like ``(int) lockStrength``.
src/backend/executor/execPartition.c changes look good to me.
updatable_views.sql: I did some ON CONFLICT DO SELECT permissions checks, and
other tests in it, please check attached.
--
jian
https://www.enterprisedb.com
v19-0001-ON-CONFLICT-DO-SELECT-tests-on-updatable_view.no-cfbot
Description: Binary data
