Thread: [PATCH] Redudant initilization

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.

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

Patch applied to head, thanks.

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> 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.)

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.

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.