log_heap_visible(): remove unused parameter and update comment
Hi,
While resuming the work on [1]https://commitfest.postgresql.org/39/3740/ 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
Attachments:
v1-0001-log_heap_visible-unused-parameter.patchtext/plain; charset=UTF-8; name=v1-0001-log_heap_visible-unused-parameter.patchDownload
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 75b214824d..7465334a83 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -8188,17 +8188,17 @@ log_heap_freeze(Relation reln, Buffer buffer, TransactionId cutoff_xid,
}
/*
- * Perform XLogInsert for a heap-visible operation. 'block' is the block
- * being marked all-visible, and vm_buffer is the buffer containing the
- * corresponding visibility map block. Both should have already been modified
- * and dirtied.
+ * Perform XLogInsert for a heap-visible operation. heap_buffer is the buffer
+ * containing the block being marked all-visible, and vm_buffer is the buffer
+ * containing the corresponding visibility map block. Both should have already
+ * been modified and dirtied.
*
* If checksums are enabled, we also generate a full-page image of
* heap_buffer, if necessary.
*/
XLogRecPtr
-log_heap_visible(RelFileLocator rlocator, Buffer heap_buffer, Buffer vm_buffer,
- TransactionId cutoff_xid, uint8 vmflags)
+log_heap_visible(Buffer heap_buffer, Buffer vm_buffer, TransactionId cutoff_xid,
+ uint8 vmflags)
{
xl_heap_visible xlrec;
XLogRecPtr recptr;
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index d62761728b..ccc97e3b09 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -283,8 +283,7 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
if (XLogRecPtrIsInvalid(recptr))
{
Assert(!InRecovery);
- recptr = log_heap_visible(rel->rd_locator, heapBuf, vmBuf,
- cutoff_xid, flags);
+ recptr = log_heap_visible(heapBuf, vmBuf, cutoff_xid, flags);
/*
* If data checksums are enabled (or wal_log_hints=on), we
diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h
index 34220d93cf..4b6bdf6955 100644
--- a/src/include/access/heapam_xlog.h
+++ b/src/include/access/heapam_xlog.h
@@ -415,7 +415,7 @@ extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple,
MultiXactId *relminmxid_out);
extern void heap_execute_freeze_tuple(HeapTupleHeader tuple,
xl_heap_freeze_tuple *frz);
-extern XLogRecPtr log_heap_visible(RelFileLocator rlocator, Buffer heap_buffer,
- Buffer vm_buffer, TransactionId cutoff_xid, uint8 vmflags);
+extern XLogRecPtr log_heap_visible(Buffer heap_buffer, Buffer vm_buffer,
+ TransactionId cutoff_xid, uint8 vmflags);
#endif /* HEAPAM_XLOG_H */
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.
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]https://man7.org/linux/man-pages/man1/gcc.1.html? 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.
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.
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.