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: [UNVERIFIED SENDER] Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
Date: Tue, 7 Sep 2021 10:32:06 +0200
Message-ID: <36ed78a0-5781-11d8-4842-afce849fb0c7@amazon.com> (raw)
In-Reply-To: <7e2a1da1-80e1-64c4-1512-78e47a6711d4@amazon.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>
On 9/7/21 9:11 AM, Drouvot, Bertrand wrote:
>
> On 9/7/21 8:51 AM, Amit Kapila wrote:
>> On Tue, Sep 7, 2021 at 11:33 AM Drouvot, Bertrand
>> <bdrouvot@amazon.com> wrote:
>>> Hi,
>>>
>>> On 9/7/21 7:58 AM, Dilip Kumar wrote:
>>>
>>>
>>> On Tue, Sep 7, 2021 at 11:10 AM Amit Kapila
>>> <amit.kapila16@gmail.com> wrote:
>>>
>>>>>> Isn't it better if we use option 2) at all places as then we won't
>>>>>> need any special check inside ReorderBufferChangeMemoryUpdate()?
>>>>>
>>>>> If we want to do this then be careful about
>>>>> REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID change. Basically,
>>>>> ReorderBufferChangeMemoryUpdate() ignores this type of change
>>>>> whereas ReorderBufferChangeSize(), consider at least
>>>>> sizeof(ReorderBufferChange) bytes to this change. So if we
>>>>> compute the size using ReorderBufferChangeSize() outside of
>>>>> ReorderBufferChangeMemoryUpdate(), then total size will be
>>>>> different from what we have now. Logically, we should be
>>>>> ignoring/asserting REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID in
>>>>> ReorderBufferChangeSize(), because
>>>>> ReorderBufferChangeMemoryUpdate() is the only caller for this
>>>>> function.
>>>>>
>>>> Why can't we simply ignore it in ReorderBufferChangeMemoryUpdate() as
>>>> we are doing now?
>>>
>>> Yeah right, we can actually do that, it doesn't matter even if we
>>> are passing the size from outside.
>>>
>>> Agree, if no objections, I'll prepare a patch with the modified
>>> approach of option 2) proposed by Amit (means passing the size from
>>> the outside in all the cases).
>>>
>> Sounds reasonable. Another point that needs some thought is do we want
>> to backpatch this change till v13? I am not sure if there is any
>> user-visible bug here but maybe it is still good to fix this in back
>> branches. What do you think?
>
> Yes, +1 to backpatch till v13 "per precaution".
>
> I will first come back with a proposed version for master and once we
> agree on a final/polished version then I'll do the backpatch ones (if
> needed).
Please find enclosed patch v2 (for the master branch) implementing the
modified approach of option 2) proposed by Amit.
Thanks
Bertrand
Attachments:v2-0001-ReorderBufferChangeMemoryUpdate-failed-assertion.patch (text/plain, 3.6K) downloaddiff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.cindex 7378beb684..d409011ba7 100644--- a/src/backend/replication/logical/reorderbuffer.c+++ b/src/backend/replication/logical/reorderbuffer.c@@ -290,7 +290,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
@@ -474,7 +475,8 @@ ReorderBufferReturnChange(ReorderBuffer *rb, ReorderBufferChange *change,
{
/* update memory accounting info */
if (upd_mem)
- ReorderBufferChangeMemoryUpdate(rb, change, false);+ ReorderBufferChangeMemoryUpdate(rb, change, false,+ ReorderBufferChangeSize(change));
/* free contained data */
switch (change->action)
@@ -792,7 +794,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));
/* process partial change */
ReorderBufferProcessPartialChange(rb, txn, change, toast_insert);
@@ -3100,9 +3103,8 @@ ReorderBufferAddNewCommandId(ReorderBuffer *rb, TransactionId xid,
static void
ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
ReorderBufferChange *change,
- bool addition)+ bool addition, Size sz)
{
- Size sz;
ReorderBufferTXN *txn;
ReorderBufferTXN *toptxn;
@@ -3127,8 +3129,6 @@ ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
else
toptxn = txn;
- sz = ReorderBufferChangeSize(change);-
if (addition)
{
txn->size += sz;
@@ -4359,7 +4359,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));
}
/*
@@ -4605,17 +4606,19 @@ 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.
*/
- ReorderBufferChangeMemoryUpdate(rb, change, false);+ old_size = ReorderBufferChangeSize(change);
oldcontext = MemoryContextSwitchTo(rb->context);
@@ -4766,8 +4769,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));
}
/*
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: [UNVERIFIED SENDER] Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
In-Reply-To: <36ed78a0-5781-11d8-4842-afce849fb0c7@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