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

On Sat, Jan 6, 2018 at 7:33 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> Greetings Claudio,
>
> * Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> > On Mon, Nov 27, 2017 at 2:39 PM, Jing Wang <jingwangian(at)gmail(dot)com>
> wrote:
> > > A few general comments.
> > >
> > > + FreeSpaceMapVacuum(onerel, 64);
> > >
> > > Just want to know why '64' is used here? It's better to give a
> description.
> > >
> > > + else
> > > + {
> > > + newslot = fsm_get_avail(page, 0);
> > > + }
> > >
> > > Since there is only one line in the else the bracket will not be
> needed. And
> > > there in one more space ahead of else which should be removed.
> > >
> > >
> > > + if (nindexes == 0 &&
> > > + (vacuumed_pages_at_fsm_vac - vacuumed_pages) >
> > > vacuum_fsm_every_pages)
> > > + {
> > > + /* Vacuum the Free Space Map */
> > > + FreeSpaceMapVacuum(onerel, 0);
> > > + vacuumed_pages_at_fsm_vac = vacuumed_pages;
> > > + }
> > >
> > > vacuumed_pages_at_fsm_vac is initialised with value zero and seems no
> chance
> > > to go into the bracket.
> >
> > The patch presented still applies, and there has been a review two
> > days ago. This is a short delay to answer for the author, so I am
> > moving this patch to next CF with the same status of waiting on
> > author.
>
> Looks like this patch probably still applies and the changes suggested
> above seem pretty small, any chance you can provide an updated patch
> soon Claudio, so that it can be reviewed properly during this
> commitfest?
>

I failed to notice the comments among the list's noise, sorry.

I'll get on to it now.

On Mon, Nov 27, 2017 at 2:39 AM, Jing Wang <jingwangian(at)gmail(dot)com> wrote:

> A few general comments.
>
> + FreeSpaceMapVacuum(onerel, 64);
>
> Just want to know why '64' is used here? It's better to give a
> description.
>

It's just a random cutoff point.

The point of that vacuum run is to mark free space early, to avoid needless
relation extension before long vacuum runs finish. This only happens if the
FSM is mostly zeroes, if there's free space in there, it will be used.

To make this run fast, since it's all redundant work before the final FSM
vacuum pass, branches with more than that amount of free space are skipped.
There's little point in vacuuming those early since they already contain
free space. This helps avoid the quadratic cost of vacuuming the FSM every
N dead tuples, since already-vacuumed branches will have free space, and
will thus be skipped. It could still be quadratic in the worst case, but it
avoids it often enough.

As for the value itself, it's an arbitrary choice. In-between index passes,
we have a rather precise cutoff we can use to avoid this redundant work.
But in the first run, we don't, so I made an arbitrary choice there that
felt right. I have no empirical performance data about alternative values.

>
> + else
> + {
> + newslot = fsm_get_avail(page, 0);
> + }
>
> Since there is only one line in the else the bracket will not be needed.
> And there in one more space ahead of else which should be removed.
>

Ok

>
> + if (nindexes == 0 &&
> + (vacuumed_pages_at_fsm_vac - vacuumed_pages) >
> vacuum_fsm_every_pages)
> + {
> + /* Vacuum the Free Space Map */
> + FreeSpaceMapVacuum(onerel, 0);
> + vacuumed_pages_at_fsm_vac = vacuumed_pages;
> + }
>
> vacuumed_pages_at_fsm_vac is initialised with value zero and seems no
> chance to go into the bracket.
>

You're right, the difference there is backwards

Attached patch with the proposed changes.