On Tue, Nov 14, 2023 at 12:52 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, Nov 10, 2023 at 6:27 AM Dilip Kumar <dilipbalaut@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