Remove some unnecessary if-condition
Hi
I found some likely unnecessary if-condition in code.
1. Some check in else branch seems unnecessary.
In (/src/backend/replication/logical/reorderbuffer.c)
① @@ -4068,7 +4068,7 @@ ReorderBufferToastAppendChunk(ReorderBuffer *rb, ReorderBufferTXN *txn,
bool found;
if (!found)
{
...
}
else if (found && chunk_seq != ent->last_chunk_seq + 1)
...
The check of "found" in else if branch seems unnecessary.
② (/src/backend/utils/init/postinit.c)
@@ -924,11 +924,8 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
bool bootstrap = IsBootstrapProcessingMode();
if (bootstrap)
{
...
}
else if(...)
{...}
else
{
if (!bootstrap)
{
...
}
}
The check of "bootstrap" in else branch seems unnecessary.
2.In (/src/interfaces/ecpg/compatlib/informix.c)
@@ -944,7 +944,7 @@ rupshift(char *str)
for (len--; str[len] && str[len] == ' '; len--);
The first "str[len]" seems unnecessary since " str[len] == ' '" will check it as well.
Do you think we should remove these if-condition for code clean ?
Best regards,
houzj
Attachments:
0001-Remove-some-unnecessary-path.patchapplication/octet-stream; name=0001-Remove-some-unnecessary-path.patchDownload
From 78b5b48ea4270d6b0980fbce63cc4a76db7536c1 Mon Sep 17 00:00:00 2001
From: sherlockcpp <sherlockcpp@foxmail.com>
Date: Thu, 8 Oct 2020 21:11:48 +0800
Subject: [PATCH] Remove some unnecessary path
---
src/backend/replication/logical/reorderbuffer.c | 2 +-
src/backend/utils/init/postinit.c | 7 ++-----
src/interfaces/ecpg/compatlib/informix.c | 2 +-
3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 1975d62..d27f39d 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -4068,7 +4068,7 @@ ReorderBufferToastAppendChunk(ReorderBuffer *rb, ReorderBufferTXN *txn,
elog(ERROR, "got sequence entry %d for toast chunk %u instead of seq 0",
chunk_seq, chunk_id);
}
- else if (found && chunk_seq != ent->last_chunk_seq + 1)
+ else if (chunk_seq != ent->last_chunk_seq + 1)
elog(ERROR, "got sequence entry %d for toast chunk %u instead of seq %d",
chunk_seq, chunk_id, ent->last_chunk_seq + 1);
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index d4ab4c7..a6b1cf2 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -924,11 +924,8 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
* if we are bound to a specific database. We do need to close the
* transaction we started before returning.
*/
- if (!bootstrap)
- {
- pgstat_bestart();
- CommitTransactionCommand();
- }
+ pgstat_bestart();
+ CommitTransactionCommand();
return;
}
diff --git a/src/interfaces/ecpg/compatlib/informix.c b/src/interfaces/ecpg/compatlib/informix.c
index 0bca383..293af0c 100644
--- a/src/interfaces/ecpg/compatlib/informix.c
+++ b/src/interfaces/ecpg/compatlib/informix.c
@@ -944,7 +944,7 @@ rupshift(char *str)
int
byleng(char *str, int len)
{
- for (len--; str[len] && str[len] == ' '; len--);
+ for (len--; str[len] == ' '; len--);
return (len + 1);
}
--
1.8.3.1
On Fri, Oct 9, 2020 at 6:29 AM Hou, Zhijie <houzj.fnst@cn.fujitsu.com> wrote:
Hi
I found some likely unnecessary if-condition in code.
1. Some check in else branch seems unnecessary.
In (/src/backend/replication/logical/reorderbuffer.c)
① @@ -4068,7 +4068,7 @@ ReorderBufferToastAppendChunk(ReorderBuffer *rb, ReorderBufferTXN *txn,bool found;
if (!found)
{
...
}
else if (found && chunk_seq != ent->last_chunk_seq + 1)
...The check of "found" in else if branch seems unnecessary.
② (/src/backend/utils/init/postinit.c)
@@ -924,11 +924,8 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,bool bootstrap = IsBootstrapProcessingMode();
if (bootstrap)
{
...
}
else if(...)
{...}
else
{
if (!bootstrap)
{
...
}
}The check of "bootstrap" in else branch seems unnecessary.
2.In (/src/interfaces/ecpg/compatlib/informix.c)
@@ -944,7 +944,7 @@ rupshift(char *str)for (len--; str[len] && str[len] == ' '; len--);
The first "str[len]" seems unnecessary since " str[len] == ' '" will check it as well.
Do you think we should remove these if-condition for code clean ?
To me it looks good to clean up the conditions as you have done in the
patch. Please add this to commitfest so that it's not forgotten. I
have verified the code and indeed the conditions you are removing are
unnecessary. So the patch can be marked as CFP right away.
--
Best Wishes,
Ashutosh Bapat
To me it looks good to clean up the conditions as you have done in the patch.
Please add this to commitfest so that it's not forgotten. I have verified
the code and indeed the conditions you are removing are unnecessary. So
the patch can be marked as CFP right away.
Thank you for reviewing! added it to commitfest
https://commitfest.postgresql.org/30/2760/
Best regards,
houzj
On Mon, Oct 12, 2020 at 11:42:31AM +0000, Hou, Zhijie wrote:
Thank you for reviewing! added it to commitfest
https://commitfest.postgresql.org/30/2760/
- if (!bootstrap)
- {
- pgstat_bestart();
- CommitTransactionCommand();
- }
+ pgstat_bestart();
+ CommitTransactionCommand();
FWIW, I prefer the original style here. The if/elif dance is quite
long here so it can be easy to miss by reading the code that no
transaction commit should happen in bootstrap mode as this is
conditioned only by the top of the if logic.I would also keep the code in reorderbuffer.c in its original shape,
because it does not actually hurt and changing it could introduce some
back-patching hazard, even if that be a conflict easy to fix.
There may be a point for the bit in informix.c, but similarly when you
think about back-patching I'd just keep it as it is.
--
Michael