Re: Setting visibility map in VACUUM's second phase

On Sat, Jan 26, 2013 at 11:25 PM, Pavan Deolasee
<pavan(dot)deolasee(at)gmail(dot)com> wrote:
> On Thu, Jan 24, 2013 at 9:31 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>> On Thu, Jan 24, 2013 at 1:28 AM, Pavan Deolasee
>> <pavan(dot)deolasee(at)gmail(dot)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