Thread: Duplicated assignment of slot_name in walsender.c
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
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
Attachment
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
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
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
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
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
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
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.
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