Re: Re: BUG #18486: Is there something wrong with the calculation in ReorderBufferChangeSize()?

On Thu, Jun 6, 2024 at 3:36 PM Michael <xu(dot)xw2008(at)163(dot)com> wrote:
>
> At 2024-06-04 13:28:52, "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> >On Thu, May 30, 2024 at 2:30 AM PG Bug reporting form
> ><noreply(at)postgresql(dot)org> wrote:
> >>
> >> The following bug has been logged on the website:
> >>
> >> Bug reference: 18486
> >> Logged by: Xingwang Xu
> >> Email address: xu(dot)xw2008(at)163(dot)com
> >> PostgreSQL version: 17beta1
> >> Operating system: CentOS7.9
> >> Description:
> >>
> >> In the code related to logical replication, there is a function
> >> ReorderBufferChangeSize(), which is used to calculate the size of a change
> >> in memory.
> >>
> >> When looking at the ReorderBufferChangeSize() function, I saw the following
> >> code：
> >>
> >> case REORDER_BUFFER_CHANGE_MESSAGE:
> >> {
> >> Size prefix_size = strlen(change->data.msg.prefix) + 1;
> >>
> >> sz += prefix_size + change->data.msg.message_size +
> >> sizeof(Size) + sizeof(Size);
> >>
> >> break;
> >> }
> >>
> >> When calculating the change size of the message type, there are two
> >> “sizeof(Size)” in the code. It is not clear why these two “sizeof(Size)” are
> >> added and whether these two “sizeof(Size)” are redundant.
> >>
> >
> >These two sizeof(Size) are added as while serializing or restoring
> >message change, we explicitly serialize/restore the size of the prefix
> >and the actual message. See
> >ReorderBufferSerializeChange()/ReorderBufferRestoreChange().
> >
>
> Thanks for your reply.
>
>
> I see in the code that the function ReorderBufferChangeSize() is used to calculate the space in memory, while ReorderBufferSerializeChange()/ReorderBufferRestoreChange() calculates the space on disk.
>

Shouldn't what we do in ReorderBufferRestoreChange() be the size of
the change in memory? Anyway, please see the following calculation in
ReorderBufferQueueMessage()

{
...
change->data.msg.prefix = pstrdup(prefix);
change->data.msg.message_size = message_size;
change->data.msg.message = palloc(message_size);
memcpy(change->data.msg.message, message, message_size);
...

As per this, the two sizeof(Size) seems to correspond to
'message_size' and 'message' in the above calculation. So, the
calculation corresponds to the size of the change we have in memory.

--
With Regards,
Amit Kapila.