Thread: [patch] pg_basebackup: mention that spread checkpoints are the default in --help
[patch] pg_basebackup: mention that spread checkpoints are the default in --help
From
Michael Banck
Date:
Hi, I believed that spread (not fast) checkpoints are the default in pg_basebackup, but noticed that --help does not specify which is which - contrary to the reference documentation. So I propose the small attached patch to clarify that. Michael
Attachment
Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help
From
Aleksander Alekseev
Date:
Hi, > I believed that spread (not fast) checkpoints are the default in > pg_basebackup, but noticed that --help does not specify which is which - > contrary to the reference documentation. > > So I propose the small attached patch to clarify that. You are right and I believe this is a good change. Maybe we should also display the defaults for -X, --manifest-checksums, etc for consistency. -- Best regards, Aleksander Alekseev
Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help
From
Michael Banck
Date:
Hi, On Thu, Oct 19, 2023 at 04:21:19PM +0300, Aleksander Alekseev wrote: > > I believed that spread (not fast) checkpoints are the default in > > pg_basebackup, but noticed that --help does not specify which is which - > > contrary to the reference documentation. > > > > So I propose the small attached patch to clarify that. > > You are right and I believe this is a good change. > > Maybe we should also display the defaults for -X, > --manifest-checksums, etc for consistency. Hrm right, but those have multiple options and they do not enumerate them in the help string as do -F and -c - not sure what general project policy here is for mentioning defaults in --help, I will check some of the other commands. Michael
Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help
From
Michael Paquier
Date:
On Thu, Oct 19, 2023 at 10:30:04PM +0200, Michael Banck wrote: > Hrm right, but those have multiple options and they do not enumerate > them in the help string as do -F and -c - not sure what general project > policy here is for mentioning defaults in --help, I will check some of > the other commands. Then comes the point that this bloats the --help output. A bunch of system commands I use on a daily-basis outside Postgres don't do that, so it's kind of hard to put a line on what's good or not in this area while we have the SGML and man pages to do the job, with always more details. -- Michael
Attachment
Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help
From
Aleksander Alekseev
Date:
Hi, > On Thu, Oct 19, 2023 at 10:30:04PM +0200, Michael Banck wrote: > > Hrm right, but those have multiple options and they do not enumerate > > them in the help string as do -F and -c - not sure what general project > > policy here is for mentioning defaults in --help, I will check some of > > the other commands. > > Then comes the point that this bloats the --help output. A bunch of > system commands I use on a daily-basis outside Postgres don't do that, > so it's kind of hard to put a line on what's good or not in this area > while we have the SGML and man pages to do the job, with always more > details. Right. Then I suggest merging the patch as is. -- Best regards, Aleksander Alekseev
Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help
From
Peter Eisentraut
Date:
On 19.10.23 11:39, Michael Banck wrote:
> Hi,
>
> I believed that spread (not fast) checkpoints are the default in
> pg_basebackup, but noticed that --help does not specify which is which -
> contrary to the reference documentation.
>
> So I propose the small attached patch to clarify that.
> printf(_(" -c, --checkpoint=fast|spread\n"
>- " set fast or spread
checkpointing\n"));
>+ " set fast or spread (default)
checkpointing\n"));
Could we do like
-c, --checkpoint=fast|spread
set fast or spread checkpointing
(default: spread)
This seems to be easier to read.
Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help
From
Michael Banck
Date:
Hi,
On Wed, Oct 25, 2023 at 04:36:41PM +0200, Peter Eisentraut wrote:
> On 19.10.23 11:39, Michael Banck wrote:
> > Hi,
> >
> > I believed that spread (not fast) checkpoints are the default in
> > pg_basebackup, but noticed that --help does not specify which is which -
> > contrary to the reference documentation.
> >
> > So I propose the small attached patch to clarify that.
>
> > printf(_(" -c, --checkpoint=fast|spread\n"
> >- " set fast or spread checkpointing\n"));
> >+ " set fast or spread (default)
> checkpointing\n"));
>
> Could we do like
>
> -c, --checkpoint=fast|spread
> set fast or spread checkpointing
> (default: spread)
>
> This seems to be easier to read.
Yeah, we could do that. But then again the question pops up what to do
about the other option that mentions defaults (-F) and the others which
have a default but it is not spelt out yet (-X, -Z at least) (output is
still from v15, additional options have been added since):
-F, --format=p|t output format (plain (default), tar)
-X, --wal-method=none|fetch|stream
include required WAL files with specified method
-Z, --compress=0-9 compress tar output with given compression level
So, my personal opinion is that we should really document -c because it
is quite user-noticable compared to the others.
So attached is a new version with just your proposed change for now.
Michael
Attachment
Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help
From
Shlok Kyal
Date:
Hi,
On Thu, 26 Oct 2023 at 13:58, Michael Banck <mbanck@gmx.net> wrote:
>
> Hi,
>
> On Wed, Oct 25, 2023 at 04:36:41PM +0200, Peter Eisentraut wrote:
> > On 19.10.23 11:39, Michael Banck wrote:
> > > Hi,
> > >
> > > I believed that spread (not fast) checkpoints are the default in
> > > pg_basebackup, but noticed that --help does not specify which is which -
> > > contrary to the reference documentation.
> > >
> > > So I propose the small attached patch to clarify that.
> >
> > > printf(_(" -c, --checkpoint=fast|spread\n"
> > >- " set fast or spread checkpointing\n"));
> > >+ " set fast or spread (default)
> > checkpointing\n"));
> >
> > Could we do like
> >
> > -c, --checkpoint=fast|spread
> > set fast or spread checkpointing
> > (default: spread)
> >
> > This seems to be easier to read.
>
> Yeah, we could do that. But then again the question pops up what to do
> about the other option that mentions defaults (-F) and the others which
> have a default but it is not spelt out yet (-X, -Z at least) (output is
> still from v15, additional options have been added since):
>
> -F, --format=p|t output format (plain (default), tar)
> -X, --wal-method=none|fetch|stream
> include required WAL files with specified method
> -Z, --compress=0-9 compress tar output with given compression level
>
> So, my personal opinion is that we should really document -c because it
> is quite user-noticable compared to the others.
>
> So attached is a new version with just your proposed change for now.
>
>
> Michael
I went through the Cfbot for this patch and found out that the build
is failing with the following error (Link:
https://cirrus-ci.com/task/4648506929971200?logs=build#L1217):
[08:34:47.625] FAILED: src/bin/pg_basebackup/pg_basebackup.p/pg_basebackup.c.o
[08:34:47.625] ccache cc -Isrc/bin/pg_basebackup/pg_basebackup.p
-Isrc/include -I../src/include -Isrc/interfaces/libpq
-I../src/interfaces/libpq -Isrc/include/catalog -Isrc/include/nodes
-Isrc/include/utils -fdiagnostics-color=always -pipe
-D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -g -fno-strict-aliasing
-fwrapv -fexcess-precision=standard -D_GNU_SOURCE -Wmissing-prototypes
-Wpointer-arith -Werror=vla -Wendif-labels -Wmissing-format-attribute
-Wimplicit-fallthrough=3 -Wcast-function-type
-Wshadow=compatible-local -Wformat-security
-Wdeclaration-after-statement -Wno-format-truncation
-Wno-stringop-truncation -pthread -MD -MQ
src/bin/pg_basebackup/pg_basebackup.p/pg_basebackup.c.o -MF
src/bin/pg_basebackup/pg_basebackup.p/pg_basebackup.c.o.d -o
src/bin/pg_basebackup/pg_basebackup.p/pg_basebackup.c.o -c
../src/bin/pg_basebackup/pg_basebackup.c
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c: In function ‘usage’:
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c:411:5:
warning: statement with no effect [-Wunused-value]
[08:34:47.625] 411 | " (default: spread)\n"));
[08:34:47.625] | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c:411:51: error:
expected ‘;’ before ‘)’ token
[08:34:47.625] 411 | " (default: spread)\n"));
[08:34:47.625] | ^
[08:34:47.625] | ;
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c:411:51: error:
expected statement before ‘)’ token
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c:411:52: error:
expected statement before ‘)’ token
[08:34:47.625] 411 | " (default: spread)\n"));
[08:34:47.625] | ^
[08:34:47.629] [1210/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/parallel.c.o
[08:34:47.639] [1211/1832] Compiling C object
src/bin/pg_basebackup/pg_recvlogical.p/pg_recvlogical.c.o
[08:34:47.641] [1212/1832] Linking static target
src/bin/pg_basebackup/libpg_basebackup_common.a
[08:34:47.658] [1213/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/compress_io.c.o
[08:34:47.669] [1214/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/compress_lz4.c.o
[08:34:47.678] [1215/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/compress_zstd.c.o
[08:34:47.692] [1216/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/dumputils.c.o
[08:34:47.692] ninja: build stopped: subcommand failed.
I also see that patch is marked 'Ready for Committer' on commitfest.
Just wanted to make sure, you are aware of this error.
Thanks,
Shlok Kumar Kyal
Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help
From
Michael Banck
Date:
Hi, On Tue, Oct 31, 2023 at 04:59:24PM +0530, Shlok Kyal wrote: > I went through the Cfbot for this patch and found out that the build > is failing with the following error (Link: > https://cirrus-ci.com/task/4648506929971200?logs=build#L1217): Oops, sorry. Attached is a working third version of this patch. Michael
Attachment
Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help
From
Magnus Hagander
Date:
On Tue, Oct 31, 2023 at 8:01 PM Michael Banck <mbanck@gmx.net> wrote: > > Hi, > > On Tue, Oct 31, 2023 at 04:59:24PM +0530, Shlok Kyal wrote: > > I went through the Cfbot for this patch and found out that the build > > is failing with the following error (Link: > > https://cirrus-ci.com/task/4648506929971200?logs=build#L1217): > > Oops, sorry. Attached is a working third version of this patch. While I think Peters argument about one reading better than the other one, that does also increase the "help message bloat" mentioned by Michael. So I think we're better off actually using the original version, so I'm going to go ahead and push that one (and also to avoid endless bikeshedding)- -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/