Horton

Thread

Commits

  1. Show the default checkpoint method in the pg_basebackup help message

  1. [patch] pg_basebackup: mention that spread checkpoints are the default in --help

    Michael Banck <mbanck@gmx.net> — 2023-10-19T09:39:32Z

    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

  2. Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

    Aleksander Alekseev <aleksander@timescale.com> — 2023-10-19T13:21:19Z

    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

  3. Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

    Michael Banck <mbanck@gmx.net> — 2023-10-19T20:30:04Z

    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

  4. Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

    Michael Paquier <michael@paquier.xyz> — 2023-10-19T23:29:53Z

    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

  5. Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

    Aleksander Alekseev <aleksander@timescale.com> — 2023-10-20T09:03:05Z

    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

  6. Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

    Peter Eisentraut <peter@eisentraut.org> — 2023-10-25T14:36:41Z

    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.

  7. Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

    Michael Banck <mbanck@gmx.net> — 2023-10-26T08:27:51Z

    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

  8. Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2023-10-31T11:29:24Z

    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

  9. Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

    Michael Banck <mbanck@gmx.net> — 2023-10-31T19:01:38Z

    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

  10. Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

    Magnus Hagander <magnus@hagander.net> — 2024-01-10T12:33:25Z

    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/