Re: Setting visibility map in VACUUM's second phase - Mailing list pgsql-hackers

On Sat, Jan 26, 2013 at 11:25 PM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:
> On Thu, Jan 24, 2013 at 9:31 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
>> On Thu, Jan 24, 2013 at 1:28 AM, Pavan Deolasee
>> <pavan.deolasee@gmail.com> wrote:
>
>>>
>>> Good idea. Even though the cost of pinning/unpinning may not be high
>>> with respect to the vacuum cost itself, but it seems to be a good idea
>>> because we already do that at other places. Do you have any other
>>> review comments on the patch or I'll fix this and send an updated
>>> patch soon.
>>
>> That was the only thing that stood out to me.
>
> The attached patch gets that improvement. Also rebased on the latest head.

Hi Pavan,

I get this warning:
vacuumlazy.c:890: warning: passing argument 6 of 'lazy_vacuum_page'
makes pointer from integer without a cast

and make check then fails.

I've added '&' to that line, and it now passes make check with --enable-cassert.



At line 1096, when you release the vmbuffer, you don't set it to
InvalidBuffer like the other places in the code do.  It seems like
this does would lead to a crash or assertion failure, but it does not
seem to do so.

other places:
            if (BufferIsValid(vmbuffer))
              {
                  ReleaseBuffer(vmbuffer);
                  vmbuffer = InvalidBuffer;
              }

Also, the "Note: If you change anything below, also look at" should
probably say "Note: If you change anything in the for loop below, also
look at".  Otherwise I'd be wondering how far below the caveat
applies.


I've attached a patch with these changes made.  Does this look OK?

Thanks,

Jeff