Thread: log_heap_visible(): remove unused parameter and update comment
Hi, While resuming the work on [1] I noticed that: - there is an unused parameter in log_heap_visible() - the comment associated to the function is not in "sync" with the current implementation (referring a "block" that is not involved anymore) Attached a tiny patch as an attempt to address the above remarks. [1]: https://commitfest.postgresql.org/39/3740/ Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On Fri, Sep 30, 2022 at 1:07 PM Drouvot, Bertrand <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > While resuming the work on [1] I noticed that: > > - there is an unused parameter in log_heap_visible() > - the comment associated to the function is not in "sync" with the > current implementation (referring a "block" that is not involved anymore) > > Attached a tiny patch as an attempt to address the above remarks. > > [1]: https://commitfest.postgresql.org/39/3740/ It looks like that parameter was originally introduced and used in PG 9.4 where xl_heap_visible structure was having RelFileNode, which was later removed in PG 9.5, since then the RelFileNode rnode parameter is left out. This parameter got renamed to RelFileLocator rlocator by the commit b0a55e43299c4ea2a9a8c757f9c26352407d0ccc in HEAD. The attached patch LGTM. We recently committed another patch to remove an unused function parameter - 65b158ae4e892c2da7a5e31e2d2645e5e79a0bfd. It makes me think that why can't we remove the unused function parameters once and for all, say with a compiler flag such as -Wunused-parameter [1]? We might have to be careful in removing certain parameters which are not being used right now, but might be used in the near future though. [1] https://man7.org/linux/man-pages/man1/gcc.1.html -Wunused-parameter Warn whenever a function parameter is unused aside from its declaration. To suppress this warning use the "unused" attribute. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On 9/30/22 1:32 PM, Bharath Rupireddy wrote: > On Fri, Sep 30, 2022 at 1:07 PM Drouvot, Bertrand > <bertranddrouvot.pg@gmail.com> wrote: >> >> Hi, >> >> While resuming the work on [1] I noticed that: >> >> - there is an unused parameter in log_heap_visible() >> - the comment associated to the function is not in "sync" with the >> current implementation (referring a "block" that is not involved anymore) >> >> Attached a tiny patch as an attempt to address the above remarks. >> >> [1]: https://commitfest.postgresql.org/39/3740/ > > It looks like that parameter was originally introduced and used in PG > 9.4 where xl_heap_visible structure was having RelFileNode, which was > later removed in PG 9.5, since then the RelFileNode rnode parameter is > left out. This parameter got renamed to RelFileLocator rlocator by the > commit b0a55e43299c4ea2a9a8c757f9c26352407d0ccc in HEAD. > > The attached patch LGTM. Thanks for looking at it! > > We recently committed another patch to remove an unused function > parameter - 65b158ae4e892c2da7a5e31e2d2645e5e79a0bfd. > > It makes me think that why can't we remove the unused function > parameters once and for all, say with a compiler flag such as > -Wunused-parameter [1]? We might have to be careful in removing > certain parameters which are not being used right now, but might be > used in the near future though. > > [1] https://man7.org/linux/man-pages/man1/gcc.1.html > > -Wunused-parameter > Warn whenever a function parameter is unused aside from its > declaration. > > To suppress this warning use the "unused" attribute. > That's right. I have the feeling this will be somehow time consuming and I'm not sure the added value is worth it (as compare to fix them when "accidentally" cross their paths). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Fri, 30 Sep 2022 at 19:32, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > On Fri, Sep 30, 2022 at 1:07 PM Drouvot, Bertrand > <bertranddrouvot.pg@gmail.com> wrote: >> >> Hi, >> >> While resuming the work on [1] I noticed that: >> >> - there is an unused parameter in log_heap_visible() >> - the comment associated to the function is not in "sync" with the >> current implementation (referring a "block" that is not involved anymore) >> >> Attached a tiny patch as an attempt to address the above remarks. >> >> [1]: https://commitfest.postgresql.org/39/3740/ > > It looks like that parameter was originally introduced and used in PG > 9.4 where xl_heap_visible structure was having RelFileNode, which was > later removed in PG 9.5, since then the RelFileNode rnode parameter is > left out. This parameter got renamed to RelFileLocator rlocator by the > commit b0a55e43299c4ea2a9a8c757f9c26352407d0ccc in HEAD. > > The attached patch LGTM. > > We recently committed another patch to remove an unused function > parameter - 65b158ae4e892c2da7a5e31e2d2645e5e79a0bfd. > > It makes me think that why can't we remove the unused function > parameters once and for all, say with a compiler flag such as > -Wunused-parameter [1]? We might have to be careful in removing > certain parameters which are not being used right now, but might be > used in the near future though. > > [1] https://man7.org/linux/man-pages/man1/gcc.1.html > > -Wunused-parameter > Warn whenever a function parameter is unused aside from its > declaration. > > To suppress this warning use the "unused" attribute. When I try to use -Wunused-parameter, I find there are many warnings :-( . /home/japin/Codes/postgres/Debug/../src/backend/optimizer/geqo/geqo_pool.c: In function ‘free_chromo’: /home/japin/Codes/postgres/Debug/../src/backend/optimizer/geqo/geqo_pool.c:176:26: warning: unused parameter ‘root’ [-Wunused-parameter] 176 | free_chromo(PlannerInfo *root, Chromosome *chromo) | ~~~~~~~~~~~~~^~~~ /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/equivclass.c: In function ‘eclass_useful_for_merging’: /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/equivclass.c:3091:40: warning: unused parameter ‘root’ [-Wunused-parameter] 3091 | eclass_useful_for_merging(PlannerInfo *root, | ~~~~~~~~~~~~~^~~~ /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/indxpath.c: In function ‘ec_member_matches_indexcol’: /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/indxpath.c:3453:41: warning: unused parameter ‘root’ [-Wunused-parameter] 3453 | ec_member_matches_indexcol(PlannerInfo *root, RelOptInfo *rel, | ~~~~~~~~~~~~~^~~~ /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/indxpath.c:3453:59: warning: unused parameter ‘rel’ [-Wunused-parameter] 3453 | ec_member_matches_indexcol(PlannerInfo *root, RelOptInfo *rel, | ~~~~~~~~~~~~^~~ /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/indxpath.c: In function ‘relation_has_unique_index_for’: /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/indxpath.c:3511:44: warning: unused parameter ‘root’ [-Wunused-parameter] 3511 | relation_has_unique_index_for(PlannerInfo *root, RelOptInfo *rel, | ~~~~~~~~~~~~~^~~~ /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/joinpath.c: In function ‘allow_star_schema_join’: /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/joinpath.c:356:37: warning: unused parameter ‘root’ [-Wunused-parameter] 356 | allow_star_schema_join(PlannerInfo *root, | ~~~~~~~~~~~~~^~~~ /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/joinpath.c: In function ‘paraminfo_get_equal_hashops’: /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/joinpath.c:378:42: warning: unused parameter ‘root’ [-Wunused-parameter] 378 | paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info, | ~~~~~~~~~~~~~^~~~ -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
On Fri, Sep 30, 2022 at 7:30 PM Japin Li <japinli@hotmail.com> wrote: > > When I try to use -Wunused-parameter, I find there are many warnings :-( . Great! I think we can't just remove every unused parameter, for instance, it makes sense to retain PlannerInfo *root parameter even though it's not used now, in future it may be. But if the parameter is of type unrelated to the context of the function, like the one committed 65b158ae4e892c2da7a5e31e2d2645e5e79a0bfd and like the proposed patch to some extent, it could be removed. Others may have different thoughts here. > /home/japin/Codes/postgres/Debug/../src/backend/optimizer/geqo/geqo_pool.c: In function ‘free_chromo’: > /home/japin/Codes/postgres/Debug/../src/backend/optimizer/geqo/geqo_pool.c:176:26: warning: unused parameter ‘root’ [-Wunused-parameter] > 176 | free_chromo(PlannerInfo *root, Chromosome *chromo) > | ~~~~~~~~~~~~~^~~~ > /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/equivclass.c: In function ‘eclass_useful_for_merging’: > /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/equivclass.c:3091:40: warning: unused parameter ‘root’ [-Wunused-parameter] > 3091 | eclass_useful_for_merging(PlannerInfo *root, > | ~~~~~~~~~~~~~^~~~ > /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/indxpath.c: In function ‘ec_member_matches_indexcol’: > /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/indxpath.c:3453:41: warning: unused parameter ‘root’ [-Wunused-parameter] > 3453 | ec_member_matches_indexcol(PlannerInfo *root, RelOptInfo *rel, > | ~~~~~~~~~~~~~^~~~ > /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/indxpath.c:3453:59: warning: unused parameter ‘rel’ [-Wunused-parameter] > 3453 | ec_member_matches_indexcol(PlannerInfo *root, RelOptInfo *rel, > | ~~~~~~~~~~~~^~~ > /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/indxpath.c: In function ‘relation_has_unique_index_for’: > /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/indxpath.c:3511:44: warning: unused parameter ‘root’ [-Wunused-parameter] > 3511 | relation_has_unique_index_for(PlannerInfo *root, RelOptInfo *rel, > | ~~~~~~~~~~~~~^~~~ > /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/joinpath.c: In function ‘allow_star_schema_join’: > /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/joinpath.c:356:37: warning: unused parameter ‘root’ [-Wunused-parameter] > 356 | allow_star_schema_join(PlannerInfo *root, > | ~~~~~~~~~~~~~^~~~ > /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/joinpath.c: In function ‘paraminfo_get_equal_hashops’: > /home/japin/Codes/postgres/Debug/../src/backend/optimizer/path/joinpath.c:378:42: warning: unused parameter ‘root’ [-Wunused-parameter] > 378 | paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info, > | ~~~~~~~~~~~~~^~~~ -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Fri, 30 Sep 2022 at 22:09, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > On Fri, Sep 30, 2022 at 7:30 PM Japin Li <japinli@hotmail.com> wrote: >> >> When I try to use -Wunused-parameter, I find there are many warnings :-( . > > Great! > > I think we can't just remove every unused parameter, for instance, it > makes sense to retain PlannerInfo *root parameter even though it's not > used now, in future it may be. But if the parameter is of type > unrelated to the context of the function, like the one committed > 65b158ae4e892c2da7a5e31e2d2645e5e79a0bfd and like the proposed patch > to some extent, it could be removed. > > Others may have different thoughts here. Maybe we can define a macro like UNUSED(x) for those parameters, and call this macro on the parameter that we might be useful, then we can use -Wunused-parameter when compiling. Any thoughts? -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
On Fri, Sep 30, 2022 at 7:48 PM Japin Li <japinli@hotmail.com> wrote: > > On Fri, 30 Sep 2022 at 22:09, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Fri, Sep 30, 2022 at 7:30 PM Japin Li <japinli@hotmail.com> wrote: > >> > >> When I try to use -Wunused-parameter, I find there are many warnings :-( . > > > > Great! > > > > I think we can't just remove every unused parameter, for instance, it > > makes sense to retain PlannerInfo *root parameter even though it's not > > used now, in future it may be. But if the parameter is of type > > unrelated to the context of the function, like the one committed > > 65b158ae4e892c2da7a5e31e2d2645e5e79a0bfd and like the proposed patch > > to some extent, it could be removed. > > > > Others may have different thoughts here. > > Maybe we can define a macro like UNUSED(x) for those parameters, and > call this macro on the parameter that we might be useful, then > we can use -Wunused-parameter when compiling. Any thoughts? We have the pg_attribute_unused() macro already. I'm not sure if adding -Wunused-parameter for compilation plus using pg_attribute_unused() for unused-yet-contextually-required variables is a great idea. But it has some merits as it avoids unused variables lying around in the code. However, we can even discuss this in a separate thread IMO to hear more from other hackers. While on this, I noticed that pg_attribute_unused() is being used for npages in AdvanceXLInsertBuffer(), but the npages variable is actually being used there. I think we can remove it. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On 04.10.22 09:19, Bharath Rupireddy wrote: > We have the pg_attribute_unused() macro already. I'm not sure if > adding -Wunused-parameter for compilation plus using > pg_attribute_unused() for unused-yet-contextually-required variables > is a great idea. But it has some merits as it avoids unused variables > lying around in the code. However, we can even discuss this in a > separate thread IMO to hear more from other hackers. I tried this once. The patch I have from a few years ago is 420 files changed, 1482 insertions(+), 1482 deletions(-) and it was a lot of work to maintain. I can send it in if there is interest. But I'm not sure if it's worth it.