Обсуждение: Header and comments describing routines in incorrect shape in visibilitymap.c

Hi all,

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.
Regards,
--
Michael

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 

On Thu, Jul 7, 2016 at 4:41 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> 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

As far as I know, it is possible to set one or two bits, so I would
think that using parenthesis makes more sense. Same when pinning a
page, the caller may want to just set one of the two bits available.
That's the choice I am trying to outline here.
-- 
Michael

At Thu, 7 Jul 2016 16:48:00 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqQO0AaCwhKsyTChxu8h9-KE0_H6qMaWrg4t9aDhq0yyhw@mail.gmail.com>
> On Thu, Jul 7, 2016 at 4:41 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > 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
> 
> As far as I know, it is possible to set one or two bits,

That's right. 

> so I would
> think that using parenthesis makes more sense. Same when pinning a
> page, the caller may want to just set one of the two bits available.
> That's the choice I am trying to outline here.

I'm not strongly opposed to the paretheses. That's fine with me.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Michael Paquier wrote:
> Hi all,
> 
> 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.

Regarding the first hunk, I don't like these INTERFACE sections too
much; they get seriously outdated over the time and aren't all that
helpful anyway.  See the one on heapam.c for example.  I'd rather get
rid of that one instead of patching it.  The rest, of course, merit
revision.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Regarding the first hunk, I don't like these INTERFACE sections too
> much; they get seriously outdated over the time and aren't all that
> helpful anyway.  See the one on heapam.c for example.  I'd rather get
> rid of that one instead of patching it.  The rest, of course, merit
> revision.

+1, as long as we make sure that any useful info therein gets migrated
to the per-function header comments rather than dropped.  If there's
anything that doesn't seem to fit naturally in any per-function comment,
maybe move it into the file header comment?
        regards, tom lane

On Fri, Jul 8, 2016 at 1:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> Regarding the first hunk, I don't like these INTERFACE sections too
>> much; they get seriously outdated over the time and aren't all that
>> helpful anyway.  See the one on heapam.c for example.  I'd rather get
>> rid of that one instead of patching it.  The rest, of course, merit
>> revision.
>
> +1, as long as we make sure that any useful info therein gets migrated
> to the per-function header comments rather than dropped.  If there's
> anything that doesn't seem to fit naturally in any per-function comment,
> maybe move it into the file header comment?

OK, that removes comment duplication. Also, what about replacing
"bit(s)" by "one or more bits" in the comment terms where adapted?
That's bikeshedding, but that's what this patch is about.
-- 
Michael

On Fri, Jul 8, 2016 at 8:31 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Jul 8, 2016 at 1:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>>> Regarding the first hunk, I don't like these INTERFACE sections too
>>> much; they get seriously outdated over the time and aren't all that
>>> helpful anyway.  See the one on heapam.c for example.  I'd rather get
>>> rid of that one instead of patching it.  The rest, of course, merit
>>> revision.
>>
>> +1, as long as we make sure that any useful info therein gets migrated
>> to the per-function header comments rather than dropped.  If there's
>> anything that doesn't seem to fit naturally in any per-function comment,
>> maybe move it into the file header comment?
>
> OK, that removes comment duplication. Also, what about replacing
> "bit(s)" by "one or more bits" in the comment terms where adapted?
> That's bikeshedding, but that's what this patch is about.

Translating my thoughts into code, I get the attached.
--
Michael

On Fri, Jul 8, 2016 at 7:14 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
>> OK, that removes comment duplication. Also, what about replacing
>> "bit(s)" by "one or more bits" in the comment terms where adapted?
>> That's bikeshedding, but that's what this patch is about.
>
> Translating my thoughts into code, I get the attached.

Seems reasonable, but is the last hunk a whitespace-only change?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jul 14, 2016 at 4:29 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Jul 8, 2016 at 7:14 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>>> OK, that removes comment duplication. Also, what about replacing
>>> "bit(s)" by "one or more bits" in the comment terms where adapted?
>>> That's bikeshedding, but that's what this patch is about.
>>
>> Translating my thoughts into code, I get the attached.
>
> Seems reasonable, but is the last hunk a whitespace-only change?

Yes, sorry for that.
-- 
Michael