Subject: Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
To: Amit Kapila <amit.kapila16@gmail.com>
CC: Dilip Kumar <dilipbalaut@gmail.com>, PostgreSQL Hackers
	<pgsql-hackers@lists.postgresql.org>
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>
From: "Drouvot, Bertrand" <bdrouvot@amazon.com>
Message-ID: <7e2a1da1-80e1-64c4-1512-78e47a6711d4@amazon.com>
Date: Tue, 7 Sep 2021 09:11:01 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: 
 <CAA4eK1L9JbhsHZpeaFrAMc7ZrPUHqd6AAERxqwZQ5n9J4=X1-g@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Archived-At: 
 <https://www.postgresql.org/message-id/7e2a1da1-80e1-64c4-1512-78e47a6711d4%40amazon.com>
Precedence: bulk


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).

Thanks

Bertrand