On Thu, Mar 7, 2019 at 3:55 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Mar 5, 2019 at 11:29 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > Attached updated patch incorporated all of comments. Also I've added
> > new reloption vacuum_index_cleanup as per discussion on the "reloption
> > to prevent VACUUM from truncating empty pages at the end of relation"
> > thread. Autovacuums also can skip index cleanup when the reloption is
> > set to false. Since the setting this to false might lead some problems
> > I've made autovacuums report the number of dead tuples and dead
> > itemids we left.
>
> It seems to me that the disable_index_cleanup should be renamed
> index_cleanup and the default should be changed to true, for
> consistency with the reloption (and, perhaps, other patches).
Hmm, the patch already has new reloption vacuum_index_cleanup and
default value is true but you meant I should change its name to
index_cleanup?
>
> - num_tuples = live_tuples = tups_vacuumed = nkeep = nunused = 0;
> + num_tuples = live_tuples = tups_vacuumed = nkeep = nunused =
> + nleft_dead_itemids = nleft_dead_tuples = 0;
>
> I would suggest leaving the existing line alone (and not adding an
> extra space to it as the patch does now) and just adding a second
> initialization on the next line as a separate statement. a = b = c = d
> = e = 0 isn't such great coding style that we should stick to it
> rigorously even when it ends up having to be broken across lines.
Fixed.
>
> + /* Index vacuum must be enabled in two-pass vacuum */
> + Assert(!skip_index_vacuum);
>
> I am a big believer in naming consistency. Please, let's use the same
> name everywhere! If it's going to be index_cleanup, then call the
> reloption vacuum_index_cleanup, and call the option index_cleanup, and
> call the variable index_cleanup. Picking a different subset of
> cleanup, index, vacuum, skip, and disable for each new name makes it
> harder to understand.
Fixed.
>
> - * If there are no indexes then we can vacuum the page right now
> - * instead of doing a second scan.
> + * If there are no indexes or index vacuum is disabled we can
> + * vacuum the page right now instead of doing a second scan.
>
> This comment is wrong. That wouldn't be safe. And that's probably
> why it's not what the code does.
Fixed.
>
> - /* If no indexes, make log report that lazy_vacuum_heap would've made */
> + /*
> + * If no index or index vacuum is disabled, make log report that
> + * lazy_vacuum_heap would've make.
> + */
> if (vacuumed_pages)
>
> Hmm, does this really do what the comment claims? It looks to me like
> we only increment vacuumed_pages when we call lazy_vacuum_page(), and
> we (correctly) don't do that when index cleanup is disabled, but then
> here this claims that if (vacuumed_pages) will be true in that case.
You're right, vacuumed_pages never be > 0 in disable_index_cleanup case. Fixed.
>
> I wonder if it would be cleaner to rename vacrelstate->hasindex to
> 'useindex' and set it to false if there are no indexes or index
> cleanup is disabled. But that might actually be worse, not sure.
>
I tried the changes and it seems good idea to me. Fixed.
Attached the updated version patches.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center