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. -- Best regards, Aleksander Alekseev
v6-0001-Remove-ReorderBufferTupleBuf-structure.patch
Description: Binary data