Horton

Thread

Commits

  1. Yet more elimination of dead stores and useless initializations.

  2. Remove still more useless assignments.

  3. Remove some more useless assignments.

  1. [PATCH] Redudant initilization

    Ranier Vilela <ranier.vf@gmail.com> — 2020-03-28T22:04:00Z

    Hi,
This patch fixes some redundant initilization, that are safe to remove.

best regards,
Ranier Vilela

  2. Re: [PATCH] Redudant initilization

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-03-30T00:57:01Z

    Hello.

At Sat, 28 Mar 2020 19:04:00 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in 
> Hi,
> This patch fixes some redundant initilization, that are safe to remove.

> diff --git a/src/backend/access/gist/gistxlog.c b/src/backend/access/gist/gistxlog.c
> index d3f3a7b803..ffaa2b1ab4 100644
> --- a/src/backend/access/gist/gistxlog.c
> +++ b/src/backend/access/gist/gistxlog.c
> @@ -396,7 +396,7 @@ gistRedoPageReuse(XLogReaderState *record)
>  	if (InHotStandby)
>  	{
>  		FullTransactionId latestRemovedFullXid = xlrec->latestRemovedFullXid;
> -		FullTransactionId nextFullXid = ReadNextFullTransactionId();
> +		FullTransactionId nextFullXid;

I'd prefer to preserve this and remove the latter.

> diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
> index 9d9e915979..795cf349eb 100644
> --- a/src/backend/catalog/heap.c
> +++ b/src/backend/catalog/heap.c
> @@ -3396,7 +3396,7 @@ List *
>  heap_truncate_find_FKs(List *relationIds)
>  {
>  	List	   *result = NIL;
> -	List	   *oids = list_copy(relationIds);
> +	List	   *oids;

This was just a waste of memory, the fix looks fine.

> diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
> index c5b771c531..37fbeef841 100644
> --- a/src/backend/storage/smgr/md.c
> +++ b/src/backend/storage/smgr/md.c
> @@ -730,9 +730,11 @@ mdwrite(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
>  BlockNumber
>  mdnblocks(SMgrRelation reln, ForkNumber forknum)
>  {
> -	MdfdVec    *v = mdopenfork(reln, forknum, EXTENSION_FAIL);
> +	MdfdVec    *v;
>  	BlockNumber nblocks;
> -	BlockNumber segno = 0;
> +	BlockNumber segno;
> +
> +    mdopenfork(reln, forknum, EXTENSION_FAIL);
>  
>  	/* mdopen has opened the first segment */
>  	Assert(reln->md_num_open_segs[forknum] > 0);

It doesn't seems *to me* an issue.

> diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
> index 8bb00abb6b..7a6a2ecbe9 100644
> --- a/src/backend/utils/adt/json.c
> +++ b/src/backend/utils/adt/json.c
> @@ -990,7 +990,7 @@ catenate_stringinfo_string(StringInfo buffer, const char *addon)
>  Datum
>  json_build_object(PG_FUNCTION_ARGS)
>  {
> -	int			nargs = PG_NARGS();
> +	int			nargs;

This part looks fine.

>  	int			i;
>  	const char *sep = "";
>  	StringInfo	result;
> @@ -998,6 +998,8 @@ json_build_object(PG_FUNCTION_ARGS)
>  	bool	   *nulls;
>  	Oid		   *types;
>  
> +    PG_NARGS();
> +
>  	/* fetch argument values to build the object */
>  	nargs = extract_variadic_args(fcinfo, 0, false, &args, &types, &nulls);

PG_NARGS() doesn't have a side-effect so no need to call independently.


> diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
> index 9e24fec72d..fb0e833b2d 100644
> --- a/src/backend/utils/mmgr/mcxt.c
> +++ b/src/backend/utils/mmgr/mcxt.c
> @@ -475,7 +475,7 @@ MemoryContextMemAllocated(MemoryContext context, bool recurse)
>  
>  	if (recurse)
>  	{
> -		MemoryContext child = context->firstchild;
> +		MemoryContext child;
>  
>  		for (child = context->firstchild;
>  			 child != NULL;

This looks fine.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  3. Re: [PATCH] Redudant initilization

    Ranier Vilela <ranier.vf@gmail.com> — 2020-03-30T12:08:59Z

    Em dom., 29 de mar. de 2020 às 21:57, Kyotaro Horiguchi <
horikyota.ntt@gmail.com> escreveu:

> Hello.
>
> At Sat, 28 Mar 2020 19:04:00 -0300, Ranier Vilela <ranier.vf@gmail.com>
> wrote in
> > Hi,
> > This patch fixes some redundant initilization, that are safe to remove.
>
> > diff --git a/src/backend/access/gist/gistxlog.c
> b/src/backend/access/gist/gistxlog.c
> > index d3f3a7b803..ffaa2b1ab4 100644
> > --- a/src/backend/access/gist/gistxlog.c
> > +++ b/src/backend/access/gist/gistxlog.c
> > @@ -396,7 +396,7 @@ gistRedoPageReuse(XLogReaderState *record)
> >       if (InHotStandby)
> >       {
> >               FullTransactionId latestRemovedFullXid =
> xlrec->latestRemovedFullXid;
> > -             FullTransactionId nextFullXid =
> ReadNextFullTransactionId();
> > +             FullTransactionId nextFullXid;
>
> I'd prefer to preserve this and remove the latter.
>

Ok.


> > diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
> > index 9d9e915979..795cf349eb 100644
> > --- a/src/backend/catalog/heap.c
> > +++ b/src/backend/catalog/heap.c
> > @@ -3396,7 +3396,7 @@ List *
> >  heap_truncate_find_FKs(List *relationIds)
> >  {
> >       List       *result = NIL;
> > -     List       *oids = list_copy(relationIds);
> > +     List       *oids;
>
> This was just a waste of memory, the fix looks fine.
>
> > diff --git a/src/backend/storage/smgr/md.c
> b/src/backend/storage/smgr/md.c
> > index c5b771c531..37fbeef841 100644
> > --- a/src/backend/storage/smgr/md.c
> > +++ b/src/backend/storage/smgr/md.c
> > @@ -730,9 +730,11 @@ mdwrite(SMgrRelation reln, ForkNumber forknum,
> BlockNumber blocknum,
> >  BlockNumber
> >  mdnblocks(SMgrRelation reln, ForkNumber forknum)
> >  {
> > -     MdfdVec    *v = mdopenfork(reln, forknum, EXTENSION_FAIL);
> > +     MdfdVec    *v;
> >       BlockNumber nblocks;
> > -     BlockNumber segno = 0;
> > +     BlockNumber segno;
> > +
> > +    mdopenfork(reln, forknum, EXTENSION_FAIL);
> >
> >       /* mdopen has opened the first segment */
> >       Assert(reln->md_num_open_segs[forknum] > 0);
>
> It doesn't seems *to me* an issue.
>

Not a big deal, but the assignment of the variable v here is a small waste,
since it is again highlighted right after.


> > diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
> > index 8bb00abb6b..7a6a2ecbe9 100644
> > --- a/src/backend/utils/adt/json.c
> > +++ b/src/backend/utils/adt/json.c
> > @@ -990,7 +990,7 @@ catenate_stringinfo_string(StringInfo buffer, const
> char *addon)
> >  Datum
> >  json_build_object(PG_FUNCTION_ARGS)
> >  {
> > -     int                     nargs = PG_NARGS();
> > +     int                     nargs;
>
> This part looks fine.
>
> >       int                     i;
> >       const char *sep = "";
> >       StringInfo      result;
> > @@ -998,6 +998,8 @@ json_build_object(PG_FUNCTION_ARGS)
> >       bool       *nulls;
> >       Oid                *types;
> >
> > +    PG_NARGS();
> > +
> >       /* fetch argument values to build the object */
> >       nargs = extract_variadic_args(fcinfo, 0, false, &args, &types,
> &nulls);
>
> PG_NARGS() doesn't have a side-effect so no need to call independently.
>
 Sorry, does that mean we can remove it completely?


>
> > diff --git a/src/backend/utils/mmgr/mcxt.c
> b/src/backend/utils/mmgr/mcxt.c
> > index 9e24fec72d..fb0e833b2d 100644
> > --- a/src/backend/utils/mmgr/mcxt.c
> > +++ b/src/backend/utils/mmgr/mcxt.c
> > @@ -475,7 +475,7 @@ MemoryContextMemAllocated(MemoryContext context,
> bool recurse)
> >
> >       if (recurse)
> >       {
> > -             MemoryContext child = context->firstchild;
> > +             MemoryContext child;
> >
> >               for (child = context->firstchild;
> >                        child != NULL;
>
> This looks fine.
>

Thank you for the review and consideration.

best regards,
Ranier Vilela

  4. Re: [PATCH] Redudant initilization

    Ranier Vilela <ranier.vf@gmail.com> — 2020-04-01T11:57:18Z

    Hi,
New patch with yours suggestions.

best regards,
Ranier Vilela

  5. Re: [PATCH] Redudant initilization

    Bruce Momjian <bruce@momjian.us> — 2020-09-04T02:57:51Z

    On Wed, Apr  1, 2020 at 08:57:18AM -0300, Ranier Vilela wrote:
> Hi,
> New patch with yours suggestions.

Patch applied to head, thanks.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

  6. Re: [PATCH] Redudant initilization

    Ranier Vilela <ranier.vf@gmail.com> — 2020-09-04T12:39:45Z

    Em qui., 3 de set. de 2020 às 23:57, Bruce Momjian <bruce@momjian.us>
escreveu:

> On Wed, Apr  1, 2020 at 08:57:18AM -0300, Ranier Vilela wrote:
> > Hi,
> > New patch with yours suggestions.
>
> Patch applied to head, thanks.
>
Thank you Bruce.

regards,
Ranier Vilela

  7. Re: [PATCH] Redudant initilization

    Bruce Momjian <bruce@momjian.us> — 2020-09-04T14:01:23Z

    On Fri, Sep  4, 2020 at 09:39:45AM -0300, Ranier Vilela wrote:
> Em qui., 3 de set. de 2020 às 23:57, Bruce Momjian <bruce@momjian.us> escreveu:
> 
>     On Wed, Apr  1, 2020 at 08:57:18AM -0300, Ranier Vilela wrote:
>     > Hi,
>     > New patch with yours suggestions.
> 
>     Patch applied to head, thanks.
> 
> Thank you Bruce.

I have to say, I am kind of stumped why compilers do not warn of such
cases, and why we haven't gotten reports about these cases before.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

  8. Re: [PATCH] Redudant initilization

    Ranier Vilela <ranier.vf@gmail.com> — 2020-09-04T16:55:52Z

    Em sex., 4 de set. de 2020 às 11:01, Bruce Momjian <bruce@momjian.us>
escreveu:

> On Fri, Sep  4, 2020 at 09:39:45AM -0300, Ranier Vilela wrote:
> > Em qui., 3 de set. de 2020 às 23:57, Bruce Momjian <bruce@momjian.us>
> escreveu:
> >
> >     On Wed, Apr  1, 2020 at 08:57:18AM -0300, Ranier Vilela wrote:
> >     > Hi,
> >     > New patch with yours suggestions.
> >
> >     Patch applied to head, thanks.
> >
> > Thank you Bruce.
>
> I have to say, I am kind of stumped why compilers do not warn of such
> cases, and why we haven't gotten reports about these cases before.
>
I believe it is because, syntactically, there is no error.

I would like to thank Kyotaro Horiguchi,
my thanks for your review.

regards,
Ranier Vilela

  9. Re: [PATCH] Redudant initilization

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-04T17:40:26Z

    Bruce Momjian <bruce@momjian.us> writes:
> I have to say, I am kind of stumped why compilers do not warn of such
> cases, and why we haven't gotten reports about these cases before.

I was just experimenting with clang's "scan-build" tool.  It finds
all of the cases you just fixed, and several dozen more beside.
Quite a few are things that, as a matter of style, we should *not*
change, for instance

rewriteHandler.c:2807:5: warning: Value stored to 'outer_reloids' is never read
                                outer_reloids = list_delete_last(outer_reloids);
                                ^               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Failing to update the list pointer here would just be asking for bugs.
However, I see some that look like genuine oversights; will go fix.

(I'm not sure how much I trust scan-build overall.  It produces a
whole bunch of complaints about null pointer dereferences, for instance.
If those aren't 99% false positives, we'd be crashing constantly.
It's also dog-slow.  But it might be something to try occasionally.)

			regards, tom lane

  10. Re: [PATCH] Redudant initilization

    Ranier Vilela <ranier.vf@gmail.com> — 2020-09-04T21:20:01Z

    Em sex., 4 de set. de 2020 às 14:40, Tom Lane <tgl@sss.pgh.pa.us> escreveu:

> Bruce Momjian <bruce@momjian.us> writes:
> > I have to say, I am kind of stumped why compilers do not warn of such
> > cases, and why we haven't gotten reports about these cases before.
>
> I was just experimenting with clang's "scan-build" tool.  It finds
> all of the cases you just fixed, and several dozen more beside.
> Quite a few are things that, as a matter of style, we should *not*
> change, for instance
>
> rewriteHandler.c:2807:5: warning: Value stored to 'outer_reloids' is never
> read
>                                 outer_reloids =
> list_delete_last(outer_reloids);
>                                 ^
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
There are some like this, in the analyzes that I did.
Even when it is the last action of the function.


> Failing to update the list pointer here would just be asking for bugs.
> However, I see some that look like genuine oversights; will go fix.
>
Thanks for fixing this.


> (I'm not sure how much I trust scan-build overall.  It produces a
> whole bunch of complaints about null pointer dereferences, for instance.
> If those aren't 99% false positives, we'd be crashing constantly.
> It's also dog-slow.  But it might be something to try occasionally.)
>
I believe it would be very beneficial.

Attached is a patch I made in March/2020, but due to problems,
it was sent but did not make the list.
Would you mind taking a look?
Certainly, if accepted, rebasing would have to be done.

regards,
Ranier Vilela

  11. Re: [PATCH] Redudant initilization

    Ranier Vilela <ranier.vf@gmail.com> — 2020-09-05T16:40:37Z

    Em sex., 4 de set. de 2020 às 18:20, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

> Em sex., 4 de set. de 2020 às 14:40, Tom Lane <tgl@sss.pgh.pa.us>
> escreveu:
>
>> Bruce Momjian <bruce@momjian.us> writes:
>> > I have to say, I am kind of stumped why compilers do not warn of such
>> > cases, and why we haven't gotten reports about these cases before.
>>
>> I was just experimenting with clang's "scan-build" tool.  It finds
>> all of the cases you just fixed, and several dozen more beside.
>> Quite a few are things that, as a matter of style, we should *not*
>> change, for instance
>>
>> rewriteHandler.c:2807:5: warning: Value stored to 'outer_reloids' is
>> never read
>>                                 outer_reloids =
>> list_delete_last(outer_reloids);
>>                                 ^
>>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
> There are some like this, in the analyzes that I did.
> Even when it is the last action of the function.
>
>
>> Failing to update the list pointer here would just be asking for bugs.
>> However, I see some that look like genuine oversights; will go fix.
>>
> Thanks for fixing this.
>
>
>> (I'm not sure how much I trust scan-build overall.  It produces a
>> whole bunch of complaints about null pointer dereferences, for instance.
>> If those aren't 99% false positives, we'd be crashing constantly.
>> It's also dog-slow.  But it might be something to try occasionally.)
>>
> I believe it would be very beneficial.
>
> Attached is a patch I made in March/2020, but due to problems,
> it was sent but did not make the list.
> Would you mind taking a look?
> Certainly, if accepted, rebasing would have to be done.
>
Here it is simplified, splitted and rebased.
Some are bogus, others are interesting.

regards,
Ranier Vilela

  12. Re: [PATCH] Redudant initilization

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-05T17:29:08Z

    Ranier Vilela <ranier.vf@gmail.com> writes:
> Attached is a patch I made in March/2020, but due to problems,
> it was sent but did not make the list.
> Would you mind taking a look?

I applied some of this, but other parts had been overtaken by
events, and there were other changes that I didn't agree with.

A general comment on the sort of "dead store" that I don't think
we should remove is where a function is trying to maintain an
internal invariant, such as "this pointer points past the last
data written to a buffer" or "these two variables are in sync".
If the update happens to be the last one in the function, the
compiler may be able to see that the store is dead ... but IMO
it should just optimize such a store away and not get in the
programmer's face about it.  If we manually remove the dead
store then what we've done is broken the invariant, and we'll
pay for that in future bugs and maintenance costs.  Somebody
may someday want to add more code after the step in question,
and if they fail to undo the manual optimization then they've
got a bug.  Besides which, it's confusing when a function
does something the same way N-1 times and then differently the
N'th time.

			regards, tom lane

  13. Re: [PATCH] Redudant initilization

    Ranier Vilela <ranier.vf@gmail.com> — 2020-09-05T17:44:20Z

    Em sáb., 5 de set. de 2020 às 14:29, Tom Lane <tgl@sss.pgh.pa.us> escreveu:

> Ranier Vilela <ranier.vf@gmail.com> writes:
> > Attached is a patch I made in March/2020, but due to problems,
> > it was sent but did not make the list.
> > Would you mind taking a look?
>
> I applied some of this, but other parts had been overtaken by
> events, and there were other changes that I didn't agree with.
>
I fully agree with your judgment.


> A general comment on the sort of "dead store" that I don't think
> we should remove is where a function is trying to maintain an
> internal invariant, such as "this pointer points past the last
> data written to a buffer" or "these two variables are in sync".
> If the update happens to be the last one in the function, the
> compiler may be able to see that the store is dead ... but IMO
> it should just optimize such a store away and not get in the
> programmer's face about it.  If we manually remove the dead
> store then what we've done is broken the invariant, and we'll
> pay for that in future bugs and maintenance costs.  Somebody
> may someday want to add more code after the step in question,
> and if they fail to undo the manual optimization then they've
> got a bug.  Besides which, it's confusing when a function
> does something the same way N-1 times and then differently the

N'th time.
>
Good point.
The last store is a little strange, but the compiler will certainly
optimize.
Maintenance is expensive, and the current code should be the best example.

regards,
Ranier Vilela