Duplicated assignment of slot_name in walsender.c

On 20-10-2015 08:28, Bernd Helmle wrote:

The 2nd assignment to slot_name looks unnecessary?

Yes, it is. Seems to be an oversight. Patch attached.

--
Euler Taveira Timbira - http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Euler Taveira wrote:

It seems that the 2nd assignment was an oversight. Spotted by Bernd
Helmle.

I think the first assignment is also pointless -- I mean can't we just
use MyReplicationSlot->data.name in both places where slot_name is used?

In the same routine, it seems wasteful to be doing strlen() twice for
every string sent over the wire.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2015-10-21 17:21:25 -0300, Alvaro Herrera wrote:

It seems that the 2nd assignment was an oversight. Spotted by Bernd
Helmle.

Yea, that's obviously redudant. Will remove.

I think the first assignment is also pointless -- I mean can't we just
use MyReplicationSlot->data.name in both places where slot_name is used?

Makes the lines a bit long. strlen(NameStr(MyReplicationSlot->data.name)) ...

In the same routine, it seems wasteful to be doing strlen() twice for
every string sent over the wire.

That seems fairly insignificant. For one this is a rather infrequent and
expensive operation, for another every decent compiler can optimize
those away. Note that those duplicate strlen() calls are there in a lot
of places in walsender.c

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund wrote:

On 2015-10-21 17:21:25 -0300, Alvaro Herrera wrote:

I think the first assignment is also pointless -- I mean can't we just
use MyReplicationSlot->data.name in both places where slot_name is used?

Makes the lines a bit long. strlen(NameStr(MyReplicationSlot->data.name)) ...

Meh. Assign the strlen somewhere?

In the same routine, it seems wasteful to be doing strlen() twice for
every string sent over the wire.

That seems fairly insignificant. For one this is a rather infrequent and
expensive operation,

OK, I can buy that.

for another every decent compiler can optimize those away. Note that
those duplicate strlen() calls are there in a lot of places in
walsender.c

It can? Tom has repeatedly argue the opposite, in the past.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund wrote:

That seems fairly insignificant. For one this is a rather infrequent and
expensive operation, for another every decent compiler can optimize
those away. Note that those duplicate strlen() calls are there in a lot
of places in walsender.c

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index c6043cd..5487cc0 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -762,10 +762,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;
+	int			len;

Assert(!MyReplicationSlot);

@@ -791,14 +791,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);

@@ -834,7 +831,6 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
ReplicationSlotSave();
}

-	slot_name = NameStr(MyReplicationSlot->data.name);
 	snprintf(xpos, sizeof(xpos), "%X/%X",
 			 (uint32) (MyReplicationSlot->data.confirmed_flush >> 32),
 			 (uint32) MyReplicationSlot->data.confirmed_flush);
@@ -885,18 +881,21 @@ 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);	/* col2 len */
+	pq_sendbytes(&buf, xpos, len);

 	/* snapshot name */
 	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);		/* col3 len */
+		pq_sendbytes(&buf, snapshot_name, len);
 	}
 	else
 		pq_sendint(&buf, -1, 4);	/* col3 len, NULL */
@@ -904,8 +903,9 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
 	/* plugin */
 	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);		/* col4 len */
+		pq_sendbytes(&buf, cmd->plugin, len);
 	}
 	else
 		pq_sendint(&buf, -1, 4);	/* col4 len, NULL */

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2015-10-21 19:36:16 -0300, Alvaro Herrera wrote:

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index c6043cd..5487cc0 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -762,10 +762,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;
+	int			len;

If you want to do that, I'm unenthusiastically not objecting. But if so,
let's also do it in IdentifySystem(), SendTimeLineHistory(),
StartReplication(), SendBackupHeader(), SendXLogRecPtResult() - they're
modeled after each other.

Do you want to commit the slot_name with the rest or do you want me to
do that?

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Andres Freund wrote:

for another every decent compiler can optimize those away. Note that
those duplicate strlen() calls are there in a lot of places in
walsender.c

It can? Tom has repeatedly argue the opposite, in the past.

I'm prepared to believe that *some* compilers do that, but I think it's
doubtful that they all do. Even if they do, it would have to be a very
tightly constrained optimization, since the compiler would have to be able
to prove that there is no way for the referenced string to get changed
between the two call sites. That would likely mean that some places where
you think this will happen will actually end up doing the strlen() twice.

I'm willing to buy the argument that performance doesn't matter in this
particular context; but if we think it does, I'd rather see us explicitly
save and re-use the strlen() result, so that the code behaves the same on
every platform.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10/22/2015 12:36 AM, Alvaro Herrera wrote:

Andres Freund wrote:

That seems fairly insignificant. For one this is a rather infrequent and
expensive operation, for another every decent compiler can optimize
those away. Note that those duplicate strlen() calls are there in a lot
of places in walsender.c

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index c6043cd..5487cc0 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -762,10 +762,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;
+	int			len;

Surely "size_t len" ?
Or am I missing some platform where size_t is not defined ?

Minor nitpicking, of course. But once we are cleaning the code up,
might as well change this too....

Thanks!

/ J.L.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund wrote:

If you want to do that, I'm unenthusiastically not objecting. But if so,
let's also do it in IdentifySystem(), SendTimeLineHistory(),
StartReplication(), SendBackupHeader(), SendXLogRecPtResult() - they're
modeled after each other.

Okay, pushed with that, backpatched to 9.4 which is where it all applied
with no conflicts, to ease future backpatching pain. (Some of this code
exists back in 9.3, but git generated a lot of conflicts in
cherry-picking so I didn't bother).

In SendXLogRecPtrResult() we now rely on snprintf's return value, rather
than doing a separate strlen call. We do have some other places (not a
lot admittedly) in which we rely on snprintf returning correctly. I
assume that's the norm when there's no possibility of failure.

I wonder why we use MAXFNAMELEN to print %X/%X -- that seems odd.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers