Re: refactoring basebackup.c (zstd workers)

On Sun, Mar 20, 2022 at 03:05:28PM -0400, Robert Haas wrote:
> On Thu, Mar 17, 2022 at 3:41 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> > - errmsg("unrecognized compression algorithm: \"%s\"",
> > + errmsg("unrecognized compression algorithm \"%s\"",
> >
> > Most other places seem to say "compression method". So I'd suggest to change
> > that here, and in doc/src/sgml/ref/pg_basebackup.sgml.
>
> I'm not sure that's really better, and I don't think this patch is
> introducing an altogether novel usage. I think I would probably try to
> standardize on algorithm rather than method if I were standardizing
> the whole source tree, but I think we can leave that discussion for
> another time.

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

> > + result->parse_error =
> > + pstrdup("found empty string where a compression option was expected");
> >
> > Needs to be localized with _() ?
> > Also, document that it's pstrdup'd.
>
> 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 '= _('

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

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

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

typo: contain a an

--
Justin