On Sun, Mar 27, 2022 at 4:50 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> Actually, I suggest to remove those comments:
> | "We check for failure here because..."
>
> That should be the rule rather than the exception, so shouldn't require
> justifying why one might checks the return value of library and system calls.
I went for modifying the comment rather than removing it. I agree with
you that checking for failure doesn't really require justification,
but I think that in a case like this it is useful to explain what we
know about why it might fail.
> In bbsink_zstd_new(), I think you need to check to see if workers were
> requested (same as the issue you found with "level").
Fixed.
> src/backend/replication/basebackup_zstd.c: elog(ERROR, "could not set zstd compression level to %d:
%s",
> src/bin/pg_basebackup/bbstreamer_gzip.c: pg_log_error("could not set compression level %d: %s",
> src/bin/pg_basebackup/bbstreamer_zstd.c: pg_log_error("could not set compression level to: %d:
%s",
>
> I'm not sure why these messages sometimes mention the current compression
> method and sometimes don't. I suggest that they shouldn't - errcontext will
> have the algorithm, and the user already specified it anyway. It'd allow the
> compiler to merge strings.
I don't think that errcontext() helps here. On the client side, it
doesn't exist. On the server side, it's not in use. I do see
STATEMENT: <whatever> in the server log when a replication command
throws a server-side error, which is similar, but pg_basebackup
doesn't display that STATEMENT line. I don't really know how to
balance the legitimate desire for future messages against the
also-legitimate desire for clarity about where things are failing. I'm
slightly inclined to think that including the algorithm name is
better, because options are in the end algorithm-specific, but it's
certainly debatable. I would be interested in hearing other
opinions...
Here's an updated and rebased version of my patch.
--
Robert Haas
EDB: http://www.enterprisedb.com
