Re: INSERT ... ON CONFLICT documentation clean-up patch - Mailing list pgsql-hackers

Hi,

On Wed, Oct 14, 2015 at 9:38 AM, Andres Freund <andres@anarazel.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