Measure string lengths only once

Bernd Helmle complained that CreateReplicationSlot() was assigning the
same value to the same variable twice, so we could remove one of them.
Code inspection reveals that we can actually remove both assignments:
according to the author the assignment was there for beauty of the
strlen line only, and another possible fix to that is to put the strlen
in its own line, so do that.

To be consistent within the file, refactor all duplicated strlen()
calls, which is what we do elsewhere in the backend anyway.  In
basebackup.c, snprintf already returns the right length; no need for
strlen afterwards.

Backpatch to 9.4, where replication slots were introduced, to keep code
identical.  Some of this is older, but the patch doesn't apply cleanly
and it's only of cosmetic value anyway.

Discussion: http://www.postgresql.org/message-id/BE2FD71DEA35A2287EA5F018@eje.credativ.lan

diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index bffed284431c7040daf6b5df77716f0f5c02e789..de019edb24ebba40695efaa3888fa5794b8a530f 100644 (file)
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -748,10 +748,15 @@ SendBackupHeader(List *tablespaces)
                }
                else
                {
-                       pq_sendint(&buf, strlen(ti->oid), 4);           /* length */
-                       pq_sendbytes(&buf, ti->oid, strlen(ti->oid));
-                       pq_sendint(&buf, strlen(ti->path), 4);          /* length */
-                       pq_sendbytes(&buf, ti->path, strlen(ti->path));
+                       Size    len;
+
+                       len = strlen(ti->oid);
+                       pq_sendint(&buf, len, 4);
+                       pq_sendbytes(&buf, ti->oid, len);
+
+                       len = strlen(ti->path);
+                       pq_sendint(&buf, len, 4);
+                       pq_sendbytes(&buf, ti->path, len);
                }
                if (ti->size >= 0)
                        send_int8_string(&buf, ti->size / 1024);
