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

[Michael Paquier]

On Thu, Jul 14, 2016 at 4:29 AM, Robert Haas  wrote:
> On Fri, Jul 8, 2016 at 7:14 AM, Michael Paquier
>  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

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

[Robert Haas]

On Fri, Jul 8, 2016 at 7:14 AM, Michael Paquier
 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

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

[Michael Paquier]

On Fri, Jul 8, 2016 at 8:31 AM, Michael Paquier
 wrote:
> On Fri, Jul 8, 2016 at 1:18 AM, Tom Lane  wrote:
>> Alvaro Herrera  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
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 

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

[Michael Paquier]

On Fri, Jul 8, 2016 at 1:18 AM, Tom Lane  wrote:
> Alvaro Herrera  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

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

[Tom Lane]

Alvaro Herrera  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

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

[Alvaro Herrera]

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 Herrerahttp://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

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

[Kyotaro HORIGUCHI]

At Thu, 7 Jul 2016 16:48:00 +0900, Michael Paquier  
wrote in 
> On Thu, Jul 7, 2016 at 4:41 PM, Kyotaro HORIGUCHI
>  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

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

[Michael Paquier]

On Thu, Jul 7, 2016 at 4:41 PM, Kyotaro HORIGUCHI
 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

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

[Kyotaro HORIGUCHI]

Hello,

At Wed, 6 Jul 2016 11:28:19 +0900, Michael Paquier  
wrote in 
> 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 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

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