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
From 20cc1084c4943bdaf23753f2a7d9add22097ed95 Mon Sep 17 00:00:00 2001 From: Dilip Kumar <dilip.ku...@enterprisedb.com> Date: Fri, 25 Nov 2022 13:11:44 +0530 Subject: [PATCH v1] Fix thinko in when to stream a transaction Actually, 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 itslef 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() in order to abort any streamed changes. 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 streaming case and also added an assert inside ReorderBufferForget() to ensure that the transaction should have never been streamed. --- src/backend/replication/logical/reorderbuffer.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 31f7381..ddd5db0 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -2942,9 +2942,8 @@ ReorderBufferForget(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn) if (txn == NULL) return; - /* For streamed transactions notify the remote node about the abort. */ - if (rbtxn_is_streamed(txn)) - rb->stream_abort(rb, txn, lsn); + /* the transaction which is being skipped shouldn't have been streamed */ + Assert(!rbtxn_is_streamed(txn)); /* cosmetic... */ txn->final_lsn = lsn; @@ -3919,7 +3918,7 @@ ReorderBufferCanStartStreaming(ReorderBuffer *rb) * restarting. */ if (ReorderBufferCanStream(rb) && - !SnapBuildXactNeedsSkip(builder, ctx->reader->EndRecPtr)) + !SnapBuildXactNeedsSkip(builder, ctx->reader->ReadRecPtr)) return true; return false; -- 1.8.3.1
