Re: New IndexAM API controlling index vacuum strategies

Hi,
bq. We can mention in the commit log that since the commit changes
MaxHeapTuplesPerPage the encoding in gin posting list is also changed.

Yes - this is fine.

Thanks

On Mon, Jan 25, 2021 at 12:28 AM Masahiko Sawada <sawada.mshk@gmail.com>
wrote:

> (Please avoid top-posting on the mailing lists[1]: top-posting breaks
> the logic of a thread.)
>
> On Tue, Jan 19, 2021 at 12:02 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> >
> > Hi, Masahiko-san:
>
> Thank you for reviewing the patch!
>
> >
> > For v2-0001-Introduce-IndexAM-API-for-choosing-index-vacuum-s.patch :
> >
> > For blvacuumstrategy():
> >
> > +   if (params->index_cleanup == VACOPT_TERNARY_DISABLED)
> > +       return INDEX_VACUUM_STRATEGY_NONE;
> > +   else
> > +       return INDEX_VACUUM_STRATEGY_BULKDELETE;
> >
> > The 'else' can be omitted.
>
> Yes, but I'd prefer to leave it as it is because it's more readable
> without any performance side effect that we return BULKDELETE if index
> cleanup is enabled.
>
> >
> > Similar comment for ginvacuumstrategy().
> >
> > For v2-0002-Choose-index-vacuum-strategy-based-on-amvacuumstr.patch :
> >
> > If index_cleanup option is specified neither VACUUM command nor
> > storage option
> >
> > I think this is what you meant (by not using passive voice):
> >
> > If index_cleanup option specifies neither VACUUM command nor
> > storage option,
> >
> > - * integer, but you can't fit that many items on a page. 11 ought to be
> more
> > + * integer, but you can't fit that many items on a page. 13 ought to be
> more
> >
> > It would be nice to add a note why the number of bits is increased.
>
> I think that it might be better to mention such update history in the
> commit log rather than in the source code. Because most readers are
> likely to be interested in why 12 bits for offset number is enough,
> rather than why this value has been increased. In the source code
> comment, we describe why 12 bits for offset number is enough. We can
> mention in the commit log that since the commit changes
> MaxHeapTuplesPerPage the encoding in gin posting list is also changed.
> What do you think?
>
> > For choose_vacuum_strategy():
> >
> > +       IndexVacuumStrategy ivstrat;
> >
> > The variable is only used inside the loop. You can use
> vacrelstats->ivstrategies[i] directly and omit the variable.
>
> Fixed.
>
> >
> > +       int ndeaditems_limit = (int) ((freespace / sizeof(ItemIdData)) *
> 0.7);
> >
> > How was the factor of 0.7 determined ? Comment below only mentioned
> 'safety factor' but not how it was chosen.
> > I also wonder if this factor should be exposed as GUC.
>
> Fixed.
>
> >
> > +   if (nworkers > 0)
> > +       nworkers--;
> >
> > Should log / assert be added when nworkers is <= 0 ?
>
> Hmm I don't think so. As far as I read the code, there is no
> possibility that nworkers can be lower than 0 (we always increment it)
> and actually, the code works fine even if nworkers is a negative
> value.
>
> >
> > + * XXX: allowing to fill the heap page with only line pointer seems a
> overkill.
> >
> > 'a overkill' -> 'an overkill'
> >
>
> Fixed.
>
> The above comments are incorporated into the latest patch I just posted[2].
>
> [1] https://en.wikipedia.org/wiki/Posting_style#Top-posting
> [2]
> https://www.postgresql.org/message-id/CAD21AoCS94vK1fs-_%3DR5J3tp2DsZPq9zdcUu2pk6fbr7BS7quA%40mail.gmail.com
>
>
>
>
> --
> Masahiko Sawada
> EDB:  https://www.enterprisedb.com/
>