On 2017/11/24 10:57, Craig Ringer wrote:
> On 24 November 2017 at 09:20, atorikoshi
<torikoshi_atsushi...@lab.ntt.co.jp
>> wrote:
>
>>
>> Thanks for letting me know.
>> I think I only tested running "make check" at top directory, sorry
>> for my insufficient test.
>>
>> The test failure happened at the beginning of replication(creating
>> slot), so there are no changes yet and getting the tail of changes
>> failed.
>>
>> Because the bug we are fixing only happens after creating files,
>> I've added "txn->serialized" to the if statement condition.
>
>
> Thanks.
>
> Re-reading the patch I see
>
> * The final_lsn of which transaction that hasn't produced an abort
> * record is invalid.
>
> which I find very hard to parse. I suggest:
>
> We set final_lsn when we decode the commit or abort record for a
> transaction,
> but transactions implicitly aborted by a crash never write such a
record.
>
> then continue from there with the same text as in the patch.
>
> Otherwise I'm happy. It passes make check, test decoding and the recovery
> TAP tests too.
>
Thanks for your kind suggestion and running test.
I've added your comment at the beginning.
--
Atsushi Torikoshi
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
diff --git a/src/backend/replication/logical/reorderbuffer.c
b/src/backend/replication/logical/reorderbuffer.c
index 0f607ba..ee28d26 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1724,6 +1724,22 @@ ReorderBufferAbortOld(ReorderBuffer *rb, TransactionId
oldestRunningXid)
if (TransactionIdPrecedes(txn->xid, oldestRunningXid))
{
+ /*
+ * We set final_lsn when we decode the commit or abort
record for
+ * a transaction, but transactions implicitly aborted
by a crash
+ * never write such a record.
+ * The final_lsn of which transaction that hasn't
produced an abort
+ * record is invalid. To ensure cleanup the serialized
changes of
+ * such transaction we set the LSN of the last change
action to
+ * final_lsn.
+ */
+ if (txn->serialized && txn->final_lsn == 0)
+ {
+ ReorderBufferChange *last_change =
+ dlist_tail_element(ReorderBufferChange, node,
&txn->changes);
+ txn->final_lsn = last_change->lsn;
+ }
+
elog(DEBUG2, "aborting old transaction %u", txn->xid);
/* remove potential on-disk data, and deallocate this
tx */
diff --git a/src/include/replication/reorderbuffer.h
b/src/include/replication/reorderbuffer.h
index 86effe1..7931757 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -168,6 +168,8 @@ typedef struct ReorderBufferTXN
* * plain abort record
* * prepared transaction abort
* * error during decoding
+ * Note that this can also be a LSN of the last change action of this
xact
+ * if it is an implicitly aborted transaction.
* ----
*/
XLogRecPtr final_lsn;
