Hi, On Wed, Oct 14, 2015 at 9:38 AM, Andres Freund <and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers