Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently - Mailing list pgsql-hackers

On Mon, Feb 26, 2018 at 1:32 PM, Claudio Freire <klaussfreire@gmail.com> wrote:
> On Mon, Feb 26, 2018 at 11:31 AM, Claudio Freire <klaussfreire@gmail.com> wrote:
>> On Mon, Feb 26, 2018 at 6:00 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>> Here is review comment for v4 patch.
>>>
>>> @@ -1922,6 +1988,8 @@ count_nondeletable_pages(Relation onerel,
>>> LVRelStats *vacrelstats)
>>>                  * We don't insert a vacuum delay point here, because we have an
>>>                  * exclusive lock on the table which we want to hold
>>> for as short a
>>>                  * time as possible.  We still need to check for
>>> interrupts however.
>>> +                * We might have to acquire the autovacuum lock,
>>> however, but that
>>> +                * shouldn't pose a deadlock risk.
>>>                  */
>>>                 CHECK_FOR_INTERRUPTS();
>>>
>>> Is this change necessary?
>>
>> I don't recall doing that change. Maybe a rebase gone wrong.
>>
>>> ----
>>> +               /*
>>> +                * If there are no indexes then we should periodically
>>> vacuum the FSM
>>> +                * on huge relations to make free space visible early.
>>> +                */
>>> +               if (nindexes == 0 &&
>>> +                       (vacuumed_pages - vacuumed_pages_at_fsm_vac) >
>>> vacuum_fsm_every_pages)
>>> +               {
>>> +                       /* Vacuum the Free Space Map */
>>> +                       FreeSpaceMapVacuum(onerel, max_freespace);
>>> +                       vacuumed_pages_at_fsm_vac = vacuumed_pages;
>>> +                       max_freespace = 0;
>>> +               }
>>>
>>> I think this code block should be executed before we check if the page
>>> is whether new or empty and then do 'continue'. Otherwise we cannot
>>> reach this code if the table has a lot of new or empty pages.
>>
>> In order for the counter (vacuumed_pages) to increase, there have to
>> be plenty of opportunities for this code to run, and I specifically
>> wanted to avoid vacuuming the FSM too often for those cases
>> particularly (when Vacuum scans lots of pages but does no writes).
>>
>>> ----
>>> @@ -663,6 +663,8 @@ fsm_extend(Relation rel, BlockNumber fsm_nblocks)
>>>   * If minValue > 0, the updated page is also searched for a page with at
>>>   * least minValue of free space. If one is found, its slot number is
>>>   * returned, -1 otherwise.
>>> + *
>>> + * If minValue == 0, the value at the root node is returned.
>>>   */
>>>  static int
>>>  fsm_set_and_search(Relation rel, FSMAddress addr, uint16 slot,
>>> @@ -687,6 +689,8 @@ fsm_set_and_search(Relation rel, FSMAddress addr,
>>> uint16 slot,
>>>
>>> addr.level == FSM_BOTTOM_LEVEL,
>>>                                                                    true);
>>>         }
>>> +       else
>>> +               newslot = fsm_get_avail(page, 0);
>>>
>>> I think it's not good idea to make fsm_set_and_search return either a
>>> slot number or a category value according to the argument. Category
>>> values is actually uint8 but this function returns it as int.
>>
>> Should be fine, uint8 can be contained inside an int in all platforms.
>>
>>> Also we
>>> can call fsm_set_and_search with minValue = 0 at
>>> RecordAndGetPageWithFreeSpace() when search_cat is 0. With you patch,
>>> fsm_set_and_search() then returns the root value but it's not correct.
>>
>> I guess I can add another function to do that.
>>
>>> Also, is this change necessary for this patch? ISTM this change is
>>> used for the change in fsm_update_recursive() as follows but I guess
>>> this change can be a separate patch.
>>>
>>> +       new_cat = fsm_set_and_search(rel, parent, parentslot, new_cat, 0);
>>> +
>>> +       /*
>>> +        * Bubble up, not the value we just set, but the one now in the root
>>> +        * node of the just-updated page, which is the page's highest value.
>>> +        */
>>
>> I can try to separate them I guess.
>
> Patch set with the requested changes attached.
>
> 2 didn't change AFAIK, it's just rebased on top of the new 1.

Oops. Forgot about the comment changes requested. Fixed those in v6, attached.