On Fri, Jan 26, 2024 at 4:04 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> 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.
Pushed (08e6344fd642).
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com