Re: UPSERT wiki page, and SQL MERGE syntax

Peter Geoghegan <pg@heroku.com> wrote:
> On Fri, Oct 10, 2014 at 12:09 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> I think what's realistic here is that the patch isn't going to get
>> committed the way you have it today, because nobody else likes that
>> design.  That may be harsh, but I think it's accurate.
>
> I do think that's harsh, because it's unnecessary: I am looking for
> the best design. If you want to propose alternatives, great, but there
> is a reason why I've done things that way, and that should be
> acknowledged. I too think naming the unique index is ugly as sin, and
> have said as much multiple times. We're almost on the same page here.

I hope you can adjust to the feedback, because it would be
disappointing to have this feature slip from the release, which is
what will happen if the index name remains part of the syntax.

> Would you be okay with this never working with partial unique
> indexes? That gives me something to work with.

That seems like the only sensible course, to me.

>> If you want to allow this to work with expression indexes, that's not
>> really a problem; you can let the user write INSERT .. ON CONFLICT
>> (upper(foo)) UPDATE ... and match that to the index.  It wouldn't
>> bother me if the first version of this feature couldn't target
>> expression indexes anyway, but if you want to include that, having the
>> user mention the actual expression rather than the index name
>> shouldn't make much difference.
>
> I'm not that worried about expression indexes, actually. I'm mostly
> worried about partial unique indexes, particularly when before insert
> row-level triggers are in play (making *that* play nice with
> expression indexes is harder still, but expression indexes on their
> own are probably not that much of a problem).

There is a problem if any column of the index allows nulls, since
that would allow multiple rows in the target table to match an
argument. Proving that an expression does not could be tricky.  I
suggest that, at least for a first cut, we restrict this to
matching an index on NOT NULL columns.

>>>> Also, how about making the SET clause optional, with the semantics
>>>> that we just update all of the fields for which a value is explicitly
>>>> specified:
>>>>
>>>> INSERT INTO overwrite_with_abandon (key, value)
>>>>    VALUES (42, 'meaning of life')
>>>>    ON DUPLICATE (key) UPDATE;
>
>> Your syntax allows the exact same thing; it simply require the user to
>> be more verbose in order to get that behavior.  If we think that
>> people wanting that behavior will be rare, then it's fine to require
>> them to spell it out when they want it.  If we think it will be the
>> overwhelming common application of this feature, and I do, then making
>> people spell it out when we could just as well infer it is pointless.

+1

> Did you consider my example? I think that people will like this idea,
> too - that clearly isn't the only consideration, though. As you say,
> it would be very easy to implement this. However, IMV, we shouldn't,
> because it is hazardous.

To quote the cited case:

> 1. Developer writes the query, and it works fine.
>
> 2. Some time later, the DBA adds an inserted_at column (those are
> common). The DBA is not aware of the existence of this particular
> query. The new column has a default value of now(), say.
>
> Should we UPDATE the inserted_at column to be NULL? Or (more
> plausibly) the default value filled in by the INSERT? Or leave it be?
> I think there is a case to be made for all of these behaviors, and
> that tension makes me prefer to not do this at all. It's like
> encouraging "SELECT *" queries in production, only worse.

That does point out the problem, in general, with carrying values
from a BEFORE INSERT trigger into an UPDATE.  Perhaps if the INSERT
fails the UPDATE phase should start with the values specified in
the first place, and not try to use anything returned by the BEFORE
INSERT triggers?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company