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

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

Thanks for looking at this.

Em ter., 20 de dez. de 2022 às 21:51, Justin Pryzby <pryzby@telsasoft.com>
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

Em qua., 21 de dez. de 2022 às 04:10, Michael Paquier <michael@paquier.xyz>
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

Em ter., 20 de dez. de 2022 às 21:51, Justin Pryzby <pryzby@telsasoft.com>
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.
5671 else 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 &= 0xffffffff....

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

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 <pryzby@telsasoft.com> 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.
5671 else 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 &= 0xffffffff....

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

On Fri, Dec 23, 2022 at 8:08 AM Justin Pryzby <pryzby@telsasoft.com> 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

On Thu, Jan 12, 2023 at 12:15:24PM +0700, John Naylor wrote:

On Fri, Dec 23, 2022 at 8:08 AM Justin Pryzby <pryzby@telsasoft.com> 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 */

On Thu, Jan 12, 2023 at 12:34 PM Justin Pryzby <pryzby@telsasoft.com> 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 <pryzby@telsasoft.com>

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

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

Em seg., 16 de jan. de 2023 às 03:28, John Naylor <
john.naylor@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

On Mon, Jan 16, 2023 at 1:28 PM John Naylor <john.naylor@enterprisedb.com>
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

Em ter., 17 de jan. de 2023 às 04:37, John Naylor <
john.naylor@enterprisedb.com> escreveu:

On Mon, Jan 16, 2023 at 1:28 PM John Naylor <john.naylor@enterprisedb.com>
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