On Thu, Jan 10, 2019 at 2:45 PM Masahiko Sawada <sawada.m...@gmail.com> wrote:
>
> 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.
>Attached the updated patch. Please review it. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
v4-0001-Add-DISABLE_INDEX_CLEANUP-option-to-VACUUM-comman.patch
Description: Binary data
