Excellent catch. We were looking at this code last week and wondered the purpose of this abort. Probably we should have some macro or function to decided whether to skip a transaction based on log record. That will avoid using different values in different places.
On Fri, Nov 25, 2022 at 1:35 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > During DecodeCommit() for skipping a transaction we use ReadRecPtr to > check whether to skip this transaction or not. Whereas in > ReorderBufferCanStartStreaming() we use EndRecPtr to check whether to > stream or not. Generally it will not create a problem but if the > commit record itself is adding some changes to the transaction(e.g. > snapshot) and if the "start_decoding_at" is in between ReadRecPtr and > EndRecPtr then streaming will decide to stream the transaction where > as DecodeCommit will decide to skip it. And for handling this case in > ReorderBufferForget() we call stream_abort(). > > So ideally if we are planning to skip the transaction we should never > stream it hence there is no need to stream abort such transaction in > case of skip. > > In this patch I have fixed the skip condition in the streaming case > and also added an assert inside ReorderBufferForget() to ensure that > the transaction should have never been streamed. > > -- > Regards, > Dilip Kumar > EnterpriseDB: http://www.enterprisedb.com -- Best Wishes, Ashutosh Bapat
