On Thu, Jan 10, 2019 at 5:23 AM Bossart, Nathan <bossa...@amazon.com> wrote:
>
> Hi,
>
> On 1/8/19, 7:03 PM, "Masahiko Sawada" <sawada.m...@gmail.com> wrote:
> > Attached the updated version patch incorporated all comments I got.
>
> Thanks for the new patch!
>
> > * Instead of freezing xmax I've changed the behaviour of the new
> > option (DISABLE_INDEX_CLEANUP) so that it sets dead tuples as dead
> > instead of as unused and skips both index vacuum and index cleanup.
> > That is, we remove the storage of dead tuple but leave dead itemids
> > for the index lookups. These are removed by the next vacuum execution
> > enabling index cleanup. Currently HOT-pruning doesn't set the root of
> > the chain as unused even if the whole chain is dead. Since setting
> > tuples as dead removes storage the freezing xmax is no longer
> > required.
>
> I tested this option with a variety of cases (HOT, indexes, etc.), and
> it seems to work well. I haven't looked too deeply into the
> implications of using LP_DEAD this way, but it seems like a reasonable
> approach at first glance.
Thank you for reviewing the patch!
>
> + <varlistentry>
> + <term><literal>DISABLE_INDEX_CLEANUP</literal></term>
> + <listitem>
> + <para>
> + <command>VACUUM</command> removes dead tuples and prunes HOT-updated
> + tuples chain for live tuples on table. If the table has any dead tuple
> + it removes them from both table and indexes for re-use. With this
> + option <command>VACUUM</command> marks tuples as dead (i.e., it doesn't
> + remove tuple storage) and disables removing dead tuples from indexes.
> + This is suitable for avoiding transaction ID wraparound but not
> + sufficient for avoiding index bloat. This option cannot be used in
> + conjunction with <literal>FULL</literal> option.
> + </para>
> + </listitem>
> + </varlistentry>
>
> I think we should clarify the expected behavior when this option is
> used on a table with no indexes. We probably do not want to fail, as
> this could disrupt VACUUM commands that touch several tables. Also,
> we don't need to set tuples as dead instead of unused, which appears
> to be what this patch is actually doing:
>
> + if (nindexes > 0 && disable_index_cleanup)
> + lazy_set_tuples_dead(onerel, blkno, buf,
> vacrelstats);
> + else
> + lazy_vacuum_page(onerel, blkno, buf, 0,
> vacrelstats, &vmbuffer);
>
> In this case, perhaps we should generate a log statement that notes
> that DISABLE_INDEX_CLEANUP is being ignored and set
> disable_index_cleanup to false during processing.
Agreed. How about output a NOTICE message before calling
lazy_scan_heap() in lazy_vacuum_rel()?
>
> + if (disable_index_cleanup)
> + ereport(elevel,
> + (errmsg("\"%s\": marked %.0f row
> versions in %u pages as dead",
> +
> RelationGetRelationName(onerel),
> + tups_vacuumed,
> vacuumed_pages)));
> + else
> + ereport(elevel,
> + (errmsg("\"%s\": removed %.0f row
> versions in %u pages",
> +
> RelationGetRelationName(onerel),
> + tups_vacuumed,
> vacuumed_pages)));
>
> Should the first log statement only be generated when there are also
> indexes?
You're right. Will fix.
>
> +static void
> +lazy_set_tuples_dead(Relation onerel, BlockNumber blkno, Buffer buffer,
> + LVRelStats *vacrelstats)
>
> This function looks very similar to lazy_vacuum_page(). Perhaps the
> two could be combined?
>
Yes, I intentionally separed them as I was concerned the these
functions have different assumptions and usage. But the combining them
also could work. Will change it.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
