Re: Remove unused fields in ReorderBufferTupleBuf

Sorry for being absent for a while.

On Mon, Jan 22, 2024 at 11:19 PM Aleksander Alekseev
<aleksander(at)timescale(dot)com> wrote:
>
> Hi,
>
> > > But why didn't you pursue your idea of getting rid of the wrapper
> > > structure ReorderBufferTupleBuf which after this patch will have just
> > > one member? I think there could be hassles in backpatching bug-fixes
> > > in some cases but in the longer run it would make the code look clean.

+1

> >
> > Indeed. In fact turned out that I suggested the same above but
> > apparently forgot:
> >
> > > On top of that IMO it doesn't make much sense to keep a one-field
> > > wrapper structure. Perhaps we should get rid of it entirely and just
> > > use HeapTupleData instead.
> >
> > After actually trying the refactoring I agree that the code becomes
> > cleaner and it's going to be beneficial in the long run. Here is the
> > patch.
>
> I did a mistake in v4:
>
> ```
> - alloc_len = tuple_len + SizeofHeapTupleHeader;
> + alloc_len = tuple_len + HEAPTUPLESIZE;
> ```
>
> Here is the corrected patch.

Thank you for updating the patch! I have some comments:

- 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.

---
xl_parameter_change *xlrec =
- (xl_parameter_change *)
XLogRecGetData(buf->record);
+ (xl_parameter_change *)
XLogRecGetData(buf->record);

Unnecessary change.

---
-/*
- * 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.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com