On Thu, Jan 25, 2024 at 10:17 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> Hi,
>
> > > Here is the corrected patch.
> >
> > Thank you for updating the patch! I have some comments:
>
> Thanks for the review.
>
> > - tuple = (ReorderBufferTupleBuf *)
> > + tuple = (HeapTuple)
> > MemoryContextAlloc(rb->tup_context,
> > -
> > sizeof(ReorderBufferTupleBuf) +
> > + HEAPTUPLESIZE +
> > MAXIMUM_ALIGNOF +
> > alloc_len);
> > - tuple->alloc_tuple_size = alloc_len;
> > - tuple->tuple.t_data = ReorderBufferTupleBufData(tuple);
> > + tuple->t_data = (HeapTupleHeader)((char *)tuple + HEAPTUPLESIZE);
> >
> > Why do we need to add MAXIMUM_ALIGNOF? since HEAPTUPLESIZE is the
> > MAXALIGN'd size, it seems we don't need it. heap_form_tuple() does a
> > similar thing but it doesn't add it.
>
> Indeed. I gave it a try and nothing crashed, so it appears that
> MAXIMUM_ALIGNOF is not needed.
>
> > ---
> > xl_parameter_change *xlrec =
> > - (xl_parameter_change *)
> > XLogRecGetData(buf->record);
> > + (xl_parameter_change *)
> > XLogRecGetData(buf->record);
> >
> > Unnecessary change.
>
> That's pgindent. Fixed.
>
> > ---
> > -/*
> > - * Free a ReorderBufferTupleBuf.
> > - */
> > -void
> > -ReorderBufferReturnTupleBuf(ReorderBuffer *rb, ReorderBufferTupleBuf *tuple)
> > -{
> > - pfree(tuple);
> > -}
> > -
> >
> > Why does ReorderBufferReturnTupleBuf need to be moved from
> > reorderbuffer.c to reorderbuffer.h? It seems not related to this
> > refactoring patch so I think we should do it in a separate patch if we
> > really want it. I'm not sure it's necessary, though.
>
> OK, fixed.
Thank you for updating the patch. It looks good to me. I'm going to
push it next week, barring any objections.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com