Re: INSERT ... ON CONFLICT {UPDATE | IGNORE} 2.0

On Tue, Feb 10, 2015 at 12:04 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> FWIW, I don't think anything here really should refer to the wiki...

The Wiki pages have done a good job of summarizing things...it
certainly didn't seem that hard for you to get up to speed here. Also,
as you'll know from working on logical decoding, it's hard to know
what is complex and esoteric and what is relatively accessible when
you're this close to a big project. I recall that you said as much
before. I'm focused on signposting so that reviewers can follow what
each patch does with the minimum of effort (with reference to the Wiki
or whatever). I see no reason to not do things that way until
commit...it feels like there is less chance of the concepts going over
people's head this way.

> I don't really think there actually is that much common inbetween
> those. It looks to me that most of the code in heap_delete isn't
> actually relevant for this case and could be cut short. My guess is that
> only the WAL logging would be separated out.

I'll think about that some more.

>> > * I doubt logical decoding works with the patch as it stands.
>>
>> I thought so. Perhaps you could suggest a better use of the available
>> XLOG_HEAP_* bits. I knew I needed to consider that more carefully
>> (hence the XXX comment), but didn't get around to it.
>
> I think you probably need to add test macros that make sure only the
> individual bits are sets, and not the combination and then only use those.

This too.

>> > * /*
>> > * This may occur when an instantaneously invisible tuple is blamed
>> > * as a conflict because multiple rows are inserted with the same
>> > * constrained values.
>> > How can this happen? We don't insert multiple rows with the same
>> > command id?
>>
>> This is a cardinality violation [1]. It can definitely happen - just
>> try the examples you see on the Wiki.
>
> I don't care about the wiki from the point of code comments. This needs
> to be understandable in five years time.

That wasn't clear before - you seemed to me to be questioning if this
was even possible. There is a section in the INSERT SQL reference page
about cardinality violations, so we're certainly talking about
something that a code reader likely heard of. Also, the nitty gritty
showing various scenarios on the Wiki is the quickest way to know what
is possible (but is much too long winded for user visible
documentation or code comments).

>> > * Perhaps it has previously been discussed but I'm not convinced by the
>> > reasoning for not looking at opclasses in infer_unique_index(). This
>> > seems like it'd prohibit ever having e.g. case insensitive opclasses -
>> > something surely worthwile.
>>
>> I don't think anyone gave that idea the thumbs-up. However, I really
>> don't see the problem. Sure, we could have case insensitive opclasses
>> in the future, and you may want to make a unique index using one.
>
> Then the problem suddenly becomes that previous choices of
> indexes/statements aren't possible anymore. It seems much better to
> introduce the syntax now and not have too much of a usecase for
> it.

The only way the lack of a way of specifying which opclass to use
could bite us is if there were two *actually* defined unique indexes
on the same column, each with different "equality" operators. That
seems like kind of a funny scenario, even if that were quite possible
(even if non-default opclasses existed that had a non-identical
"equality" operators, which is basically not the case today).

I grant that is a bit odd that we're talking about unique indexes
definitions affecting semantics, but that is to a certain extent the
nature of the beast. As a compromise, I suggest having the inference
specification optionally accept a named opclass per attribute, in the
style of CREATE INDEX (I'm already reusing a bit of the raw parser
support for CREATE INDEX, you see) - that'll make inference insist on
that opclass, rather than make it a strict matter of costing available
alternatives (not that any alternative is expected with idiomatic
usage). That implies no additional parser overhead, really. If that's
considered ugly, then at least it's an ugly thing that literally no
one will ever use in the foreseeable future...and an ugly thing that
is no more necessary in CREATE INDEX than here (and yet CREATE INDEX
lives with the ugliness).

> It's really not about me understanding it right now, but about longer term.

Sure.

> Can you add a UPSERT test for logical decoding? I doubt it'll work right
> now, even in the repost.

Okay.

>> > * /*
>> > * Immediately VACUUM "super-deleted" tuples
>> > */
>> > if (TransactionIdEquals(HeapTupleHeaderGetRawXmin(tuple),
>> > InvalidTransactionId))
>> > return HEAPTUPLE_DEAD;
>> > Is that branch really needed? Shouldn't it just be happening as a
>> > consequence of the already existing code? Same in SatisfiesMVCC. If
>> > you actually needed that block, it'd need to be done in SatisfiesSelf
>> > as well, no? You have a comment about a possible loop - but that seems
>> > wrong to me, implying that HEAP_XMIN_COMMITTED was set invalidly.
>>
>> Indeed, this code is kind of odd. While I think the omission within
>> SatisfiesSelf() may be problematic too, if you really want to know why
>> this code is needed, uncomment it and run Jeff's stress-test. It will
>> reliably break.
>
> Again. I don't care about running some strange tool when reading code
> comments.

Again, I thought you were skeptical about the very need for this code
(and not how it was presented). If that was the case, that tool would
provide you with a pretty quick and easy way of satisfying yourself
that it is needed. The actual reason that it is needed is that if it
isn't, then the system can see a "broken promise" tuple as spuriously
visible. This will cause Jeff's tool to spit out a bunch of errors due
to finding all-NULL values in these tuples. VACUUM could not reclaim
the tuples unless and until they stopped appearing visible for
VACUUM's purposes (which this particular code snippet relates to).
Maybe the comment could be improved, and maybe the code could be
improved, but the code is necessary as things stand.

--
Peter Geoghegan