Hi, On 24/11/17 07:41, Craig Ringer wrote: > On 24 November 2017 at 13:44, Nikhil Sontakke <nikh...@2ndquadrant.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). 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. >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. 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. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
