Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently

On Tue, Feb 27, 2018 at 1:44 AM, Claudio Freire <klaussfreire@gmail.com> wrote:
> 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.

Thank you for updating patches!

0001 patch looks good to me except for the following unnecessary empty lines.

+                * If there are no indexes then we should periodically
vacuum the FSM
+                * on huge relations to make free space visible early.
+                */
+               else if (nindexes == 0 && fsm_updated_pages >
vacuum_fsm_every_pages)
+               {
+                       /* Vacuum the Free Space Map */
+                       FreeSpaceMapVacuum(onerel, max_freespace);
+                       fsm_updated_pages = 0;
+                       max_freespace = 0;
+               }
+
+
+               /*

@@ -913,10 +967,14 @@ lazy_scan_heap(Relation onerel, int options,
LVRelStats *vacrelstats,

vmbuffer, InvalidTransactionId,

VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN);
                                END_CRIT_SECTION();
+
                        }

0002 patch looks good to me.

For 0003 patch, I think fsm_set() function name could lead misreading
because it actually not only sets the value but also returns the
value. fsm_set_and_get() might be better?

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center