On Fri, Jul 1, 2016 at 9:30 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Jul 1, 2016 at 10:09 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>>> Ah, I see. So your suggestion is to do this job in lazy_scan_heap()
>>>> when scanning each block, and then to issue a WARNING and clear the
>>>> visibility map. Indeed that's better. I guess I need to take a closer
>>>> look at vacuumlazy.c. See attached for example, but that's perhaps not
>>>> something to have in 9.6 as that's more a micro-optimization than
>>>> anything else.
>>>
>>> Right, something like that. I think Andres actually wants something
>>> like this in 9.6, and I'm inclined to think it might be a good idea,
>>> too. I think there should probably be a test for
>>> all_visible_according_to_vm at the beginning of that so that we don't
>>> add more visibility map checks for pages where we already know the VM
>>> bit can't possibly be set.
>>>
>>> Other opinions on the concept or the patch?
>>
>> +1 on the idea.
>>
>> + PageClearAllVisible(page);
>> + MarkBufferDirty(buf);
>>
>> What is the need to clear the Page level bit, if it is already
>> cleared, doesn't '!all_frozen' indicate that?
>
> No, I don't think so. I think all_frozen indicates whether we think
> that all tuples on the page qualify as fully frozen. I don't think it
> tells us anything about whether PD_ALL_VISIBLE is set on the page.
>
Then, can we decide to clear it on that basis? Isn't it possible that
page is marked as all_visible, even if it contains frozen tuples? In
the other nearby code (refer below part of code), we only clear the
page level bit after ensuring it is set. Am I missing something?
else if (PageIsAllVisible(page) && has_dead_tuples)
{
elog(WARNING, "page containing dead tuples is marked as all-visible in
relation \"%s\" page %u",
relname, blkno);
PageClearAllVisible(page);
MarkBufferDirty(buf);
visibilitymap_clear(onerel, blkno, vmbuffer);
}
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com