From b63bea5fd3bba4d7a61c3beaba51a06f24b38da6 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Mon, 7 Mar 2016 14:24:03 -0800 Subject: [PATCH] Further improvements to c8f621c43. Coverity and inspection for the issue addressed in fd45d16f found some questionable code. Specifically coverity noticed that the wrong length was added in ReorderBufferSerializeChange() - without immediate negative consequences as the variable isn't used afterwards. During code-review and testing I noticed that a bit of space was wasted when allocating tuple bufs in several places. Thirdly, the debug memset()s in ReorderBufferGetTupleBuf() reduce the error checking valgrind can do. Backpatch: 9.4, like c8f621c43. --- src/backend/replication/logical/decode.c | 28 +++++++++++++------ .../replication/logical/reorderbuffer.c | 5 +++- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c index 36e6d9a5c9..13af48524a 100644 --- a/src/backend/replication/logical/decode.c +++ b/src/backend/replication/logical/decode.c @@ -607,13 +607,14 @@ DecodeInsert(LogicalDecodingContext *ctx, XLogRecordBuffer *buf) if (xlrec->flags & XLH_INSERT_CONTAINS_NEW_TUPLE) { - Size tuplelen; - char *tupledata = XLogRecGetBlockData(r, 0, &tuplelen); + Size datalen; + char *tupledata = XLogRecGetBlockData(r, 0, &datalen); + Size tuplelen = datalen - SizeOfHeapHeader; change->data.tp.newtuple = ReorderBufferGetTupleBuf(ctx->reorder, tuplelen); - DecodeXLogTuple(tupledata, tuplelen, change->data.tp.newtuple); + DecodeXLogTuple(tupledata, datalen, change->data.tp.newtuple); } change->data.tp.clear_toast_afterwards = true; @@ -634,7 +635,6 @@ DecodeUpdate(LogicalDecodingContext *ctx, XLogRecordBuffer *buf) xl_heap_update *xlrec; ReorderBufferChange *change; char *data; - Size datalen; RelFileNode target_node; xlrec = (xl_heap_update *) XLogRecGetData(r); @@ -655,22 +655,31 @@ DecodeUpdate(LogicalDecodingContext *ctx, XLogRecordBuffer *buf) if (xlrec->flags & XLH_UPDATE_CONTAINS_NEW_TUPLE) { + Size datalen; + Size tuplelen; + data = XLogRecGetBlockData(r, 0, &datalen); + tuplelen = datalen - SizeOfHeapHeader; + change->data.tp.newtuple = - ReorderBufferGetTupleBuf(ctx->reorder, datalen); + ReorderBufferGetTupleBuf(ctx->reorder, tuplelen); DecodeXLogTuple(data, datalen, change->data.tp.newtuple); } if (xlrec->flags & XLH_UPDATE_CONTAINS_OLD) { + Size datalen; + Size tuplelen; + /* caution, remaining data in record is not aligned */ data = XLogRecGetData(r) + SizeOfHeapUpdate; datalen = XLogRecGetDataLen(r) - SizeOfHeapUpdate; + tuplelen = datalen - SizeOfHeapHeader; change->data.tp.oldtuple = - ReorderBufferGetTupleBuf(ctx->reorder, datalen); + ReorderBufferGetTupleBuf(ctx->reorder, tuplelen); DecodeXLogTuple(data, datalen, change->data.tp.oldtuple); } @@ -720,15 +729,16 @@ DecodeDelete(LogicalDecodingContext *ctx, XLogRecordBuffer *buf) /* old primary key stored */ if (xlrec->flags & XLH_DELETE_CONTAINS_OLD) { - Size len = XLogRecGetDataLen(r) - SizeOfHeapDelete; + Size datalen = XLogRecGetDataLen(r) - SizeOfHeapDelete; + Size tuplelen = datalen - SizeOfHeapHeader; Assert(XLogRecGetDataLen(r) > (SizeOfHeapDelete + SizeOfHeapHeader)); change->data.tp.oldtuple = - ReorderBufferGetTupleBuf(ctx->reorder, len); + ReorderBufferGetTupleBuf(ctx->reorder, tuplelen); DecodeXLogTuple((char *) xlrec + SizeOfHeapDelete, - len, change->data.tp.oldtuple); + datalen, change->data.tp.oldtuple); } change->data.tp.clear_toast_afterwards = true; diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index e2ad4728cd..f2b8f4b7b8 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -471,12 +471,15 @@ ReorderBufferGetTupleBuf(ReorderBuffer *rb, Size tuple_len) rb->nr_cached_tuplebufs--; tuple = slist_container(ReorderBufferTupleBuf, node, slist_pop_head_node(&rb->cached_tuplebufs)); + Assert(tuple->alloc_tuple_size == MaxHeapTupleSize); #ifdef USE_ASSERT_CHECKING memset(&tuple->tuple, 0xa9, sizeof(HeapTupleData)); + VALGRIND_MAKE_MEM_UNDEFINED(&tuple->tuple, sizeof(HeapTupleData)); #endif tuple->tuple.t_data = ReorderBufferTupleBufData(tuple); #ifdef USE_ASSERT_CHECKING memset(tuple->tuple.t_data, 0xa8, tuple->alloc_tuple_size); + VALGRIND_MAKE_MEM_UNDEFINED(tuple->tuple.t_data, tuple->alloc_tuple_size); #endif } else @@ -2152,7 +2155,7 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn, data += sizeof(HeapTupleData); memcpy(data, newtup->tuple.t_data, newlen); - data += oldlen; + data += newlen; } break; } -- 2.40.0