Re: INSERT ... ON CONFLICT documentation clean-up patch

Hi,

On Wed, Oct 14, 2015 at 9:38 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> "do not work sensibly" imo doesn't sound very good in docs. Maybe
> something roughly along the lines of "are unlikely to work as expected
> as the on conflict action is only taken in case of unique violation on
> the target relation, not child relations". I'd remove the whole bit
> about triggers not having access to ON CONFLICT.

"work sensibly" already appears in the documentation a number of
times. I won't argue with you on this, though.

>> diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
>> index 8caf5fe..0794acb3 100644
>> --- a/doc/src/sgml/ref/insert.sgml
>> +++ b/doc/src/sgml/ref/insert.sgml
>> @@ -99,7 +99,8 @@ INSERT INTO <replaceable class="PARAMETER">table_name</replaceable> [ AS <replac
>> <para>
>> You must have <literal>INSERT</literal> privilege on a table in
>> order to insert into it. If <literal>ON CONFLICT DO UPDATE</> is
>> - present the <literal>UPDATE</literal> privilege is also required.
>> + present, <literal>UPDATE</literal> privilege on the table is also
>> + required.
>> </para>
>
> Is this actually an improvement?

I wanted to clarify that we're still talking about table-level privilege.

>> @@ -161,10 +162,7 @@ INSERT INTO <replaceable class="PARAMETER">table_name</replaceable> [ AS <replac
>> <listitem>
>> <para>
>> A substitute name for the target table. When an alias is provided, it
>> - completely hides the actual name of the table. This is particularly
>> - useful when using <literal>ON CONFLICT DO UPDATE</literal> into a table
>> - named <literal>excluded</literal> as that's also the name of the
>> - pseudo-relation containing the proposed row.
>> + completely hides the actual name of the table.
>> </para>
>> </listitem>
>> </varlistentry>
>
> Hm?

I didn't think it was useful to acknowledge the case where a table is
named "excluded". It's a case that matters, but there is no point in
acknowledging it in the documentation. Users will just look for a way
to alias it on the rare occasion when this happens.

> I'm not convinced it's an improvement to talk about arbiter indexes
> here.

Okay.

>> + For
>> + <literal>ON CONFLICT DO NOTHING</literal>, it is optional to
>> + specify a <parameter>conflict_target</parameter>; when omitted,
>> + conflicts with all usable constraints (and unique indexes) are
>> + handled. For <literal>ON CONFLICT DO UPDATE</literal>, a
>> + <parameter>conflict_target</parameter> <emphasis>must</emphasis> be
>> + provided.
>
> Yes, that's a bit clearer.

Cool.

>> Every time an insertion without <literal>ON CONFLICT</literal>
>> would ordinarily raise an error due to violating one of the
>> - inferred (or explicitly named) constraints, a conflict (as in
>> - <literal>ON CONFLICT</literal>) occurs, and the alternative action,
>> - as specified by <parameter>conflict_action</parameter> is taken.
>> - This happens on a row-by-row basis.
>> + inferred constraints/indexes (or an explicitly named constraint), a
>> + conflict (as in <literal>ON CONFLICT</literal>) occurs, and the
>> + alternative action, as specified by
>> + <parameter>conflict_action</parameter> is taken. This happens on a
>> + row-by-row basis. Only <literal>NOT DEFERRABLE</literal>
>> + constraints are supported as arbiters.
>> </para>
>
> I'm inclined ot only add the "Only <literal>NOT DEFERRABLE</literal> constraints are
> supported as arbiters." bit - the rest doesn't really seem to be an
> improvement?

I'm just trying to make clear that at this level, it doesn't matter
how the arbiter was selected. Glad you agree that this is a better
place to talk about deferrable constraints, though.

>> <para>
>> @@ -425,17 +427,28 @@ INSERT INTO <replaceable class="PARAMETER">table_name</replaceable> [ AS <replac
>> specified columns/expressions and, if specified, whose predicate
>> implies the <replaceable class="PARAMETER">
>> index_predicate</replaceable> are chosen as arbiter indexes. Note
>> - that this means an index without a predicate will be used if a
>> - non-partial index matching every other criteria happens to be
>> - available.
>> + that this means a unique index without a predicate will be inferred
>> + (and used by <literal>ON CONFLICT</literal> as an arbiter) if such
>> + an index satisfying every other criteria is available.
>> </para>
>
> Hm. I agree that the "happens to be available" sounds a bit too
> casual. But I think the sentence was easier to understand for endusers
> before?

Not sure that it was. "Happens" creates the impression that it could
happen almost by mistake.

>> + <tip>
>> + <para>
>> + A unique index inference clause should be preferred over naming a
>> + constraint directly using <literal>ON CONFLICT ON
>> + CONSTRAINT</literal> <replaceable class="PARAMETER">
>> + constraint_name</replaceable>. Unique index inference is
>> + adaptable to nonsignificant changes in the available constraints.
>> + Furthermore, it is possible for more than one constraint and/or
>> + unique index to be inferred for the same statement.
>> + </para>
>> + </tip>
>
> I'd formulate this a more neutral. How something like "... inference
> ...

I don't follow.

> Why did you move the exclusion constraint bit? Isn't it more appropriate
> in the action section?

I wanted to de-emphasize it. Whatever section it's in, it isn't
particularly important. It's not obvious what DO UPDATE + exclusion
constraints even means.

>> Note that the effects of all per-row <literal>BEFORE INSERT</literal>
>> - triggers are reflected in <varname>excluded</varname> values, since those
>> + triggers are reflected in <varname>EXCLUDED</varname> values, since those
>> effects may have contributed to the row being excluded from insertion.
>> </para>
>
> It's not correct to use uppercase EXCLUDED here imo - the pseudo table
> is named "excluded" which is important in case the table name gets
> quoted.

I wanted to be consistent, but I guess your reason for not going that
way is at least as good as mine.

--
Peter Geoghegan