On Fri, Jul 8, 2016 at 8:31 AM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Fri, Jul 8, 2016 at 1:18 AM, Tom Lane <t...@sss.pgh.pa.us> wrote: >> Alvaro Herrera <alvhe...@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
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)
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
