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

On 2015-02-04 16:49:46 -0800, Peter Geoghegan wrote:
> On Tue, Feb 2, 2015 at 01:06 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > Generally the split into the individual commits doesn't seem to make
> > much sense to me.

I think trying to make that possible is a good idea in patches of this
size. It e.g. seems entirely possible to structure the patchset so that
the speculative lock infrastructure is added first and the rest
later. I've not thought more about how to split it up further, but I'm
pretty sure it's possible.

> > The commits individually (except the first) aren't
> > indivdiually commitable and aren't even meaningful. Splitting off the
> > internal docs, tests and such actually just seems to make reviewing
> > harder because you miss context. Splitting it so that individual piece
> > are committable and reviewable makes sense, but... I have no problem
> > doing the user docs later. If you split of RLS support, you need to
> > throw an error before it's implemented.
>
> I mostly agree. Basically, I did not intend for all of the patches to
> be individually committed. The mechanism by which EXCLUDED.*
> expressions are added is somewhat novel, and deserves to be
> independently *considered*. I'm trying to show how the parts fit
> together more so than breaking things down in to smaller commits (as
> you picked up on, 0001 is the exception - that is genuinely intended
> to be committed early). Also, those commit messages give me the
> opportunity to put those parts in their appropriate context vis-a-vis
> our discussions. They refer to the Wiki, for example, or reasons why
> pg_stat_statements shouldn't care about ExcludedExpr. Obviously the
> final commit messages won't look that way.

FWIW, I don't think anything here really should refer to the wiki...

> > 0002:
> > * Tentatively I'd say that killspeculative should be done via a separate
> > function instead of heap_delete()
>
> Really? I guess if that were to happen, it would entail refactoring
> heap_delete() to call a static function, which was also called by a
> new kill_speculative() function that does this. Otherwise, you'd have
> far too much duplication.

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 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.

> > * If a arbiter index is passed to ExecCheckIndexConstraints(), can't we
> > abort the loop after checking it? Also, do we really have to iterate
> > over indexes for that case? How about moving the loop contents to a
> > separate function and using that separately for the arbiter cases?
>
> Well, the failure to do that implies very few extra cycles, but sure.

It's not that much about the CPU cycles, but also about the mental
ones. If you have to think what happens if there's more than one
match...

> > * ItemPointerIsValid
>
> What about it?

Uh. Oh. No idea. I wrote this pretty late at night ;)

> > * /*
> > * 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.

> > * 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 whole speculative insert logic isn't really well documented. Why,
> > for example, do we actually need the token? And why are there no
> > issues with overflow? And where is it documented that a 0 means
> > there's no token? ...
>
> Fair enough. Presumably it's okay that overflow theoretically could
> occur, because a race is all but impossible. The token represents a
> particular attempt by some backend at inserting a tuple, that needs to
> be waited on specifically only if it is their active attempt (and the
> xact is still running). Otherwise, you get unprincipled deadlocks.
> Even if by some incredibly set of circumstances it wraps around, worst
> case scenario you get an unprinciped deadlock, which is hardly the end
> of the world given the immense number of insertions required, and the
> immense unlikelihood that things would work out that way - it'd be
> basically impossible.
>
> I'll document the "0" thing.

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

> > * /* XXX: Make sure that re-use of bits is safe here */ - no, not
> > unless you change existing checks.
>
> I think that this is a restatement of your remarks on logical
> decoding. No?

Yea. By here it was even later :P.

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

> > * /*
> > * 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.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services