On Tue, Feb 27, 2018 at 1:44 AM, Claudio Freire <klaussfre...@gmail.com> wrote:
> On Mon, Feb 26, 2018 at 1:32 PM, Claudio Freire <klaussfre...@gmail.com>
> wrote:
>> On Mon, Feb 26, 2018 at 11:31 AM, Claudio Freire <klaussfre...@gmail.com>
>> wrote:
>>> On Mon, Feb 26, 2018 at 6:00 AM, Masahiko Sawada <sawada.m...@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
