Обсуждение: BUG #18486: Is there something wrong with the calculation in ReorderBufferChangeSize()?

The following bug has been logged on the website:

Bug reference:      18486
Logged by:          Xingwang Xu
Email address:      xu.xw2008@163.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.

The data in change of message type is defined as:

        struct
        {
            char       *prefix;
            Size        message_size;
            char       *message;
        }            msg;

The size of msg seems to have been calculated at the beginning of
ReorderBufferChangeSize() with “sizeof(ReorderBufferChange)”.Is it only
necessary to add the specific data space occupied by *prefix and *message?

Not super familiar with this so please let me know if there's something I've
missed.

Thanks,
Xingwang xu

On Thu, May 30, 2024 at 2:30 AM PG Bug reporting form
<noreply@postgresql.org> wrote:
>
> The following bug has been logged on the website:
>
> Bug reference:      18486
> Logged by:          Xingwang Xu
> Email address:      xu.xw2008@163.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().

--
With Regards,
Amit Kapila.

On Thu, Jun 6, 2024 at 3:36 PM Michael <xu.xw2008@163.com> wrote:
>
> At 2024-06-04 13:28:52, "Amit Kapila" <amit.kapila16@gmail.com> wrote:
>
> >On Thu, May 30, 2024 at 2:30 AM PG Bug reporting form
> ><noreply@postgresql.org> wrote:
> >>
> >> The following bug has been logged on the website:
> >>
> >> Bug reference:      18486
> >> Logged by:          Xingwang Xu
> >> Email address:      xu.xw2008@163.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.