On Thu, Jan 16, 2020 at 9:17 AM Dilip Kumar <dilipbal...@gmail.com> wrote:
>
> One minor comment. Otherwise, the patch looks fine to me.
> + /*
> + * 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. Final_lsn would have been set with commit_lsn earlier when we
> + * decode it commit, no need to update in that case
> + */
> + if (txn->final_lsn < change->lsn)
> + txn->final_lsn = change->lsn;
>
> /decode it commit,/decode its commit,
>
Thanks Dilip for reviewing.
I have fixed the comments you have suggested.
Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
From 6c5de8db4b6ac4da9d4ada0e9fa56133af0ed008 Mon Sep 17 00:00:00 2001
From: vignesh <vignesh@localhost.localdomain>
Date: Fri, 17 Jan 2020 07:34:31 +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 | 26 +++++++++++--------------
src/include/replication/reorderbuffer.h | 4 ++--
2 files changed, 13 insertions(+), 17 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index bbd908a..fa8a6b0 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1974,21 +1974,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 (rbtxn_is_serialized(txn) && 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 */
@@ -2697,6 +2682,17 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
}
pgstat_report_wait_end();
+ /*
+ * 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. Final_lsn would have been set with commit_lsn earlier when we
+ * decode its commit, no need to update in that case
+ */
+ if (txn->final_lsn < change->lsn)
+ txn->final_lsn = change->lsn;
+
Assert(ondisk->change.action == change->action);
}
diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h
index 4f6c65d..5b67457 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -208,8 +208,8 @@ 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.
+ * * for a transaction with serialized changes, the LSN of the latest
+ * serialized one, unless one of the above cases.
* ----
*/
XLogRecPtr final_lsn;
--
1.8.3.1
From e9a9983ac250f2e795bfdfe78cf50e9c6c7dc5a0 Mon Sep 17 00:00:00 2001
From: vignesh <vignesh@localhost.localdomain>
Date: Fri, 17 Jan 2020 07:00:14 +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 | 26 +++++++++++--------------
src/include/replication/reorderbuffer.h | 4 ++--
2 files changed, 13 insertions(+), 17 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 6dcd973..fb2bf70 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1877,21 +1877,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 */
@@ -2467,6 +2452,17 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
}
pgstat_report_wait_end();
+ /*
+ * 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. Final_lsn would have been set with commit_lsn earlier when we
+ * decode its commit, no need to update in that case
+ */
+ if (txn->final_lsn < change->lsn)
+ txn->final_lsn = change->lsn;
+
Assert(ondisk->change.action == change->action);
}
diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h
index e5cbe25..cf2d4e7 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -167,8 +167,8 @@ 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.
+ * * for a transaction with serialized changes, the LSN of the latest
+ * serialized one, unless one of the above cases.
* ----
*/
XLogRecPtr final_lsn;
--
1.8.3.1
