From: Drouvot, Bertrand <bdrouvot@amazon.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
Date: Thu, 9 Sep 2021 15:16:00 +0200
Message-ID: <f2dbb309-2152-08a9-c985-474ba60eab53@amazon.com> (raw)
In-Reply-To: <CAA4eK1L5s=CGM-q+zC=Vsr3e5_SMwMV_w_aFpge=R4rHHNyUNQ@mail.gmail.com>
References: <92b0ee65-b8bd-e42d-c082-4f3f4bf12d34@amazon.com>
<CAA4eK1LfzPdTO73vW2qUkJeB1k1iL04rmqVJseBbF5qRAonmiw@mail.gmail.com>
<CAFiTN-uoTz0bUtOoZxJcB7kPt9xMrKsDATTXoJfktJtPOfC_bw@mail.gmail.com>
<73dcd904-cfee-21f2-d363-317cd517cb00@amazon.com>
<CAFiTN-vMGg3EEe4icesj4LJ_g0JWYq0SqHEPbtQzm+fwfaz4cg@mail.gmail.com>
<CAA4eK1KDYWp4mtCOpGymMHXd1oi_Wqy+LS7X-NwpF=pp2MSJSg@mail.gmail.com>
<CAFiTN-uZmCkLG=7Ak32tyE+KT38XhZ48pZTc7OLuX3zvDXSwRQ@mail.gmail.com>
<CAA4eK1L_HHFnZB+B68h9qv_qqkLbjYbw7M653+LjM93_N4OzNg@mail.gmail.com>
<CAFiTN-smGWa8zYVsQVm7qRdXsr7JD8sokcKdfBhyF1onRORGdw@mail.gmail.com>
<51babafd-4c0c-9b69-ec5c-5e4740d2606b@amazon.com>
<CAA4eK1L9JbhsHZpeaFrAMc7ZrPUHqd6AAERxqwZQ5n9J4=X1-g@mail.gmail.com>
<7e2a1da1-80e1-64c4-1512-78e47a6711d4@amazon.com>
<36ed78a0-5781-11d8-4842-afce849fb0c7@amazon.com>
<CAA4eK1L5s=CGM-q+zC=Vsr3e5_SMwMV_w_aFpge=R4rHHNyUNQ@mail.gmail.com>
Hi,
On 9/9/21 1:16 PM, Amit Kapila wrote:
> On Tue, Sep 7, 2021 at 2:02 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>> On 9/7/21 9:11 AM, Drouvot, Bertrand wrote:
>> Please find enclosed patch v2 (for the master branch) implementing the
>> modified approach of option 2) proposed by Amit.
>>
> The patch looks good to me. I have made a minor modification in the
> comment to make it explicit why we are not subtracting the size
> immediately
Thanks!
Indeed, good point to also mention in the comment why it has been done
that way.
> Kindly prepare back-branch patches and
> test the same.
Please find enclosed the modified (and tested) version for the
REL_13_STABLE branch (the master patch version also applied on
REL_14_STABLE).
Thanks
Bertrand
Attachments:v3-0001-Fix-reorder-buffer-memory-accounting-for-toast-ch-13_STABLE.patch (text/plain, 5.1K) download
From 71f035b9f34e98251b91bcba3030d8b3317904c6 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Thu, 9 Sep 2021 16:21:44 +0530
Subject: [PATCH v3] Fix reorder buffer memory accounting for toast changes.
While processing toast changes in logical decoding, we rejigger the
tuple change to point to in-memory toast tuples instead to on-disk toast
tuples. And, to make sure the memory accounting is correct, we were
subtracting the old change size and then after re-computing the new tuple,
re-adding its size at the end. Now, if there is any error before we add
the new size, we will release the changes and that will update the
accounting info (subtracting the size from the counters). And we were
underflowing there which leads to an assertion failure in assert enabled
builds and wrong memory accounting in reorder buffer otherwise.
Author: Bertrand Drouvot
Reviewed-by: Amit Kapila
Backpatch-through: 13, where memory accounting was introduced
Discussion: https://postgr.es/m/92b0ee65-b8bd-e42d-c082-4f3f4bf12d34@amazon.com
---
src/backend/replication/logical/reorderbuffer.c | 37 +++++++++++++++----------
1 file changed, 23 insertions(+), 14 deletions(-)
100.0% src/backend/replication/logical/
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.cindex b904697f18..9264dc247a 100644--- a/src/backend/replication/logical/reorderbuffer.c+++ b/src/backend/replication/logical/reorderbuffer.c@@ -262,7 +262,8 @@ static void ReorderBufferToastAppendChunk(ReorderBuffer *rb, ReorderBufferTXN *t
*/
static Size ReorderBufferChangeSize(ReorderBufferChange *change);
static void ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
- ReorderBufferChange *change, bool addition);+ ReorderBufferChange *change,+ bool addition, Size sz);
/*
* Allocate a new ReorderBuffer and clean out any old serialized state from
@@ -425,7 +426,8 @@ void
ReorderBufferReturnChange(ReorderBuffer *rb, ReorderBufferChange *change)
{
/* update memory accounting info */
- ReorderBufferChangeMemoryUpdate(rb, change, false);+ ReorderBufferChangeMemoryUpdate(rb, change, false,+ ReorderBufferChangeSize(change));
/* free contained data */
switch (change->action)
@@ -647,7 +649,8 @@ ReorderBufferQueueChange(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn,
txn->nentries_mem++;
/* update memory accounting information */
- ReorderBufferChangeMemoryUpdate(rb, change, true);+ ReorderBufferChangeMemoryUpdate(rb, change, true,+ ReorderBufferChangeSize(change));
/* check the memory limits and evict something if needed */
ReorderBufferCheckMemoryLimit(rb);
@@ -2160,10 +2163,8 @@ ReorderBufferAddNewCommandId(ReorderBuffer *rb, TransactionId xid,
static void
ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
ReorderBufferChange *change,
- bool addition)+ bool addition, Size sz)
{
- Size sz;-
Assert(change->txn);
/*
@@ -2174,8 +2175,6 @@ ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
if (change->action == REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID)
return;
- sz = ReorderBufferChangeSize(change);-
if (addition)
{
change->txn->size += sz;
@@ -3073,7 +3072,8 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
* update the accounting too (subtracting the size from the counters). And
* we don't want to underflow there.
*/
- ReorderBufferChangeMemoryUpdate(rb, change, true);+ ReorderBufferChangeMemoryUpdate(rb, change, true,+ ReorderBufferChangeSize(change));
}
/*
@@ -3321,17 +3321,23 @@ ReorderBufferToastReplace(ReorderBuffer *rb, ReorderBufferTXN *txn,
TupleDesc toast_desc;
MemoryContext oldcontext;
ReorderBufferTupleBuf *newtup;
+ Size old_size;
/* no toast tuples changed */
if (txn->toast_hash == NULL)
return;
/*
- * We're going to modify the size of the change, so to make sure the- * accounting is correct we'll make it look like we're removing the change- * now (with the old size), and then re-add it at the end.+ * We're going to modify the size of the change. So, to make sure the+ * accounting is correct we record the current change size and then after+ * re-computing the change we'll subtract the recorded size and then+ * re-add the new change size at the end. We don't immediately subtract+ * the old size because if there is any error before we add the new size,+ * we will release the changes and that will update the accounting info+ * (subtracting the size from the counters). And we don't want to+ * underflow there.
*/
- ReorderBufferChangeMemoryUpdate(rb, change, false);+ old_size = ReorderBufferChangeSize(change);
oldcontext = MemoryContextSwitchTo(rb->context);
@@ -3482,8 +3488,11 @@ ReorderBufferToastReplace(ReorderBuffer *rb, ReorderBufferTXN *txn,
MemoryContextSwitchTo(oldcontext);
+ /* substract the old change size */+ ReorderBufferChangeMemoryUpdate(rb, change, false, old_size);
/* now add the change back, with the correct size */
- ReorderBufferChangeMemoryUpdate(rb, change, true);+ ReorderBufferChangeMemoryUpdate(rb, change, true,+ ReorderBufferChangeSize(change));
}
/*
--
2.16.6
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: bdrouvot@amazon.com, amit.kapila16@gmail.com, dilipbalaut@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
In-Reply-To: <f2dbb309-2152-08a9-c985-474ba60eab53@amazon.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox