On Tue, Dec 17, 2019 at 2:32 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Wed, Dec 11, 2019 at 11:13 AM vignesh C <vignes...@gmail.com> wrote:
> >
> >
> > It sets the final_lsn here so that it can iterate from the start_lsn
> > to final_lsn and cleanup the serialized files in
> > ReorderBufferRestoreCleanup function. One solution We were thinking
> > was to store the lsn of the last serialized change while serialiizing
> > and set the final_lsn in the above case where it crashes like the
> > below code:
>
> Sure, we can do something on the lines what you are suggesting, but
> why can't we update final_lsn at the time of serializing the changes?
> If we do that then we don't even need to compute it separately during
> ReorderBufferAbortOld.
>
> Let me try to explain the problem and proposed solutions for the same.
> Currently, after serializing the changes we remove the 'changes' from
> ReorderBufferTXN. Now, if the system crashes due to any reason after
> that, we won't be able to compute final_lsn after the restart. And
> that leads to access violation in ReorderBufferAbortOld which is
> trying to access changes list from ReorderBufferTXN to compute
> final_lsn.
>
> We could fix it by either tracking 'last_serialized_change' as
> proposed by Vignesh or we could update the final_lsn while we
> serialize the changes.
>
I felt amit solution also solves the problem. Attached patch has the
fix based on the solution proposed.
Thoughts?
Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
From 0a25c2f93f9aff267e529b3b0a7842b3e043d782 Mon Sep 17 00:00:00 2001
From: vignesh <vignesh@localhost.localdomain>
Date: Fri, 27 Dec 2019 13:17:04 +0530
Subject: [PATCH] Reorder buffer crash while aborting old transactions.
While aborting aborted transactions it crashed as there are no reorderbuffer
changes present in the list because all the reorderbuffer changes were
serialized. Fixing it by storing the last change's lsn while serializing the
reorderbuffer changes. This lsn will be used as final_lsn which will help in
cleaning of spilled files in pg_replslot.
---
src/backend/replication/logical/reorderbuffer.c | 16 +---------------
src/include/replication/reorderbuffer.h | 3 +--
2 files changed, 2 insertions(+), 17 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 53affeb..7fa9163 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1956,21 +1956,6 @@ ReorderBufferAbortOld(ReorderBuffer *rb, TransactionId oldestRunningXid)
if (TransactionIdPrecedes(txn->xid, oldestRunningXid))
{
- /*
- * We set final_lsn on a transaction when we decode its commit or
- * abort record, but we never see those records for crashed
- * transactions. To ensure cleanup of these transactions, set
- * final_lsn to that of their last change; this causes
- * ReorderBufferRestoreCleanup to do the right thing.
- */
- if (txn->serialized && txn->final_lsn == 0)
- {
- ReorderBufferChange *last =
- dlist_tail_element(ReorderBufferChange, node, &txn->changes);
-
- txn->final_lsn = last->lsn;
- }
-
elog(DEBUG2, "aborting old transaction %u", txn->xid);
/* remove potential on-disk data, and deallocate this tx */
@@ -2472,6 +2457,7 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
}
ReorderBufferSerializeChange(rb, txn, fd, change);
+ txn->final_lsn = change->lsn;
dlist_delete(&change->node);
ReorderBufferReturnChange(rb, change);
diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h
index 0867ee9..139846e 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -188,8 +188,7 @@ typedef struct ReorderBufferTXN
* * plain abort record
* * prepared transaction abort
* * error during decoding
- * * for a crashed transaction, the LSN of the last change, regardless of
- * what it was.
+ * * last serialized lsn
* ----
*/
XLogRecPtr final_lsn;
--
1.8.3.1
