On Tue, Nov 14, 2023 at 12:52 AM Robert Haas <robertmh...@gmail.com> wrote: > > On Fri, Nov 10, 2023 at 6:27 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > - I think 0001 looks good improvement irrespective of the patch series. > > OK, perhaps that can be independently committed, then, if nobody objects. > > Thanks for the review; I've fixed a bunch of things that you > mentioned. I'll just comment on the ones I haven't yet done anything > about below. > > > 2. > > + <varlistentry id="guc-wal-summarize-keep-time" > > xreflabel="wal_summarize_keep_time"> > > + <term><varname>wal_summarize_keep_time</varname> > > (<type>boolean</type>) > > + <indexterm> > > + <primary><varname>wal_summarize_keep_time</varname> > > configuration parameter</primary> > > + </indexterm> > > > > I feel the name of the guy should be either wal_summarizer_keep_time > > or wal_summaries_keep_time, I mean either we should refer to the > > summarizer process or to the way summaries files. > > How about wal_summary_keep_time?
Yes, that looks perfect to me. > > 6. > > + * If the whole range of LSNs is covered, returns true, otherwise false. > > + * If false is returned, *missing_lsn is set either to InvalidXLogRecPtr > > + * if there are no WAL summary files in the input list, or to the first LSN > > + * in the range that is not covered by a WAL summary file in the input > > list. > > + */ > > +bool > > +WalSummariesAreComplete(List *wslist, XLogRecPtr start_lsn, > > > > I did not see the usage of this function, but I think if the whole > > range is not covered why not keep the behavior uniform w.r.t. what we > > set for '*missing_lsn', I mean suppose there is no file then > > missing_lsn is the start_lsn because a very first LSN is missing. > > It's used later in the patch series. I think the way that I have it > makes for a more understandable error message. Okay > > 8. > > +/* > > + * Comparator to sort a List of WalSummaryFile objects by start_lsn. > > + */ > > +static int > > +ListComparatorForWalSummaryFiles(const ListCell *a, const ListCell *b) > > +{ > > I'm not sure what needs fixing here. I think I copy-pasted it by mistake, just ignore it. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com