Re: Freeze avoidance of very large table. - Mailing list pgsql-hackers

On Mon, Oct 5, 2015 at 7:31 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Sat, Oct 3, 2015 at 3:41 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Fri, Oct 2, 2015 at 11:23 AM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>>>> +             /* all-frozen information is also cleared at the same time */
>>>>               PageClearAllVisible(page);
>>>> +             PageClearAllFrozen(page);
>>>
>>> I wonder if it makes sense to have a macro to clear both in unison,
>>> which seems a very common pattern.
>>
>> I think PageClearAllVisible should clear both, and there should be no
>> other macro.  There is no event that causes a page to cease being
>> all-visible that does not also cause it to cease being all-frozen.
>> You might think that deleting or locking a tuple would fall into that
>> category - but nope, XMAX needs to be cleared or the tuple pruned, or
>> there will be problems after wraparound.
>>
>
> Thank you for your advice.
> I understood.
>
> I changed the patch so that PageClearAllVisible clear both bits, and
> removed ClearAllFrozen.
> Attached the latest v16 patch which contains draft version documentation patch.

Thanks for updating the patch! Here are another review comments.

+    ereport(elevel,
+            (errmsg("skipped %d frozen pages acoording to visibility map",
+                    vacrelstats->vmskipped_frozen_pages)));

Typo: acoording should be according.

When vmskipped_frozen_pages is 1, "1 frozen pages" in log message
sounds incorrect in terms of grammar. So probably errmsg_plural()
should be used here.

+            relallvisible = visibilitymap_count(rel,
VISIBILITYMAP_ALL_VISIBLE);
+            relallfrozen = visibilitymap_count(rel, VISIBILITYMAP_ALL_FROZEN);

We can refactor visibilitymap_count() so that it counts the numbers of
both all-visible and all-frozen tuples at the same time, in order to
avoid reading through visibility map twice.

heap_page_is_all_visible() can set all_frozen to TRUE even when
it returns FALSE. This is odd because the page must not be all frozen
when it's not all visible. heap_page_is_all_visible() should set
all_frozen to FALSE whenever all_visible is set to FALSE?
Probably it's better to forcibly set all_frozen to FALSE at the end of
the function whenever all_visible is FALSE.

+    if (PageIsAllVisible(page))    {
-        Assert(BufferIsValid(*vmbuffer));

Why did you remove this assertion?

+        if (all_frozen)
+        {
+            PageSetAllFrozen(page);
+            flags |= VISIBILITYMAP_ALL_FROZEN;
+        }

Why didn't you call visibilitymap_test() for all frozen case here?

In visibilitymap_set(), the argument flag must be either
(VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN) or
VISIBILITYMAP_ALL_VISIBLE. So I think that it's better to add
Assert() which checks whether the specified flag is valid or not.

+                     * caller is expected to set PD_ALL_VISIBLE or
+                     * PD_ALL_FROZEN first.
+                     */
+                    Assert(PageIsAllVisible(heapPage) ||
PageIsAllFrozen(heapPage));

This should be the following?
 Assert(((flag | VISIBILITYMAP_ALL_VISIBLE) && PageIsAllVisible(heapPage)) ||             ((flag |
VISIBILITYMAP_ALL_FROZEN)&& PageIsAllFrozen(heapPage)));
 

Regards,

-- 
Fujii Masao