Обсуждение: Duplicated assignment of slot_name in walsender.c

Поиск
Список
Период
Сортировка

Duplicated assignment of slot_name in walsender.c

От
Bernd Helmle
Дата:
walsender.c, CreateReplicationSlot() currently has this:
       slot_name = NameStr(MyReplicationSlot->data.name);
       if (cmd->kind == REPLICATION_KIND_LOGICAL)       {[...]       }       else if (cmd->kind ==
REPLICATION_KIND_PHYSICAL&& cmd->reserve_wal)       {[...]       }
 
       slot_name = NameStr(MyReplicationSlot->data.name);

The 2nd assignment to slot_name looks unnecessary?

-- 
Thanks
Bernd



Re: 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

Вложения

Re: 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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: 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



Re: 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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: 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 reqstatic
voidCreateReplicationSlot(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



Re: 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;
>      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



Re: Duplicated assignment of slot_name in walsender.c

От
Tom Lane
Дата:
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



Re: 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;
>       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.




Re: 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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services