Re: INSERT ... ON CONFLICT UPDATE and logical decoding

Hi,

> > Well, that implies that we'd actually know that we'd succeed when WAL
> > logging the speculative heap tuple's insertion. We literally have no
> > way of knowing if that's the case at that point, though - that's just
> > the nature of value locking scheme #2's optimistic approach.
>
> Attached patch does this. This is just a sketch, to provoke a quick
> discussion (which is hopefully all this needs). I did things this way
> because it seemed more efficient than rolling this into a whole new
> revision of the ON CONFLICT patch series.

> Ostensibly, this does the right thing: The test_decoding output plugin
> only ever reports either an INSERT or an UPDATE within a transaction
> (that contains an INSERT ... ON CONFLICT UPDATE). With jjanes_upsert
> running, I have not found any problematic cases where this fails to be
> true (although I have certainly been less than thorough so far -
> please take that into consideration).

I'm pretty sure this will entirely fail if you have a transaction that's
large enough to spill to disk. Calling ReorderBufferIterTXNNext() will
reuse the memory for the in memory changes.

It's also borked because it skips a large number of checks for the next
change. You're entirely disregarding what the routine does otherwise -
it could be a toast record, it could be a change to a system table, it
could be a new snapshot...

Also, this seems awfully fragile in th presence of toast rows and such?
Can we be entirely sure that the next WAL record logged by that session
would be the internal super deletion?

> * Do you think that what I've sketched here is roughly the right
> approach? Consolidating super deletions and insertions at transaction
> reassembly seemed like the right thing to do, but obviously I'm not
> best qualified to judge that. Clearly, it would be possible to
> optimize this so REORDER_BUFFER_CHANGE_INSERT "peek ahead" happens
> much more selectively, which I haven't bothered with yet.

It's far from pretty, but I guess it might end up being ok. I still think
the alternative of only emitting a real WAL record once the insertion
actually succeeded deserves attention - you hand waved it away, without
looking that much.

> * What are the hazards implied by my abuse of the
> ReorderBufferIterTXNNext() interface? It looks like it should be okay
> to "peek ahead" like this, because it has a slightly odd convention
> around memory management that accidentally works. But honestly, I'm
> feeling too lazy to make myself grok the code within
> ReorderBufferIterTXNNext() at the moment, and so have not put concerns
> about the code being totally broken to rest yet. Could the "current"
> ReorderBufferChange be freed before it is sent to a logical decoding
> plugin by way of a call to the rb->apply_change() callback (when the
> xact is spooled to disk, for example)?

As explained above, I can't see it working directly this way.

Greetings,

Andres Freund

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