Header and comments describing routines in incorrect shape in visibilitymap.c

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers