On Mon, Feb 26, 2018 at 6:00 AM, Masahiko Sawada <[email protected]> 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.
