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

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.