Re: Small miscellaneus fixes (Part II)
Em ter., 17 de jan. de 2023 às 04:37, John Naylor < john.nay...@enterprisedb.com> escreveu: > > On Mon, Jan 16, 2023 at 1:28 PM John Naylor > wrote: > > > > > > I wrote: > > > ...but arguably the earlier check is close enough that it's silly to > assert in the "else" branch, and I'd be okay with just nuking those lines. > Another thing that caught my attention is the assumption that unsetting a > bit is so expensive that we have to first check if it's set, so we may as > well remove "IS_BRACKET(Np->Num)" as well. > > > > The attached is what I mean. I'll commit this this week unless there are > objections. > > I've pushed this and the cosmetic fix in pg_dump. Those were the only > things I saw that had some interest, so I closed the CF entry. > Thanks for the commits. regards, Ranier Vilela
Re: Small miscellaneus fixes (Part II)
On Mon, Jan 16, 2023 at 1:28 PM John Naylor wrote: > > > I wrote: > > ...but arguably the earlier check is close enough that it's silly to assert in the "else" branch, and I'd be okay with just nuking those lines. Another thing that caught my attention is the assumption that unsetting a bit is so expensive that we have to first check if it's set, so we may as well remove "IS_BRACKET(Np->Num)" as well. > > The attached is what I mean. I'll commit this this week unless there are objections. I've pushed this and the cosmetic fix in pg_dump. Those were the only things I saw that had some interest, so I closed the CF entry. -- John Naylor EDB: http://www.enterprisedb.com
Re: Small miscellaneus fixes (Part II)
Em seg., 16 de jan. de 2023 às 03:28, John Naylor < john.nay...@enterprisedb.com> escreveu: > > I wrote: > > ...but arguably the earlier check is close enough that it's silly to > assert in the "else" branch, and I'd be okay with just nuking those lines. > Another thing that caught my attention is the assumption that unsetting a > bit is so expensive that we have to first check if it's set, so we may as > well remove "IS_BRACKET(Np->Num)" as well. > > The attached is what I mean. I'll commit this this week unless there are > objections. > +1 looks good to me. regards, Ranier Vilela
Re: Small miscellaneus fixes (Part II)
I wrote:
> ...but arguably the earlier check is close enough that it's silly to
assert in the "else" branch, and I'd be okay with just nuking those lines.
Another thing that caught my attention is the assumption that unsetting a
bit is so expensive that we have to first check if it's set, so we may as
well remove "IS_BRACKET(Np->Num)" as well.
The attached is what I mean. I'll commit this this week unless there are
objections.
--
John Naylor
EDB: http://www.enterprisedb.com
From 5dd409e6059be464ad635ce8cd885fc3de5d8e43 Mon Sep 17 00:00:00 2001
From: John Naylor
Date: Mon, 16 Jan 2023 13:03:22 +0700
Subject: [PATCH v4] Remove dead code in formatting.c
Remove some code guarded by IS_MINUS() or IS_PLUS(), where the entire
stanza is inside an else-block where both of these are false. This
should slightly improve test coverage.
While at it, remove coding that apparently assumes that unsetting a
bit is so expensive that we have to first check if it's already set
in the first place.
Reviewed by Justin Pryzby
Per Coverity report from Ranier Vilela
Discussion: https://www.postgresql.org/message-id/20221223010818.GP1153%40telsasoft.com
---
src/backend/utils/adt/formatting.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index a4b524ea3a..f3f4db5ef6 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -5664,13 +5664,9 @@ NUM_processor(FormatNode *node, NUMDesc *Num, char *inout,
{
if (Np->sign != '-')
{
-if (IS_BRACKET(Np->Num) && IS_FILLMODE(Np->Num))
+if (IS_FILLMODE(Np->Num))
Np->Num->flag &= ~NUM_F_BRACKET;
-if (IS_MINUS(Np->Num))
- Np->Num->flag &= ~NUM_F_MINUS;
}
- else if (Np->sign != '+' && IS_PLUS(Np->Num))
-Np->Num->flag &= ~NUM_F_PLUS;
if (Np->sign == '+' && IS_FILLMODE(Np->Num) && IS_LSIGN(Np->Num) == false)
Np->sign_wrote = true; /* needn't sign */
--
2.39.0
Re: Small miscellaneus fixes (Part II)
On Thu, Jan 12, 2023 at 12:34 PM Justin Pryzby wrote:
>
> On Thu, Jan 12, 2023 at 12:15:24PM +0700, John Naylor wrote:
> > On Fri, Dec 23, 2022 at 8:08 AM Justin Pryzby
wrote:
> >
> > > Makes sense now (in your first message, you said that the problem was
> > > with "sign", and the patch didn't address the actual problem in
> > > IS_PLUS()).
> > >
> > > One can look and find that the unreachable code was introduced at
> > > 7a3e7b64a.
> > >
> > > With your proposed change, the unreachable line is hit by regression
> > > tests, which is an improvment. As is the change to pg_dump.c.
> >
> > But that now reachable line just unsets a flag that we previously found
> > unset, right?
>
> Good point.
>
> > And if that line was unreachable, then surely the previous flag-clearing
> > operation is too?
> >
> > 5669 994426 : if (IS_MINUS(Np->Num)) // <- also
always
> > false
> > 5670 0 : Np->Num->flag &= ~NUM_F_MINUS;
> > 5671 : }
> > 5672 524 : else if (Np->sign != '+' &&
IS_PLUS(Np->Num))
> > 5673 0 : Np->Num->flag &= ~NUM_F_PLUS;
> >
> >
https://coverage.postgresql.org/src/backend/utils/adt/formatting.c.gcov.html
> >
> > I'm inclined to turn the dead unsets into asserts.
>
> To be clear - did you mean like this ?
>
> diff --git a/src/backend/utils/adt/formatting.c
b/src/backend/utils/adt/formatting.c
> index a4b524ea3ac..848956879f5 100644
> --- a/src/backend/utils/adt/formatting.c
> +++ b/src/backend/utils/adt/formatting.c
> @@ -5662,15 +5662,13 @@ NUM_processor(FormatNode *node, NUMDesc *Num,
char *inout,
> }
> else
> {
> + Assert(!IS_MINUS(Np->Num));
> + Assert(!IS_PLUS(Np->Num));
> if (Np->sign != '-')
> {
> if (IS_BRACKET(Np->Num) &&
IS_FILLMODE(Np->Num))
> Np->Num->flag &= ~NUM_F_BRACKET;
> - if (IS_MINUS(Np->Num))
> - Np->Num->flag &= ~NUM_F_MINUS;
> }
> - else if (Np->sign != '+' && IS_PLUS(Np->Num))
> - Np->Num->flag &= ~NUM_F_PLUS;
>
> if (Np->sign == '+' && IS_FILLMODE(Np->Num) &&
IS_LSIGN(Np->Num) == false)
> Np->sign_wrote = true; /* needn't sign */
I was originally thinking of something more localized:
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -5666,11 +5666,10 @@ NUM_processor(FormatNode *node, NUMDesc *Num, char
*inout,
{
if (IS_BRACKET(Np->Num) && IS_FILLMODE(Np->Num))
Np->Num->flag &= ~NUM_F_BRACKET;
-if (IS_MINUS(Np->Num))
-Np->Num->flag &= ~NUM_F_MINUS;
+Assert(!IS_MINUS(Np->Num));
}
-else if (Np->sign != '+' && IS_PLUS(Np->Num))
-Np->Num->flag &= ~NUM_F_PLUS;
+else if (Np->sign != '+')
+Assert(!IS_PLUS(Np->Num));
...but arguably the earlier check is close enough that it's silly to assert
in the "else" branch, and I'd be okay with just nuking those lines. Another
thing that caught my attention is the assumption that unsetting a bit is so
expensive that we have to first check if it's set, so we may as well remove
"IS_BRACKET(Np->Num)" as well.
--
John Naylor
EDB: http://www.enterprisedb.com
Re: Small miscellaneus fixes (Part II)
On Thu, Jan 12, 2023 at 12:15:24PM +0700, John Naylor wrote:
> On Fri, Dec 23, 2022 at 8:08 AM Justin Pryzby wrote:
>
> > Makes sense now (in your first message, you said that the problem was
> > with "sign", and the patch didn't address the actual problem in
> > IS_PLUS()).
> >
> > One can look and find that the unreachable code was introduced at
> > 7a3e7b64a.
> >
> > With your proposed change, the unreachable line is hit by regression
> > tests, which is an improvment. As is the change to pg_dump.c.
>
> But that now reachable line just unsets a flag that we previously found
> unset, right?
Good point.
> And if that line was unreachable, then surely the previous flag-clearing
> operation is too?
>
> 5669 994426 : if (IS_MINUS(Np->Num)) // <- also always
> false
> 5670 0 : Np->Num->flag &= ~NUM_F_MINUS;
> 5671 : }
> 5672 524 : else if (Np->sign != '+' && IS_PLUS(Np->Num))
> 5673 0 : Np->Num->flag &= ~NUM_F_PLUS;
>
> https://coverage.postgresql.org/src/backend/utils/adt/formatting.c.gcov.html
>
> I'm inclined to turn the dead unsets into asserts.
To be clear - did you mean like this ?
diff --git a/src/backend/utils/adt/formatting.c
b/src/backend/utils/adt/formatting.c
index a4b524ea3ac..848956879f5 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -5662,15 +5662,13 @@ NUM_processor(FormatNode *node, NUMDesc *Num, char
*inout,
}
else
{
+ Assert(!IS_MINUS(Np->Num));
+ Assert(!IS_PLUS(Np->Num));
if (Np->sign != '-')
{
if (IS_BRACKET(Np->Num) && IS_FILLMODE(Np->Num))
Np->Num->flag &= ~NUM_F_BRACKET;
- if (IS_MINUS(Np->Num))
- Np->Num->flag &= ~NUM_F_MINUS;
}
- else if (Np->sign != '+' && IS_PLUS(Np->Num))
- Np->Num->flag &= ~NUM_F_PLUS;
if (Np->sign == '+' && IS_FILLMODE(Np->Num) &&
IS_LSIGN(Np->Num) == false)
Np->sign_wrote = true; /* needn't sign */
Re: Small miscellaneus fixes (Part II)
On Fri, Dec 23, 2022 at 8:08 AM Justin Pryzby wrote: > Makes sense now (in your first message, you said that the problem was > with "sign", and the patch didn't address the actual problem in > IS_PLUS()). > > One can look and find that the unreachable code was introduced at > 7a3e7b64a. > > With your proposed change, the unreachable line is hit by regression > tests, which is an improvment. As is the change to pg_dump.c. But that now reachable line just unsets a flag that we previously found unset, right? And if that line was unreachable, then surely the previous flag-clearing operation is too? 5669 994426 : if (IS_MINUS(Np->Num)) // <- also always false 5670 0 : Np->Num->flag &= ~NUM_F_MINUS; 5671 : } 5672 524 : else if (Np->sign != '+' && IS_PLUS(Np->Num)) 5673 0 : Np->Num->flag &= ~NUM_F_PLUS; https://coverage.postgresql.org/src/backend/utils/adt/formatting.c.gcov.html I'm inclined to turn the dead unsets into asserts. -- John Naylor EDB: http://www.enterprisedb.com
Re: Small miscellaneus fixes (Part II)
On Thu, Dec 22, 2022 at 02:29:11PM -0300, Ranier Vilela wrote: > Em ter., 20 de dez. de 2022 às 21:51, Justin Pryzby > escreveu: > > > On Fri, Nov 25, 2022 at 06:27:04PM -0300, Ranier Vilela wrote: > > > 5. Use boolean operator with boolean operands > > > (b/src/backend/commands/tablecmds.c) > > > > tablecmds.c: right. Since 074c5cfbf > > > > pg_dump.c: right. Since b08dee24a > > > > > 4. Fix dead code (src/backend/utils/adt/formatting.c) > > > Np->sign == '+', is different than "!= '-'" and is different than "!= > > '+'" > > > So the else is never hit. > > > > formatting.c: I don't see the problem. > > > > if (Np->sign != '-') > > ... > > else if (Np->sign != '+' && IS_PLUS(Np->Num)) > > ... > > > > You said that the "else" is never hit, but why ? > > This is a Coverity report. > > dead_error_condition: The condition Np->Num->flag & 0x200 cannot be true. > 5671else if (Np->sign != '+' && IS_PLUS(Np->Num)) > > CID 1501076 (#1 of 1): Logically dead code (DEADCODE)dead_error_line: > Execution > cannot reach this statement: Np->Num->flag &= 0x > > So, the dead code is because IS_PUS(Np->Num) is already tested and cannot > be true on else. Makes sense now (in your first message, you said that the problem was with "sign", and the patch didn't address the actual problem in IS_PLUS()). One can look and find that the unreachable code was introduced at 7a3e7b64a. With your proposed change, the unreachable line is hit by regression tests, which is an improvment. As is the change to pg_dump.c. -- Justin
Re: Small miscellaneus fixes (Part II)
Em ter., 20 de dez. de 2022 às 21:51, Justin Pryzby escreveu: > On Fri, Nov 25, 2022 at 06:27:04PM -0300, Ranier Vilela wrote: > > 5. Use boolean operator with boolean operands > > (b/src/backend/commands/tablecmds.c) > > tablecmds.c: right. Since 074c5cfbf > > pg_dump.c: right. Since b08dee24a > > > 4. Fix dead code (src/backend/utils/adt/formatting.c) > > Np->sign == '+', is different than "!= '-'" and is different than "!= > '+'" > > So the else is never hit. > > formatting.c: I don't see the problem. > > if (Np->sign != '-') > ... > else if (Np->sign != '+' && IS_PLUS(Np->Num)) > ... > > You said that the "else" is never hit, but why ? > This is a Coverity report. dead_error_condition: The condition Np->Num->flag & 0x200 cannot be true. 5671else if (Np->sign != '+' && IS_PLUS(Np->Num)) CID 1501076 (#1 of 1): Logically dead code (DEADCODE)dead_error_line: Execution cannot reach this statement: Np->Num->flag &= 0x So, the dead code is because IS_PUS(Np->Num) is already tested and cannot be true on else. v1 patch attached. regards, Ranier Vilela v1-fix_dead_code_formatting.patch Description: Binary data
Re: Small miscellaneus fixes (Part II)
Em qua., 21 de dez. de 2022 às 04:10, Michael Paquier escreveu: > On Tue, Dec 20, 2022 at 06:51:34PM -0600, Justin Pryzby wrote: > > On Fri, Nov 25, 2022 at 06:27:04PM -0300, Ranier Vilela wrote: > > > 5. Use boolean operator with boolean operands > > > (b/src/backend/commands/tablecmds.c) > > > > tablecmds.c: right. Since 074c5cfbf > > Most of this does not seem to be really worth poking at. > > newcons = AddRelationNewConstraints(rel, NIL, > list_make1(copyObject(constr)), > - recursing | is_readd, /* > allow_merge */ > + recursing || is_readd, /* > allow_merge */ > There is no damage here, but that looks like a typo so no objections > on this one. > Thanks Michael, for the commit. regards, Ranier Vilela
Re: Small miscellaneus fixes (Part II)
Thanks for looking at this. Em ter., 20 de dez. de 2022 às 21:51, Justin Pryzby escreveu: > On Fri, Nov 25, 2022 at 06:27:04PM -0300, Ranier Vilela wrote: > > 5. Use boolean operator with boolean operands > > (b/src/backend/commands/tablecmds.c) > > tablecmds.c: right. Since 074c5cfbf > > pg_dump.c: right. Since b08dee24a > > > 4. Fix dead code (src/backend/utils/adt/formatting.c) > > Np->sign == '+', is different than "!= '-'" and is different than "!= > '+'" > > So the else is never hit. > > formatting.c: I don't see the problem. > > if (Np->sign != '-') > ... > else if (Np->sign != '+' && IS_PLUS(Np->Num)) > ... > > You said that the "else" is never hit, but why ? > Maybe this part of the patch is wrong. The only case for the first if not handled is sign == '-', sign == '-' is handled by else. So always the "else is true", because sign == '+' is handled by the first if. regards, Ranier Vilela
Re: Small miscellaneus fixes (Part II)
On Tue, Dec 20, 2022 at 06:51:34PM -0600, Justin Pryzby wrote: > On Fri, Nov 25, 2022 at 06:27:04PM -0300, Ranier Vilela wrote: > > 5. Use boolean operator with boolean operands > > (b/src/backend/commands/tablecmds.c) > > tablecmds.c: right. Since 074c5cfbf Most of this does not seem to be really worth poking at. newcons = AddRelationNewConstraints(rel, NIL, list_make1(copyObject(constr)), - recursing | is_readd, /* allow_merge */ + recursing || is_readd, /* allow_merge */ There is no damage here, but that looks like a typo so no objections on this one. -- Michael signature.asc Description: PGP signature
Re: Small miscellaneus fixes (Part II)
On Fri, Nov 25, 2022 at 06:27:04PM -0300, Ranier Vilela wrote: > 5. Use boolean operator with boolean operands > (b/src/backend/commands/tablecmds.c) tablecmds.c: right. Since 074c5cfbf pg_dump.c: right. Since b08dee24a > 4. Fix dead code (src/backend/utils/adt/formatting.c) > Np->sign == '+', is different than "!= '-'" and is different than "!= '+'" > So the else is never hit. formatting.c: I don't see the problem. if (Np->sign != '-') ... else if (Np->sign != '+' && IS_PLUS(Np->Num)) ... You said that the "else" is never hit, but why ? What if sign is 0 ? -- Justin
Small miscellaneus fixes (Part II)
Hi. There another assorted fixes to the head branch. 1. Avoid useless pointer increment (src/backend/utils/activity/pgstat_shmem.c) The pointer *p, is used in creation dsa memory, not p + MAXALIGN(pgstat_dsa_init_size()). 2. Discard result unused (src/backend/access/transam/xlogrecovery.c) Some compilers raise warnings about discarding return from strtoul. 3. Fix dead code (src/bin/pg_dump/pg_dump.c) tbinfo->relkind == RELKIND_MATVIEW is always true, so "INDEX" is never hit. Per Coverity. 4. Fix dead code (src/backend/utils/adt/formatting.c) Np->sign == '+', is different than "!= '-'" and is different than "!= '+'" So the else is never hit. Per Coverity. 5. Use boolean operator with boolean operands (b/src/backend/commands/tablecmds.c) regards, Ranier Vilela avoid_useless_pointer_increment.patch Description: Binary data discard_strtoul_unused_result.patch Description: Binary data fix_dead_code_pg_dump.patch Description: Binary data fix_dead_code_formatting.patch Description: Binary data use_boolean_operator_with_boolean_operands.patch Description: Binary data
