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

On Wed, Feb 14, 2018 at 3:59 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Fri, Feb 9, 2018 at 11:48 PM, Claudio Freire <klaussfreire(at)gmail(dot)com> wrote:
>> On Fri, Feb 9, 2018 at 1:36 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> On Fri, Feb 9, 2018 at 12:45 AM, Claudio Freire <klaussfreire(at)gmail(dot)com> wrote:
>>>> On Thu, Feb 8, 2018 at 1:36 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>>> On Tue, Feb 6, 2018 at 9:51 PM, Claudio Freire <klaussfreire(at)gmail(dot)com> wrote:
>>>>>> I can look into doing 3, that *might* get rid of the need to do that
>>>>>> initial FSM vacuum, but all other intermediate vacuums are still
>>>>>> needed.
>>>>>
>>>>> Understood. So how about that this patch focuses only make FSM vacuum
>>>>> more frequently and leaves the initial FSM vacuum and the handling
>>>>> cancellation cases? The rest can be a separate patch.
>>>>
>>>> Ok.
>>>>
>>>> Attached split patches. All but the initial FSM pass is in 1, the
>>>> initial FSM pass as in the original patch is in 2.
>>>>
>>>> I will post an alternative patch for 2 when/if I have one. In the
>>>> meanwhile, we can talk about 1.
>>>
>>> Thank you for updating the patch!
>>>
>>> + /* Tree pruning for partial vacuums */
>>> + if (threshold)
>>> + {
>>> + child_avail = fsm_get_avail(page, slot);
>>> + if (child_avail >= threshold)
>>> + continue;
>>> + }
>>>
>>> I think slots in a non-leaf page might not have a correct value
>>> because fsm_set_avail doesn't propagate up beyond pages. So do we need
>>> to check the root of child page rather than a slot in the non-lead
>>> page? I might be missing something though.
>>
>> I'm not sure I understand what you're asking.
>>
>> Yes, a partial FSM vacuum won't correct those inconsistencies. It
>> doesn't matter though, because as long as the root node (or whichever
>> inner node we're looking at the time) reports free space, the lookup
>> for free space will recurse and find the lower nodes, even if they
>> report more space than the inner node reported. The goal of making
>> free space discoverable will have been achieved nonetheless.
>
> For example, if a slot of internal node of fsm tree has a value
> greater than the threshold while the root of leaf node of fsm tree has
> a value less than threshold, we want to update the internal node of
> fsm tree but we will skip it with your patch. I wonder if this can
> happen.

If I understand your point correctly, that would mean that space was
used since the last vacuum. Partial FSM vacuums don't concern
themselves with that case, the normal free space search algorithm will
handle that, retrying with the next slot until it succeeds (or until
it gives up).

IIUC, each time a failure of such kind is found while searching, the
FSM gets updated to avoid following that link a second time.

See, in fsm_search:

/*
* At lower level, failure can happen if the value in the upper-
* level node didn't reflect the value on the lower page. Update
* the upper node, to avoid falling into the same trap again, and
* start over.

>
>> The final FSM vacuum pass isn't partial, to finally correct all those
>> small inconsistencies.
>
> Yes, but the purpose of this patch is to prevent table bloating from
> concurrent modifications?

Yes, by *marking* unmarked space. If the slot is above the threshold,
it means there's already marked free space on that branch, and search
will go into it already, so no pressing need to refine the mark. The
space already marked can serve while vacuum makes further progress.
Ie: it can wait.

> Here is other review comments.
>
> + /* Tree pruning for partial vacuums */
> + if (threshold)
> + {
> + child_avail = fsm_get_avail(page, slot);
> + if (child_avail >= threshold)
> + continue;
> + }
>
> 'child_avail' is a category value while 'threshold' is an actual size
> of freespace.

Good catch. It was meant to be a category, but I see the public
interface of the FSM doesn't usually expose categories, so fixing
that.

> The logic of finding the max_freespace seems not work fine if table
> with indices because max_freespace is not updated based on
> post-compaction freespace. I think we need to get the max freespace
> size during vacuuming heap (i.g. at lazy_vacuum_heap) and use it as
> the threshold.

Good point.

>
> + vacuum_fsm_every_pages = nblocks / 8;
> + vacuum_fsm_every_pages = Max(vacuum_fsm_every_pages, 1048576);
>
> I think a comment for 1048576 is needed. And perhaps we can define it
> as a macro.

1M pages = 8GB with an 8kb page size.

I can clarify.

Attached are new versions of the patches with these corrections.