Re: Header and comments describing routines in incorrect shape in visibilitymap.c - Mailing list pgsql-hackers

Hello,

At Wed, 6 Jul 2016 11:28:19 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqQsQVZbuyjtkGdb5csry-bcp740G2oMPNcQz3yzx4t0xw@mail.gmail.com>
> I just bumped into a couple of things in visibilitymap.c:
> - visibilitymap_clear clears all bits of a visibility map, its header
> comment mentions the contrary
> - The header of visibilitymap.c mentions correctly "a bit" when
> referring to setting them, but when clearing, it should say that all
> bits are cleared.
> - visibilitymap_set can set multiple bits
> - visibilitymap_pin can be called to set up more than 1 bit.
> 
> This can be summed by the patch attached.

Thank you, I must have been careless on reviewing.


Although some of these are not directly related to 'a bit'
correction, I have some comments on the edited words.

====
- *        visibilitymap_pin     - pin a map page for setting a bit
+ *        visibilitymap_pin     - pin a map page for setting bit(s)

Is the parentheses needed? And then pinning is needed not only
for setting bits, but also for clearing. How about the following?

- *        visibilitymap_pin     - pin a map page for setting a bit
+ *        visibilitymap_pin     - pin a map page for changing bits

====
- *        visibilitymap_set     - set a bit in a previously pinned page
+ *        visibilitymap_set     - set bit(s) in a previously pinned page

So, the 'pinned' is not necessary here or should be added also to
_clear.  I think the former is preferable since it is already
written in the comments for the functions and seems to be a bit
too detailed to be put here.

- *        visibilitymap_set     - set a bit in a previously pinned page
+ *        visibilitymap_set     - set bits in the visibility map
====

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index b472d31..7985c1d 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -11,10 +11,10 @@ *      src/backend/access/heap/visibilitymap.c * * INTERFACE ROUTINES
- *        visibilitymap_clear  - clear a bit in the visibility map
- *        visibilitymap_pin     - pin a map page for setting a bit
+ *        visibilitymap_clear  - clear all bits in the visibility map
+ *        visibilitymap_pin     - pin a map page for changing bits *        visibilitymap_pin_ok - check whether
correctmap page is already pinned
 
- *        visibilitymap_set     - set a bit in a previously pinned page
+ *        visibilitymap_set     - set bits in the visibility map *        visibilitymap_get_status - get status of
bits*        visibilitymap_count  - count number of bits set in visibility map *        visibilitymap_truncate    -
truncatethe visibility map
 
@@ -34,7 +34,7 @@ * might not be true. * * Clearing visibility map bits is not separately WAL-logged.  The callers
- * must make sure that whenever a bit is cleared, the bit is cleared on WAL
+ * must make sure that whenever bits are cleared, the bits are cleared on WAL * replay of the updating operation as
well.* * When we *set* a visibility map during VACUUM, we must write WAL.  This may
 
@@ -78,8 +78,8 @@ * When a bit is set, the LSN of the visibility map page is updated to make * sure that the visibility
mapupdate doesn't get written to disk before the * WAL record of the changes that made it possible to set the bit is
flushed.
- * But when a bit is cleared, we don't have to do that because it's always
- * safe to clear a bit in the map from correctness point of view.
+ * But when bits are cleared, we don't have to do that because it's always
+ * safe to clear bits in the map from correctness point of view. *
*-------------------------------------------------------------------------*/
 
@@ -195,9 +195,9 @@ visibilitymap_clear(Relation rel, BlockNumber heapBlk, Buffer buf)}/*
- *    visibilitymap_pin - pin a map page for setting a bit
+ *    visibilitymap_pin - pin a map page for setting bit(s) *
- * Setting a bit in the visibility map is a two-phase operation. First, call
+ * Setting bit(s) in the visibility map is a two-phase operation. First, call * visibilitymap_pin, to pin the
visibilitymap page containing the bit for * the heap page. Because that can require I/O to read the map page, you *
shouldn'thold a lock on the heap page while doing that. Then, call