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

On 07/17/2018 08:10 PM, Robert Haas wrote:
> On Mon, Jul 16, 2018 at 3:25 PM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> Oh, right, I forgot the patch also adds the leader into the group, for
>> some reason (I agree it's unclear why that would be necessary, as you
>> pointed out later).
>>
>> But all this is happening while holding the partition lock (in exclusive
>> mode). And the decoding backends do synchronize on the lock correctly
>> (although, man, the rechecks are confusing ...).
>>
>> But now I see ProcKill accesses decodeGroupLeader in multiple places,
>> and only the first one is protected by the lock, for some reason
>> (interestingly enough the one in lockGroupLeader block). Is that what
>> you mean?
> 
> I haven't traced out the control flow completely, but it sure looks to
> me like there are places where decodeGroupLeader is checked without
> holding any LWLock at all.  Also, it looks to me like some places
> (like where we're trying to find a PGPROC by XID) we use ProcArrayLock
> and in others -- I guess where we're checking the decodeGroupBlah
> stuff -- we are using the lock manager locks.  I don't know how safe
> that is, and there are not a lot of comments justifying it.  I also
> wonder why we're using the lock manager locks to protect the
> decodeGroup stuff rather than backendLock.
> 
>> FWIW I suspect the ProcKill part is borked due to incorrectly resolved
>> merge conflict or something, per my initial response from today.
> 
> Yeah I wasn't seeing the code the way I thought you were describing it
> in that response, but I'm dumb this week so maybe I just
> misunderstood.
> 
>>> I think that's probably not going to work out, but of course it's up
>>> to you how you want to spend your time!
>>
>> Well, yeah. I'm sure I could think of more fun things to do, but OTOH I
>> also have patches that require the capability to decode in-progress
>> transactions.
> 
> It's not a matter of fun; it's a matter of whether it can be made to
> work.  Don't get me wrong -- I want the ability to decode in-progress
> transactions.  I complained about that aspect of the design to Andres
> when I was reviewing and committing logical slots & logical decoding,
> and I complained about it probably more than I complained about any
> other aspect of it, largely because it instantaneously generates a
> large lag when a bulk load commits.  But not liking something about
> the way things are is not the same as knowing how to make them better.
> I believe there is a way to make it work because I believe there's a
> way to make anything work.  But I suspect that it's at least one order
> of magnitude more complex than this patch currently is, and likely an
> altogether different design.
> 

Sure, it may turn out not to work - but how do you know until you try?

We have a well known theater play here, where of the actors is blowing 
tobacco smoke into the sink, to try if gold can be created that way. 
Which is foolish, but his reasoning is "Someone had to try, to be sure!" 
So we're in the phase of blowing tobacco smoke, kinda ;-)

Also, you often discover solutions while investigating approaches that 
seem to be unworkable initially. Or entirely new approaches. It sure 
happened to me, many times.

There's a great book/movie "Touching the Void" [1] about a climber 
falling into a deep crevasse. Unable to climb up, he decides to crawl 
down - which is obviously foolish, but he happens to find a way out.

I suppose we're kinda doing the same thing here - crawling down a 
crevasse (while still smoking and blowing the tobacco smoke into a sink, 
which we happened to find in the crevasse or something).

Anyway, I have no clear idea what changes would be necessary to the 
original design of logical decoding to make implementing this easier 
now. The decoding in general is quite constrained by how our transam and 
WAL stuff works. I suppose Andres thought about this aspect, and I guess 
he concluded that (a) it's not needed for v1, and (b) adding it later 
will require about the same effort. So in the "better" case we'd end up 
waiting for logical decoding much longer, in the worse case we would not 
have it at all.

>> But the way I understand it, it pretty much *is* a list of waiters,
>> along with a couple of flags to allow the processes to notify the other
>> side about lock/unlock/abort. It does resemble the lock groups, but I
>> don't think it has the same goals.
> 
> So the parts that aren't relevant shouldn't be copied over.
> 

I'm not sure which parts aren't relevant, but in general I agree that 
stuff that is not necessary should not be copied over.

>>> That having been said, I still don't see how that's really going to
>>> work.  Just to take one example, suppose that the leader is trying to
>>> ERROR out, and the decoding workers are blocked waiting for a lock
>>> held by the leader.  The system has no way of detecting this deadlock
>>> and resolving it automatically, which certainly seems unacceptable.
>>> The only way that's going to work is if the leader waits for the
>>> worker by trying to acquire a lock held by the worker.  Then the
>>> deadlock detector would know to abort some transaction.  But that
>>> doesn't really work either - the deadlock was created by the
>>> foreground process trying to abort, and if the deadlock detector
>>> chooses that process as its victim, what then?  We're already trying
>>> to abort, and the abort code isn't supposed to throw further errors,
>>> or fail in any way, lest we break all kinds of other things.  Not to
>>> mention the fact that running the deadlock detector in the abort path
>>> isn't really safe to begin with, again because we can't throw errors
>>> when we're already in an abort path.
>>
>> Fair point, not sure. I'll leave this up to Nikhil.
> 
> That's fine, but please understand that I think there's a basic design
> flaw here that just can't be overcome with any amount of hacking on
> the details here.  I think we need a much higher-level consideration
> of the problem here and probably a lot of new infrastructure to
> support it.  One idea might be to initially support decoding of
> in-progress transactions only if they don't modify any catalog state.

The problem is you don't know if a transaction does DDL sometime later, 
in the part that you might not have decoded yet (or perhaps concurrently 
with the decoding). So I don't see how you could easily exclude such 
transactions from the decoding ...

> That would leave out a bunch of cases we'd probably like to support,
> such as CREATE TABLE + COPY in the same transaction, but it would
> likely dodge a lot of really hard problems, too, and we could improve
> things later.  One approach to the problem of catalog changes would be
> to prevent catalog tuples from being removed even after transaction
> abort until such time as there's no decoding in progress that might
> care about them.  That is not by itself sufficient because a
> transaction can abort after inserting a heap tuple but before
> inserting an index tuple and we can't look at the catalog when it's an
> inconsistent state like that and expect reasonable results.  But it
> helps: for example, if you are decoding a transaction that has
> inserted a WAL record with a cmin or cmax value of 4, and you know
> that none of the catalog records created by that transaction can have
> been pruned, then it should be safe to use a snapshot with CID 3 or
> smaller to decode the catalogs.  So consider a case like:
> 
> BEGIN;
> CREATE TABLE blah ...  -- command ID 0
> COPY blah FROM '/tmp/blah' ... -- command ID 1
> 
> Once we see the COPY show up in the WAL, it should be safe to decode
> the CREATE TABLE command and figure out what a snapshot with command
> ID 0 can see (again, assuming we've suppressed pruning in the catalogs
> in a sufficiently-well-considered way).  Then, as long as the COPY
> command doesn't do any DML via a trigger or a datatype input function
> (!) or whatever, we should be able to use that snapshot to decode the
> data inserted by COPY.

One obvious issue with this is that it actually does not help with 
reducing the replication lag, which is about the main goal of this whole 
effort. Because if the COPY is a big data load, waiting until after the 
COPY completes gives us pretty much nothing.

>  I'm not quite sure what happens if the COPY
> does do some DML or something like that -- we might have to stop
> decoding until the following command begins in the live transaction,
> or something like that.  Or maybe we don't have to do that.  I'm not
> totally sure how the command counter is managed for catalog snapshots.
> However it works in detail, we will get into trouble if we ever use a
> catalog snapshot that can see a change that the live transaction is
> still in the midst of making.  Even with pruning prevented, we can
> only count on the catalogs to be in a consistent state once the live
> transaction has finished the command -- otherwise, for example, it
> might have increased pg_class.relnatts but not yet added the
> pg_attribute entry at the time it aborts, or something like that.  I'm
> blathering a little bit but hopefully you get the point: I think the
> way forward is for somebody to think carefully through how and under
> what circumstances using a catalog snapshot can be made safe even if
> an abort has occurred afterwards -- not trying to postpone the abort,
> which I think is never going to be right.
> 

But isn't this (delaying the catalog cleanup etc.) pretty much the 
original approach, implemented by the original patch? Which you also 
claimed to be unworkable, IIRC? Or how is this addressing the problems 
with broken HOT chains, for example? Those issues were pretty much the 
reason why we started looking at alternative approaches, like delaying 
the abort ...

I wonder if disabling HOT on catalogs with wal_level=logical would be an 
option here. I'm not sure how important HOT on catalogs is, in practice 
(it surely does not help with the typical catalog bloat issue, which is 
temporary tables, because that's mostly insert+delete). I suppose we 
could disable it only when there's a replication slot indicating support 
for decoding of in-progress transactions, so that you still get HOT with 
plain logical decoding.

I'm sure there will be other obstacles, not just the HOT chain stuff, 
but it would mean one step closer to a solution.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services