Обсуждение: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

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

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

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

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

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

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.

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

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

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

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/