Sorry for being absent for a while. On Mon, Jan 22, 2024 at 11:19 PM Aleksander Alekseev <aleksan...@timescale.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