Re: [HACKERS] logical decoding of two-phase transactions

On 30 November 2017 at 07:40, Petr Jelinek <petr(dot)jelinek(at)2ndquadrant(dot)com>
wrote:

> Hi,
>
> On 24/11/17 07:41, Craig Ringer wrote:
> > On 24 November 2017 at 13:44, Nikhil Sontakke <nikhils(at)2ndquadrant(dot)com
> >
> >
> > > How practical is adding a lock class?
> >
> > Am open to suggestions. This looks like it could work decently.
> >
> >
> > It looks amazingly simple from here. Which probably means there's more
> > to it that I haven't seen yet. I could use advice from someone who knows
> > the locking subsystem better.
> >
>
> Hmm, I don't like the interaction that would have with ROLLBACK, meaning
> that ROLLBACK has to wait for decoding to finish which may take longer
> than the transaction took itself (given potential network calls, it's
> practically unbounded time).
>

Yeah. We could check for waiters before we do the network I/O and release +
bail out. But once we enter the network call we're committed and it could
take a long time.

I don't find that particularly troubling for 2PC, but it's an obvious
nonstarter if we want to use the same mechanism for streaming normal xacts
out before commit.

Even for 2PC, if we have >1 downstream then once one reports an ERROR on
PREPARE TRANSACTION, there's probably no point continuing to stream the 2PC
xact out to other peers. So being able to abort the txn while it's being
decoded, causing decoding to bail out, is desirable there too.

> I also think that if we'll want to add streaming of transactions in the
> future, we'll face similar problem and the locking approach will not
> work there as the transaction may still be locked by the owning backend
> while we are decoding it.
>

Agreed. For that reason I agree that we need to look further afield than
locking-based solutions.

> From my perspective this patch changes the assumption in
> HeapTupleSatisfiesVacuum() that changes done by aborted transaction
> can't be seen by anybody else. That's clearly not true here as the
> decoding can see it.

Yes, *if* we don't use some locking-like approach to stop abort from
occurring while decoding is processing an xact.

> So perhaps better approach would be to not return
> HEAPTUPLE_DEAD if the transaction id is newer than the OldestXmin (same
> logic we use for deleted tuples of committed transactions) in the
> HeapTupleSatisfiesVacuum() even for aborted transactions. I also briefly
> checked HOT pruning and AFAICS the normal HOT pruning (the one not
> called by vacuum) also uses the xmin as authoritative even for aborted
> txes so nothing needs to be done there probably.
>
> In case we are worried that this affects cleanups of for example large
> aborted COPY transactions and we think it's worth worrying about then we
> could limit the new OldestXmin based logic only to catalog tuples as
> those are the only ones we need available in decoding.

Yeah, if it's limited to catalog tuples only then that sounds good. I was
quite concerned about how it'd impact vacuuming otherwise, but if limited
to catalogs about the only impact should be on workloads that create lots
of TEMPORARY tables then ROLLBACK - and not much on those.

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