Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently
| От | Claudio Freire |
|---|---|
| Тема | Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently |
| Дата | |
| Msg-id | CAGTBQpaE8nnRA0Uj_jBXcLS3QzH97ugLkEXv5_REh=Qt3vxMvg@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently (Stephen Frost <sfrost@snowman.net>) |
| Список | pgsql-hackers |
On Sat, Jan 6, 2018 at 7:33 PM, Stephen Frost <sfrost@snowman.net> wrote:
Greetings Claudio,Looks like this patch probably still applies and the changes suggested
* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Mon, Nov 27, 2017 at 2:39 PM, Jing Wang <jingwangian@gmail.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.
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@gmail.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.
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.
Вложения
В списке pgsql-hackers по дате отправления: