Horton

Thread

Commits

  1. Remove redundant relkind check

  2. Remove dead code in formatting.c

  3. Fix operator typo in tablecmds.c

  1. Small miscellaneus fixes (Part II)

    Ranier Vilela <ranier.vf@gmail.com> — 2022-11-25T21:27:04Z

     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

  2. Re: Small miscellaneus fixes (Part II)

    Justin Pryzby <pryzby@telsasoft.com> — 2022-12-21T00:51:34Z

    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

  3. Re: Small miscellaneus fixes (Part II)

    Michael Paquier <michael@paquier.xyz> — 2022-12-21T07:10:41Z

    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

  4. Re: Small miscellaneus fixes (Part II)

    Ranier Vilela <ranier.vf@gmail.com> — 2022-12-21T17:51:38Z

    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

  5. Re: Small miscellaneus fixes (Part II)

    Ranier Vilela <ranier.vf@gmail.com> — 2022-12-22T14:35:57Z

    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

  6. Re: Small miscellaneus fixes (Part II)

    Ranier Vilela <ranier.vf@gmail.com> — 2022-12-22T17:29:11Z

    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

  7. Re: Small miscellaneus fixes (Part II)

    Justin Pryzby <pryzby@telsasoft.com> — 2022-12-23T01:08:18Z

    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

  8. Re: Small miscellaneus fixes (Part II)

    John Naylor <john.naylor@enterprisedb.com> — 2023-01-12T05:15:24Z

    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

  9. Re: Small miscellaneus fixes (Part II)

    Justin Pryzby <pryzby@telsasoft.com> — 2023-01-12T05:34:07Z

    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 */

  10. Re: Small miscellaneus fixes (Part II)

    John Naylor <john.naylor@enterprisedb.com> — 2023-01-12T06:44:00Z

    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

  11. Re: Small miscellaneus fixes (Part II)

    John Naylor <john.naylor@enterprisedb.com> — 2023-01-16T06:28:02Z

    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

  12. Re: Small miscellaneus fixes (Part II)

    Ranier Vilela <ranier.vf@gmail.com> — 2023-01-16T11:20:46Z

    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

  13. Re: Small miscellaneus fixes (Part II)

    John Naylor <john.naylor@enterprisedb.com> — 2023-01-17T07:36:56Z

    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

  14. Re: Small miscellaneus fixes (Part II)

    Ranier Vilela <ranier.vf@gmail.com> — 2023-01-17T11:26:11Z

    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