Re: refactoring basebackup.c (zstd workers) - Mailing list pgsql-hackers

From Robert Haas
Subject Re: refactoring basebackup.c (zstd workers)
Date
Msg-id CA+TgmoY90uUaqtx133BsH26ZabTRK-J5VoOsGga_wRw8+HdWpQ@mail.gmail.com
Whole thread Raw
In response to Re: refactoring basebackup.c  (Andres Freund <andres@anarazel.de>)
Responses Re: refactoring basebackup.c (zstd workers)  (Justin Pryzby <pryzby@telsasoft.com>)
List pgsql-hackers
On Sun, Mar 20, 2022 at 3:40 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> The user-facing docs are already standardized using "compression method", with
> 2 exceptions, of which one is contrib/ and the other is what I'm suggesting to
> make consistent here.
>
> $ git grep 'compression algorithm' doc
> doc/src/sgml/pgcrypto.sgml:    Which compression algorithm to use.  Only available if
> doc/src/sgml/ref/pg_basebackup.sgml:        compression algorithm is selected, or if server-side compression

Well, if you just count the number of occurrences of each string in
the documentation, sure. But all of the ones that are talking about a
compression method seem to have to do with configurable TOAST
compression, and the fact that the documentation for that feature is
more extensive than for the pre-existing feature that refers to a
compression algorithm does not, at least in my view, turn it into a
project standard from which no deviation is permitted.

> > Did the latter. The former would need to be fixed in a bunch of places
> > and while I'm happy to accept an expert opinion on exactly what needs
> > to be done here, I don't want to try to do it and do it wrong. Better
> > to let someone with good knowledge of the subject matter patch it up
> > later than do a crummy job now.
>
> I believe it just needs _("foo")
> See git grep '= _('

Hmm. Maybe.

> I mentioned another issue off-list:
> pg_basebackup.c:2741:10: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
>  2741 |   Assert(compressloc = COMPRESS_LOCATION_SERVER);
>       |          ^~~~~~~~~~~
> pg_basebackup.c:2741:3: note: in expansion of macro ‘Assert’
>  2741 |   Assert(compressloc = COMPRESS_LOCATION_SERVER);
>
> This crashes the server using your v2 patch:
>
> src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp --no-sync --no-manifest
--compress=server-zstd:level,|wc -c 

Well that's unfortunate. Will fix.

> I wonder whether the syntax should really use both ":" and ",".
> Maybe ":" isn't needed at all.

I don't think we should treat the compression method name in the same
way as a compression algorithm option.

> This patch also needs to update the other user-facing docs.

Which ones exactly?

> typo: contain a an

OK, will fix.

--
Robert Haas
EDB: http://www.enterprisedb.com



pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: refactoring basebackup.c (zstd workers)
Next
From: "Euler Taveira"
Date:
Subject: Re: Skipping logical replication transactions on subscriber side