Re: [HACKERS] Duplicated assignment of slot_name in walsender.c

[Alvaro Herrera]

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 Herrerahttp://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

Re: [HACKERS] Duplicated assignment of slot_name in walsender.c

[José Luis Tallón]

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;
charxpos[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

Re: [HACKERS] Duplicated assignment of slot_name in walsender.c

[Euler Taveira]

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
>From 87570993d29f2c98121c3a0a75c85cdc4211f24f Mon Sep 17 00:00:00 2001
From: Euler Taveira 
Date: Wed, 21 Oct 2015 16:52:26 -0300
Subject: [PATCH] Fix a duplicated assignment in walsender code

It seems that the 2nd assignment was an oversight. Spotted by Bernd
Helmle.
---
 src/backend/replication/walsender.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index c6043cd..ca1b4b9 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -834,7 +834,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);
-- 
2.1.4


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

Re: [HACKERS] Duplicated assignment of slot_name in walsender.c

[Alvaro Herrera]

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 Herrerahttp://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

Re: [HACKERS] Duplicated assignment of slot_name in walsender.c

[Andres Freund]

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

Re: [HACKERS] Duplicated assignment of slot_name in walsender.c

[Alvaro Herrera]

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 Herrerahttp://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

Re: [HACKERS] Duplicated assignment of slot_name in walsender.c

[Alvaro Herrera]

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;
charxpos[MAXFNAMELEN];
StringInfoData buf;
+   int len;
 
Assert(!MyReplicationSlot);
 
@@ -791,14 +791,11 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
 
initStringInfo(_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(, 4, 2); /* # of columns */
 
/* slot_name */
-   pq_sendint(, strlen(slot_name), 4); /* col1 len */
-   pq_sendbytes(, slot_name, strlen(slot_name));
+   len = strlen(NameStr(MyReplicationSlot->data.name));
+   pq_sendint(, len, 4);   /* col1 len */
+   pq_sendbytes(, NameStr(MyReplicationSlot->data.name), len);
 
/* consistent wal location */
-   pq_sendint(, strlen(xpos), 4);  /* col2 len */
-   pq_sendbytes(, xpos, strlen(xpos));
+   len = strlen(xpos);
+   pq_sendint(, len, 4);   /* col2 len */
+   pq_sendbytes(, xpos, len);
 
/* snapshot name */
if (snapshot_name != NULL)
{
-   pq_sendint(, strlen(snapshot_name), 4); /* col3 
len */
-   pq_sendbytes(, snapshot_name, strlen(snapshot_name));
+   len = strlen(snapshot_name);
+   pq_sendint(, len, 4);   /* col3 len */
+   pq_sendbytes(, snapshot_name, len);
}
else
pq_sendint(, -1, 4);/* col3 len, NULL */
@@ -904,8 +903,9 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
/* plugin */
if (cmd->plugin != NULL)
{
-   pq_sendint(, strlen(cmd->plugin), 4);   /* col4 
len */
-   pq_sendbytes(, cmd->plugin, strlen(cmd->plugin));
+   len = strlen(cmd->plugin);
+   pq_sendint(, len, 4);   /* col4 len */
+   pq_sendbytes(, cmd->plugin, len);
}
else
pq_sendint(, -1, 4);/* col4 len, NULL */

-- 
Álvaro Herrerahttp://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

Re: [HACKERS] Duplicated assignment of slot_name in walsender.c

[Andres Freund]

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;
>   charxpos[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

Re: [HACKERS] Duplicated assignment of slot_name in walsender.c

[Tom Lane]

Alvaro Herrera  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