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 */