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.m...@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 <z...@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/ >
