On Mon, Feb 26, 2018 at 1:32 PM, Claudio Freire <klaussfreire(at)gmail(dot)com> wrote:
> On Mon, Feb 26, 2018 at 11:31 AM, Claudio Freire <klaussfreire(at)gmail(dot)com> wrote:
>> On Mon, Feb 26, 2018 at 6:00 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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.