Code review for logical decoding patch.

Post-commit review identified a number of places where addition was
used instead of multiplication or memory wasn't zeroed where it should
have been.  This commit also fixes one case where a structure member
was mis-initialized, and moves another memory allocation closer to
the place where the allocated storage is used for clarity.

Andres Freund

2 files changed, 10 insertions, 14 deletions

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 7f2bbca302e..f96e3e1d93b 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2064,15 +2064,15 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
 				if (snap->xcnt)
 				{
 					memcpy(data, snap->xip,
-						   sizeof(TransactionId) + snap->xcnt);
-					data += sizeof(TransactionId) + snap->xcnt;
+						   sizeof(TransactionId) * snap->xcnt);
+					data += sizeof(TransactionId) * snap->xcnt;
 				}
 
 				if (snap->subxcnt)
 				{
 					memcpy(data, snap->subxip,
-						   sizeof(TransactionId) + snap->subxcnt);
-					data += sizeof(TransactionId) + snap->subxcnt;
+						   sizeof(TransactionId) * snap->subxcnt);
+					data += sizeof(TransactionId) * snap->subxcnt;
 				}
 				break;
 			}
@@ -2168,15 +2168,12 @@ ReorderBufferRestoreChanges(ReorderBuffer *rb, ReorderBufferTXN *txn,
 
 		}
 
-		ReorderBufferSerializeReserve(rb, sizeof(ReorderBufferDiskChange));
-
-
 		/*
 		 * Read the statically sized part of a change which has information
 		 * about the total size. If we couldn't read a record, we're at the
 		 * end of this file.
 		 */
-
+		ReorderBufferSerializeReserve(rb, sizeof(ReorderBufferDiskChange));
 		readBytes = read(*fd, rb->outbuf, sizeof(ReorderBufferDiskChange));
 
 		/* eof */
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index cb45f906fc1..f00fd7d422e 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -307,8 +307,7 @@ AllocateSnapshotBuilder(ReorderBuffer *reorder,
 	builder->committed.xip =
 		palloc0(builder->committed.xcnt_space * sizeof(TransactionId));
 	builder->committed.includes_all_transactions = true;
-	builder->committed.xip =
-		palloc0(builder->committed.xcnt_space * sizeof(TransactionId));
+
 	builder->initial_xmin_horizon = xmin_horizon;
 	builder->transactions_after = start_lsn;
 
@@ -1691,7 +1690,7 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
 
 	/* restore running xacts information */
 	sz = sizeof(TransactionId) * ondisk.builder.running.xcnt_space;
-	ondisk.builder.running.xip = MemoryContextAlloc(builder->context, sz);
+	ondisk.builder.running.xip = MemoryContextAllocZero(builder->context, sz);
 	readBytes = read(fd, ondisk.builder.running.xip, sz);
 	if (readBytes != sz)
 	{
@@ -1705,7 +1704,7 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
 
 	/* restore committed xacts information */
 	sz = sizeof(TransactionId) * ondisk.builder.committed.xcnt;
-	ondisk.builder.committed.xip = MemoryContextAlloc(builder->context, sz);
+	ondisk.builder.committed.xip = MemoryContextAllocZero(builder->context, sz);
 	readBytes = read(fd, ondisk.builder.committed.xip, sz);
 	if (readBytes != sz)
 	{
@@ -1763,10 +1762,10 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
 	}
 	ondisk.builder.committed.xip = NULL;
 
-	builder->running.xcnt = ondisk.builder.committed.xcnt;
+	builder->running.xcnt = ondisk.builder.running.xcnt;
 	if (builder->running.xip)
 		pfree(builder->running.xip);
-	builder->running.xcnt_space = ondisk.builder.committed.xcnt_space;
+	builder->running.xcnt_space = ondisk.builder.running.xcnt_space;
 	builder->running.xip = ondisk.builder.running.xip;
 
 	/* our snapshot is not interesting anymore, build a new one */