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

Started by Michael Paquierover 9 years ago10 messages

michael.paquier@gmail.com

over 9 years ago

1 attachment(s)

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

Attachments:

vm-comments-fix.patchtext/x-diff; charset=US-ASCII; name=vm-comments-fix.patchDownload

diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index b472d31..fa42498 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 setting bit(s)
  *		visibilitymap_pin_ok - check whether correct map page is already pinned
- *		visibilitymap_set	 - set a bit in a previously pinned page
+ *		visibilitymap_set	 - set bit(s) in a previously pinned page
  *		visibilitymap_get_status - get status of bits
  *		visibilitymap_count  - count number of bits set in visibility map
  *		visibilitymap_truncate	- truncate the 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 map update 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 visibility map page containing the bit for
  * the heap page. Because that can require I/O to read the map page, you
  * shouldn't hold a lock on the heap page while doing that. Then, call

Kyotaro HORIGUCHI

horiguchi.kyotaro@lab.ntt.co.jp

over 9 years ago

In reply to: Michael Paquier (#1)

1 attachment(s)

Re: 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

Attachments:

vm-comments-fix-2.patchtext/x-patch; charset=us-asciiDownload

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 correct map 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	- truncate the 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 map update 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 visibility map page containing the bit for
  * the heap page. Because that can require I/O to read the map page, you
  * shouldn't hold a lock on the heap page while doing that. Then, call

Michael Paquier

michael.paquier@gmail.com

over 9 years ago

In reply to: Kyotaro HORIGUCHI (#2)

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

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

Kyotaro HORIGUCHI

horiguchi.kyotaro@lab.ntt.co.jp

over 9 years ago

In reply to: Michael Paquier (#3)

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

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

Alvaro Herrera

alvherre@2ndquadrant.com

over 9 years ago

In reply to: Michael Paquier (#1)

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

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

Tom Lane

tgl@sss.pgh.pa.us

over 9 years ago

In reply to: Alvaro Herrera (#5)

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

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

Michael Paquier

michael.paquier@gmail.com

over 9 years ago

In reply to: Tom Lane (#6)

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

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

Michael Paquier

michael.paquier@gmail.com

over 9 years ago

In reply to: Michael Paquier (#7)

1 attachment(s)

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

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

Attachments:

vm-comments-fix-v2.patchapplication/x-patch; name=vm-comments-fix-v2.patchDownload

diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index b472d31..3d963c3 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -10,15 +10,6 @@
  * IDENTIFICATION
  *	  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_pin_ok - check whether correct map page is already pinned
- *		visibilitymap_set	 - set a bit in a previously pinned page
- *		visibilitymap_get_status - get status of bits
- *		visibilitymap_count  - count number of bits set in visibility map
- *		visibilitymap_truncate	- truncate the visibility map
- *
  * NOTES
  *
  * The visibility map is a bitmap with two bits (all-visible and all-frozen)
@@ -34,7 +25,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 +69,8 @@
  * When a bit is set, the LSN of the visibility map page is updated to make
  * sure that the visibility map update 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,13 +186,13 @@ 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 one or more bits
  *
- * Setting a bit in the visibility map is a two-phase operation. First, call
- * visibilitymap_pin, to pin the visibility map page containing the bit for
+ * Setting bits in the visibility map is a two-phase operation. First, call
+ * visibilitymap_pin, to pin the visibility map page containing the bits for
  * the heap page. Because that can require I/O to read the map page, you
  * shouldn't hold a lock on the heap page while doing that. Then, call
- * visibilitymap_set to actually set the bit.
+ * visibilitymap_set to actually set the bits.
  *
  * On entry, *buf should be InvalidBuffer or a valid buffer returned by
  * an earlier call to visibilitymap_pin or visibilitymap_get_status on the same
@@ -243,7 +234,7 @@ visibilitymap_pin_ok(BlockNumber heapBlk, Buffer buf)
 }
 
 /*
- *	visibilitymap_set - set bit(s) on a previously pinned page
+ *	visibilitymap_set - set one or more bits on a previously pinned page
  *
  * recptr is the LSN of the XLOG record we're replaying, if we're in recovery,
  * or InvalidXLogRecPtr in normal running.  The page LSN is advanced to the
@@ -340,13 +331,13 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
  * On entry, *buf should be InvalidBuffer or a valid buffer returned by an
  * earlier call to visibilitymap_pin or visibilitymap_get_status on the same
  * relation. On return, *buf is a valid buffer with the map page containing
- * the bit for heapBlk, or InvalidBuffer. The caller is responsible for
+ * the bits for heapBlk, or InvalidBuffer. The caller is responsible for
  * releasing *buf after it's done testing and setting bits.
  *
  * NOTE: This function is typically called without a lock on the heap page,
- * so somebody else could change the bit just after we look at it.  In fact,
+ * so somebody else could change the bits just after we look at it.  In fact,
  * since we don't lock the visibility map page either, it's even possible that
- * someone else could have changed the bit just before we look at it, but yet
+ * someone else could have changed the bits just before we look at it, but yet
  * we might see the old value.  It is the caller's responsibility to deal with
  * all concurrency issues!
  */
@@ -396,7 +387,8 @@ visibilitymap_get_status(Relation rel, BlockNumber heapBlk, Buffer *buf)
  *
  * Note: we ignore the possibility of race conditions when the table is being
  * extended concurrently with the call.  New pages added to the table aren't
- * going to be marked all-visible or all-frozen, so they won't affect the result.
+ * going to be marked all-visible or all-frozen, so they won't affect the
+ * result.
  */
 void
 visibilitymap_count(Relation rel, BlockNumber *all_visible, BlockNumber *all_frozen)

Robert Haas

robertmhaas@gmail.com

over 9 years ago

In reply to: Michael Paquier (#8)

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

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

#10

Michael Paquier

michael.paquier@gmail.com

over 9 years ago

In reply to: Robert Haas (#9)

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

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