Re: Refactoring of compression options in pg_basebackup

On Thu, Jan 20, 2022 at 2:03 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> Well, if no colon is specified, we still need to check if optarg
> is a pure integer if it does not match any of the supported methods,
> as --compress=0 should be backward compatible with no compression and
> --compress=1~9 should imply gzip, no?

Yes.

> Done this way, I hope.

This looks better, but this part could be switched around:

+ /*
+ * Check if the first part of the string matches with a supported
+ * compression method.
+ */
+ if (pg_strcasecmp(firstpart, "gzip") != 0 &&
+ pg_strcasecmp(firstpart, "none") != 0)
+ {
+ /*
+ * It does not match anything known, so check for the
+ * backward-compatible case of only an integer, where the implied
+ * compression method changes depending on the level value.
+ */
+ if (!option_parse_int(firstpart, "-Z/--compress", 0,
+ INT_MAX, levelres))
+ exit(1);
+
+ *methodres = (*levelres > 0) ?
+ COMPRESSION_GZIP : COMPRESSION_NONE;
+ return;
+ }
+
+ /* Supported method found. */
+ if (pg_strcasecmp(firstpart, "gzip") == 0)
+ *methodres = COMPRESSION_GZIP;
+ else if (pg_strcasecmp(firstpart, "none") == 0)
+ *methodres = COMPRESSION_NONE;

You don't need to test for gzip and none in two places each. Just make
the block with the "It does not match ..." comment the "else" clause
for this last part.

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