Re: Remove unused fields in ReorderBufferTupleBuf - Mailing list pgsql-hackers

From Masahiko Sawada
Subject Re: Remove unused fields in ReorderBufferTupleBuf
Date
Msg-id CAD21AoD1ueUe8O+bi4v8NNy2f4BKJvJAWa4vA4kbgQi28ib8hg@mail.gmail.com
Whole thread Raw
In response to Re: Remove unused fields in ReorderBufferTupleBuf  (Aleksander Alekseev <aleksander@timescale.com>)
Responses Re: Remove unused fields in ReorderBufferTupleBuf
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: "David G. Johnston"
Date:
Subject: Re: Small fix on COPY ON_ERROR document
Next
From: Richard Guo
Date:
Subject: Apply the "LIMIT 1" optimization to partial DISTINCT