From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Peter Eisentraut <peter(at)eisentraut(dot)org>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: trying again to get incremental backup |
Date: | 2023-11-13 19:22:30 |
Message-ID: | CA+TgmoZFK6eQ7c-wmXQUmT102EEg7TMs6uU2w3gC2jHYm_n9tg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Bruce Momjian | 2023-11-13 19:28:05 | Re: Version 14/15 documentation Section "Alter Default Privileges" |
Previous Message | Andrew Atkinson | 2023-11-13 19:12:31 | Re: [PATCH] pgbench log file headers |