Re: trying again to get incremental backup

On Fri, Nov 10, 2023 at 6:27 AM Dilip Kumar <dilipbalaut(at)gmail(dot)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?

> 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.

> 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.

--
Robert Haas
EDB: http://www.enterprisedb.com