Re: New vacuum option to do only freezing - Mailing list pgsql-hackers

On Thu, Jan 10, 2019 at 5:23 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> Hi,
>
> On 1/8/19, 7:03 PM, "Masahiko Sawada" <sawada.mshk@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