Re: Refactoring of compression options in pg_basebackup

On Tue, Jan 18, 2022 at 11:27 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> WFM. Attached is a patch that extends --compress to handle a method
> with an optional compression level. Some extra tests are added to
> cover all that.

I think that this will reject something like --compress=nonetheless by
telling you that 't' is not a valid separator. I think it would be
better to code this so that we first identify the portion preceding
the first colon, or the whole string if there is no colon. Then we
check whether that part is a compression method that we recognize. If
not, we complain. If so, we then check whatever is after the separator
for validity - and this might differ by type. For example, we could
then immediately reject none:4, and if in the future we want to allow
lz4:fast3, we could.

I think the code that handles the bare integer case should be at the
top of the function and should return, because that code is short.
Then the rest of the function doesn't need to be indented as deeply.

"First check after the compression method" seems like it would be
better written "First check for the compression method" or "First
check the compression method".

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