On Wed, Jun 17, 2020 at 9:33 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Jun 16, 2020 at 7:49 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Tue, Jun 9, 2020 at 3:04 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > On Mon, Jun 8, 2020 at 11:53 AM Amit Kapila <amit.kapil...@gmail.com> > > > wrote: > > > > > > > > I think one of the usages we still need is in ReorderBufferForget > > > > because it can be called when we skip processing the txn. See the > > > > comments in DecodeCommit where we call this function. If I am > > > > correct, we need to probably collect all invalidations in > > > > ReorderBufferTxn as we are collecting tuplecids and use them here. We > > > > can do the same during processing of XLOG_XACT_INVALIDATIONS. > > > > > > > > > > One more point related to this is that after this patch series, we > > > need to consider executing all invalidation during transaction abort. > > > Because it is possible that due to memory overflow, we have processed > > > some of the messages which also contain a few XACT_INVALIDATION > > > messages, so to avoid cache pollution, we need to execute all of them > > > in abort. We also do the similar thing in Rollback/Rollback To > > > Savepoint, see AtEOXact_Inval and AtEOSubXact_Inval. > > > > I have analyzed this further and I think there is some problem with > > that. If Instead of keeping the invalidation as an individual change, > > if we try to combine them in ReorderBufferTxn's invalidation then what > > happens if the (sub)transaction is aborted. Basically, in this case, > > we will end up executing all those invalidations for those we never > > polluted the cache if we never try to stream it. So this will affect > > the normal case where we haven't streamed the transaction because > > every time we have executed the invalidation logged by transaction > > those are aborted. One way is we develop the list at the > > sub-transaction level and just before sending the transaction (on > > commit) combine all the (sub) transaction's invalidation list. But, > > I think since we already have the invalidation in the commit message > > then there is no point in adding this complexity. > > But, my main worry is about the streaming transaction, the problems are > > - Immediately on the arrival of individual invalidation, we can not > > directly add to the top-level transaction's invalidation list because > > later if the transaction aborted before we stream (or we directly > > stream on commit) then we will get an unnecessarily long list of > > invalidation which is done by aborted subtransaction. > > > > Is there any problem you see with this or you are concerned with the > efficiency? Please note, we already do something similar in > ReorderBufferForget and if your concern is efficiency then that > applies to existing cases as well. I think if we want we can improve > it later in many ways and one of them you have already suggested, at > this time, the main thing is correctness and also aborts are not > frequent enough to worry too much about their performance.
As of now, I am not seeing the problem, I was just concerned about processing more invalidation messages in the aborted cases compared to current code, even if the streaming is off/ or transaction never streamed as memory size is not crossed. But, I agree that it is only in the case of the abort, so I will work on this and later maybe we can test the performance. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
