Re: New IndexAM API controlling index vacuum strategies

(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(at)yugabyte(dot)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/