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 <dilipbalaut@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