Hello, At Wed, 6 Jul 2016 11:28:19 +0900, Michael Paquier <michael.paqu...@gmail.com> wrote in <cab7npqqsqvzbuyjtkgdb5csry-bcp740g2ompncqz3yzx4t...@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 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
