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.