Re: Allow pg_archivecleanup to remove backup history files

From: torikoshia <torikoshia(at)oss(dot)nttdata(dot)com>
To: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
Cc: michael(at)paquier(dot)xyz, masao(dot)fujii(at)oss(dot)nttdata(dot)com, bharath(dot)rupireddyforpostgres(at)gmail(dot)com, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Allow pg_archivecleanup to remove backup history files
Date: 2023-06-15 12:38:28
Message-ID: 2ea9987153ecb1f7ce52d798011cf303@oss.nttdata.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 2023-06-15 15:20, Kyotaro Horiguchi wrote:
Thanks for your review!

> At Wed, 14 Jun 2023 00:49:39 +0900, torikoshia
> <torikoshia(at)oss(dot)nttdata(dot)com> wrote in
>> On 2023-06-12 16:33, Michael Paquier wrote:
>> > On Fri, Jun 09, 2023 at 12:32:15AM +0900, Fujii Masao wrote:
>> Thanks for reviewing!
>>
>> >> printf(_(" -n, --dry-run dry run, show the names of the files that
>> >> would be removed\n"));
>> >> + printf(_(" -b, --clean-backup-history clean up files including
>> >> backup history files\n"));
>> >> Shouldn't -b option be placed in alphabetical order?
>> > +1.
>>
>> Modified the place.
>
> - printf(_(" -d generate debug output (verbose mode)\n"));
> - printf(_(" -n dry run, show the names of the files that
> would be removed\n"));
> - printf(_(" -V, --version output version information, then
> exit\n"));
> - printf(_(" -x EXT clean up files if they have this
> extension\n"));
> - printf(_(" -?, --help show this help, then exit\n"));
> + printf(_(" -d, --debug generate debug output
> (verbose mode)\n"));
> + printf(_(" -n, --dry-run dry run, show the names of
> the files that would be removed\n"));
> + printf(_(" -V, --version output version information,
> then exit\n"));
> + printf(_(" -x, --strip-extension=EXT strip this extention before
> identifying files fo clean up\n"));
> + printf(_(" -?, --help show this help, then
> exit\n"));
>
> After this change, some of these lines corss the boundary of the 80
> columns width. (is that policy viable noadays? I am usually working
> using terminal windows with such a width..) It's somewhat unrelated to
> this patch, but a help line a few lines further down also exceeds the
> width. We could shorten it by removing the "/mnt/server" portion, but
> I'm not sure if it's worth doing.

I also highlight 80th column according to the wiki[1].
Since usage() in other files like pg_rewind.c and initdb.c also
exceeded the 80th column, I thought that was something like a guide.

>> Or for use as a standalone archive cleaner:
>> e.g.
>> pg_archivecleanup /mnt/server/archiverdir
>> 000000010000000000000010.00000020.backup
>
>
> + printf(_(" -x, --strip-extension=EXT strip this extention before
> identifying files fo clean up\n"));
>
> s/fo/for/ ?

Yeah, it's a typo. Fixed it.

[1]
https://wiki.postgresql.org/wiki/Configuring_vim_for_postgres_development
--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

Attachment Content-Type Size
v7-0001-Introduce-pg_archivecleanup-into-getopt_long.patch text/x-diff 3.9 KB
v7-0002-Allow-pg_archivecleanup-to-remove-backup-history-.patch text/x-diff 10.8 KB
v7-0003-Refactored-to-reduce-the-nesting-level.patch text/x-diff 5.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message torikoshia 2023-06-15 13:00:46 Re: RFC: Logging plan of the running query
Previous Message Laurenz Albe 2023-06-15 11:54:34 Re: When IMMUTABLE is not.