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

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 7f2bbca302e56cb599b75001ec9b09e217840b86..f96e3e1d93bd8b6b1703d10ff7b1d29b2275fca7 100644 (file)
--- 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 cb45f906fc1c002867d6ff5fc629f3afe12de1ba..f00fd7d422e43d058f64185ec77c2cfbd3b6fa52 100644 (file)
--- 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 */