@@ -774,6 +779,7 @@ SendXlogRecPtrResult(XLogRecPtr ptr, TimeLineID tli)
 {
        StringInfoData buf;
        char            str[MAXFNAMELEN];
+       Size            len;
 
        pq_beginmessage(&buf, 'T'); /* RowDescription */
        pq_sendint(&buf, 2, 2);         /* 2 fields */
@@ -792,7 +798,7 @@ SendXlogRecPtrResult(XLogRecPtr ptr, TimeLineID tli)
        pq_sendint(&buf, 0, 2);         /* attnum */
 
        /*
-        * int8 may seem like a surprising data type for this, but in thory int4
+        * int8 may seem like a surprising data type for this, but in theory int4
         * would not be wide enough for this, as TimeLineID is unsigned.
         */
        pq_sendint(&buf, INT8OID, 4);           /* type oid */
@@ -805,13 +811,15 @@ SendXlogRecPtrResult(XLogRecPtr ptr, TimeLineID tli)
        pq_beginmessage(&buf, 'D');
        pq_sendint(&buf, 2, 2);         /* number of columns */
 
-       snprintf(str, sizeof(str), "%X/%X", (uint32) (ptr >> 32), (uint32) ptr);
-       pq_sendint(&buf, strlen(str), 4);       /* length */
-       pq_sendbytes(&buf, str, strlen(str));
+       len = snprintf(str, sizeof(str),
+                                  "%X/%X", (uint32) (ptr >> 32), (uint32) ptr);
+       pq_sendint(&buf, len, 4);
+       pq_sendbytes(&buf, str, len);
+
+       len = snprintf(str, sizeof(str), "%u", tli);
+       pq_sendint(&buf, len, 4);
+       pq_sendbytes(&buf, str, len);
 
-       snprintf(str, sizeof(str), "%u", tli);
-       pq_sendint(&buf, strlen(str), 4);       /* length */
-       pq_sendbytes(&buf, str, strlen(str));
        pq_endmessage(&buf);
 
        /* Send a CommandComplete message */

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index f8dd3c3cf915e5e5d302496145d193f87d4f367b..df064d29a3b2244079d2bcdc88b5bbeeae85e7d7 100644 (file)
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -298,6 +298,7 @@ IdentifySystem(void)
        char            xpos[MAXFNAMELEN];
        XLogRecPtr      logptr;
        char       *dbname = NULL;
+       Size            len;
 
        /*
         * Reply with a result set with one row, four columns. First col is system
@@ -379,21 +380,32 @@ IdentifySystem(void)
        /* Send a DataRow message */
        pq_beginmessage(&buf, 'D');
        pq_sendint(&buf, 4, 2);         /* # of columns */
-       pq_sendint(&buf, strlen(sysid), 4); /* col1 len */
-       pq_sendbytes(&buf, (char *) &sysid, strlen(sysid));
-       pq_sendint(&buf, strlen(tli), 4);       /* col2 len */
-       pq_sendbytes(&buf, (char *) tli, strlen(tli));
-       pq_sendint(&buf, strlen(xpos), 4);      /* col3 len */
-       pq_sendbytes(&buf, (char *) xpos, strlen(xpos));
-       /* send NULL if not connected to a database */
+
+       /* column 1: system identifier */
+       len = strlen(sysid);
+       pq_sendint(&buf, len, 4);
+       pq_sendbytes(&buf, (char *) &sysid, len);
+
+       /* column 2: timeline */
+       len = strlen(tli);
+       pq_sendint(&buf, len, 4);
+       pq_sendbytes(&buf, (char *) tli, len);
+
+       /* column 3: xlog position */
+       len = strlen(xpos);
+       pq_sendint(&buf, len, 4);
+       pq_sendbytes(&buf, (char *) xpos, len);
+
+       /* column 4: database name, or NULL if none */
        if (dbname)
        {
-               pq_sendint(&buf, strlen(dbname), 4);    /* col4 len */
-               pq_sendbytes(&buf, (char *) dbname, strlen(dbname));
+               len = strlen(dbname);
+               pq_sendint(&buf, len, 4);
+               pq_sendbytes(&buf, (char *) dbname, len);
        }
        else
        {
-               pq_sendint(&buf, -1, 4);        /* col4 len, NULL */
+               pq_sendint(&buf, -1, 4);
        }
 
        pq_endmessage(&buf);
@@ -412,6 +424,7 @@ SendTimeLineHistory(TimeLineHistoryCmd *cmd)
        int                     fd;
        off_t           histfilelen;
        off_t           bytesleft;
+       Size            len;
 
        /*
         * Reply with a result set with one row, and two columns. The first col is
@@ -447,8 +460,9 @@ SendTimeLineHistory(TimeLineHistoryCmd *cmd)
        /* Send a DataRow message */
        pq_beginmessage(&buf, 'D');
        pq_sendint(&buf, 2, 2);         /* # of columns */
-       pq_sendint(&buf, strlen(histfname), 4);         /* col1 len */
-       pq_sendbytes(&buf, histfname, strlen(histfname));
+       len = strlen(histfname);
+       pq_sendint(&buf, len, 4);               /* col1 len */
+       pq_sendbytes(&buf, histfname, len);
 
        fd = OpenTransientFile(path, O_RDONLY | PG_BINARY, 0666);
        if (fd < 0)
@@ -674,6 +688,7 @@ StartReplication(StartReplicationCmd *cmd)
        {
                char            tli_str[11];
                char            startpos_str[8 + 1 + 8 + 1];
+               Size            len;
 
                snprintf(tli_str, sizeof(tli_str), "%u", sendTimeLineNextTLI);
                snprintf(startpos_str, sizeof(startpos_str), "%X/%X",
@@ -710,11 +725,13 @@ StartReplication(StartReplicationCmd *cmd)
                pq_beginmessage(&buf, 'D');
                pq_sendint(&buf, 2, 2); /* number of columns */
 
-               pq_sendint(&buf, strlen(tli_str), 4);   /* length */
-               pq_sendbytes(&buf, tli_str, strlen(tli_str));
+               len = strlen(tli_str);
+               pq_sendint(&buf, len, 4);       /* length */
+               pq_sendbytes(&buf, tli_str, len);
 
-               pq_sendint(&buf, strlen(startpos_str), 4);              /* length */
-               pq_sendbytes(&buf, startpos_str, strlen(startpos_str));
+               len = strlen(startpos_str);
+               pq_sendint(&buf, len, 4);               /* length */
+               pq_sendbytes(&buf, startpos_str, len);
 
                pq_endmessage(&buf);
        }
@@ -762,10 +779,10 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
 static void
 CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
 {
-       const char *slot_name;
        const char *snapshot_name = NULL;
        char            xpos[MAXFNAMELEN];
        StringInfoData buf;
+       Size            len;
 
        Assert(!MyReplicationSlot);
 
@@ -790,14 +807,11 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
 
        initStringInfo(&output_message);
 
-       slot_name = NameStr(MyReplicationSlot->data.name);
-
        if (cmd->kind == REPLICATION_KIND_LOGICAL)
        {
                LogicalDecodingContext *ctx;
 
-               ctx = CreateInitDecodingContext(
-                                                                               cmd->plugin, NIL,
+               ctx = CreateInitDecodingContext(cmd->plugin, NIL,
                                                                                logical_read_xlog_page,
                                                                                WalSndPrepareWrite, WalSndWriteData);
 
@@ -825,7 +839,6 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
                ReplicationSlotPersist();
        }
 
-       slot_name = NameStr(MyReplicationSlot->data.name);
        snprintf(xpos, sizeof(xpos), "%X/%X",
                         (uint32) (MyReplicationSlot->data.confirmed_flush >> 32),
                         (uint32) MyReplicationSlot->data.confirmed_flush);
@@ -876,30 +889,34 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
        pq_sendint(&buf, 4, 2);         /* # of columns */
 
        /* slot_name */
-       pq_sendint(&buf, strlen(slot_name), 4);         /* col1 len */
-       pq_sendbytes(&buf, slot_name, strlen(slot_name));
+       len = strlen(NameStr(MyReplicationSlot->data.name));
+       pq_sendint(&buf, len, 4);               /* col1 len */
+       pq_sendbytes(&buf, NameStr(MyReplicationSlot->data.name), len);
 
        /* consistent wal location */
-       pq_sendint(&buf, strlen(xpos), 4);      /* col2 len */
-       pq_sendbytes(&buf, xpos, strlen(xpos));
+       len = strlen(xpos);
+       pq_sendint(&buf, len, 4);
+       pq_sendbytes(&buf, xpos, len);
 
-       /* snapshot name */
+       /* snapshot name, or NULL if none */
        if (snapshot_name != NULL)
        {
-               pq_sendint(&buf, strlen(snapshot_name), 4);             /* col3 len */
-               pq_sendbytes(&buf, snapshot_name, strlen(snapshot_name));
+               len = strlen(snapshot_name);
+               pq_sendint(&buf, len, 4);
+               pq_sendbytes(&buf, snapshot_name, len);
        }
        else
-               pq_sendint(&buf, -1, 4);        /* col3 len, NULL */
+               pq_sendint(&buf, -1, 4);
 
-       /* plugin */
+       /* plugin, or NULL if none */
        if (cmd->plugin != NULL)
        {
-               pq_sendint(&buf, strlen(cmd->plugin), 4);               /* col4 len */
-               pq_sendbytes(&buf, cmd->plugin, strlen(cmd->plugin));
+               len = strlen(cmd->plugin);
+               pq_sendint(&buf, len, 4);
+               pq_sendbytes(&buf, cmd->plugin, len);
        }
        else
-               pq_sendint(&buf, -1, 4);        /* col4 len, NULL */
+               pq_sendint(&buf, -1, 4);
 
        pq_endmessage(&buf);