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

On Thu, Oct 8, 2015 at 7:03 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> 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.
>

Thank you for reviewing!
Attached the latest patch.

> +    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.

Thank you for your advice.
Fixed.

> +            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.

I agree.
I've changed so.

> 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.

Fixed.

> +    if (PageIsAllVisible(page))
>      {
> -        Assert(BufferIsValid(*vmbuffer));
>
> Why did you remove this assertion?

It's my mistake.
Fixed.

> +        if (all_frozen)
> +        {
> +            PageSetAllFrozen(page);
> +            flags |= VISIBILITYMAP_ALL_FROZEN;
> +        }
>
> Why didn't you call visibilitymap_test() for all frozen case here?

Same as above.
Fixed.

> 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.

I agree.
I added Assert() to beginning of visibilitymap_set() function.

> +                     * 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)));

I agree.
Fixed.

Regards,

--
Masahiko Sawada