Обсуждение: 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

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