Add compress-level range checking for each compress-type.

The prior range checking was done based on the valid values for gz. While this worked it was a subset of what is available for lz4 and zst.

Allow the range to be specified for each compress-type. Adding this functionality to the parse module would be a better solution but that is a bigger project than this fix deserves, at least for now.

doc/lib/pgBackRestDoc/Custom/DocConfigData.pm

@@ -426,9 +426,8 @@ foreach my $strKey (sort(keys(%{$rhConfigDefine})))
         $rhConfigDefine->{$strKey}{&CFGDEF_SECURE} = false;
     }
 
-    # All int, size and time options must have an allow range
+    # All size and time options must have an allow range
-    if (($rhConfigDefine->{$strKey}{&CFGDEF_TYPE} eq CFGDEF_TYPE_INTEGER ||
+    if (($rhConfigDefine->{$strKey}{&CFGDEF_TYPE} eq CFGDEF_TYPE_TIME ||
-         $rhConfigDefine->{$strKey}{&CFGDEF_TYPE} eq CFGDEF_TYPE_TIME ||
          $rhConfigDefine->{$strKey}{&CFGDEF_TYPE} eq CFGDEF_TYPE_SIZE) &&
          !(defined($rhConfigDefine->{$strKey}{&CFGDEF_ALLOW_RANGE}) || defined($rhConfigDefine->{$strKey}{&CFGDEF_ALLOW_LIST})))
     {

doc/xml/release.xml

@@ -27,6 +27,21 @@
 
                         <p>Remove support for <proper>PostgreSQL</proper> <id>9.0</id>/<id>9.1</id>/<id>9.2</id>.</p>
                     </release-item>
+
+                    <release-item>
+                        <github-issue id="1216"/>
+                        <github-issue id="1931"/>
+                        <github-pull-request id="1962"/>
+
+                        <release-item-contributor-list>
+                            <release-item-ideator id="gkleen"/>
+                            <release-item-ideator id="viperru"/>
+                            <release-item-contributor id="david.steele"/>
+                            <release-item-reviewer id="stefan.fercot"/>
+                        </release-item-contributor-list>
+
+                        <p>Add <br-option>compress-level</br-option> range checking for each <br-option>compress-type</br-option>.</p>
+                    </release-item>
                 </release-improvement-list>
 
                 <release-development-list>
@@ -12046,6 +12061,11 @@
             <contributor-id type="github">geirra</contributor-id>
         </contributor>
 
+        <contributor id="gkleen">
+            <contributor-name-display>gkleen</contributor-name-display>
+            <contributor-id type="github">gkleen</contributor-id>
+        </contributor>
+
         <contributor id="greg.sabino.mullane">
             <contributor-name-display>Greg Sabino Mullane</contributor-name-display>
             <contributor-id type="github">turnstep</contributor-id>
@@ -12606,6 +12626,11 @@
             <contributor-name-display>Viorel Tabara</contributor-name-display>
         </contributor>
 
+        <contributor id="viperru">
+            <contributor-name-display>ViperRu</contributor-name-display>
+            <contributor-id type="github">ViperRu</contributor-id>
+        </contributor>
+
         <contributor id="vitaliy.kukharik">
             <contributor-name-display>Vitaliy Kukharik</contributor-name-display>
             <contributor-id type="github">vitabaks</contributor-id>

src/build/config/config.yaml

@@ -692,7 +692,6 @@ option:
     section: global
     type: integer
     required: false
-    allow-range: [0, 9]
     command: compress
     command-role:
       async: {}

src/common/compress/bz2/compress.c

@@ -163,7 +163,7 @@ bz2CompressNew(int level)
         FUNCTION_LOG_PARAM(INT, level);
     FUNCTION_LOG_END();
 
-    ASSERT(level > 0);
+    ASSERT(level >= BZ2_COMPRESS_LEVEL_MIN && level <= BZ2_COMPRESS_LEVEL_MAX);
 
     IoFilter *this = NULL;
 

src/common/compress/bz2/compress.h

@@ -13,6 +13,13 @@ Filter type constant
 ***********************************************************************************************************************************/
 #define BZ2_COMPRESS_FILTER_TYPE                                    STRID5("bz2-cmp", 0x41a3df3420)
 
+/***********************************************************************************************************************************
+Level constants
+***********************************************************************************************************************************/
+#define BZ2_COMPRESS_LEVEL_DEFAULT                                  9
+#define BZ2_COMPRESS_LEVEL_MIN                                      1
+#define BZ2_COMPRESS_LEVEL_MAX                                      9
+
 /***********************************************************************************************************************************
 Constructors
 ***********************************************************************************************************************************/

src/common/compress/gz/compress.c

@@ -168,7 +168,7 @@ gzCompressNew(int level)
         FUNCTION_LOG_PARAM(INT, level);
     FUNCTION_LOG_END();
 
-    ASSERT(level >= -1 && level <= 9);
+    ASSERT(level >= GZ_COMPRESS_LEVEL_MIN && level <= GZ_COMPRESS_LEVEL_MAX);
 
     IoFilter *this = NULL;
 

src/common/compress/gz/compress.h

@@ -13,6 +13,13 @@ Filter type constant
 ***********************************************************************************************************************************/
 #define GZ_COMPRESS_FILTER_TYPE                                     STRID5("gz-cmp", 0x20d1ef470)
 
+/***********************************************************************************************************************************
+Level constants
+***********************************************************************************************************************************/
+#define GZ_COMPRESS_LEVEL_DEFAULT                                   6
+#define GZ_COMPRESS_LEVEL_MIN                                       -1
+#define GZ_COMPRESS_LEVEL_MAX                                       9
+
 /***********************************************************************************************************************************
 Constructors
 ***********************************************************************************************************************************/

src/common/compress/helper.c

@@ -42,7 +42,9 @@ static const struct CompressHelperLocal
     IoFilter *(*compressNew)(int);                                  // Function to create new compression filter
     StringId decompressType;                                        // Type of the decompression filter
     IoFilter *(*decompressNew)(void);                               // Function to create new decompression filter
-    int levelDefault;                                               // Default compression level
+    int levelDefault:8;                                             // Default compression level
+    int levelMin:8;                                                 // Minimum compression level
+    int levelMax:8;                                                 // Maximum compression level
 } compressHelperLocal[] =
 {
     {
@@ -58,7 +60,9 @@ static const struct CompressHelperLocal
         .compressNew = bz2CompressNew,
         .decompressType = BZ2_DECOMPRESS_FILTER_TYPE,
         .decompressNew = bz2DecompressNew,
-        .levelDefault = 9,
+        .levelDefault = BZ2_COMPRESS_LEVEL_DEFAULT,
+        .levelMin = BZ2_COMPRESS_LEVEL_MIN,
+        .levelMax = BZ2_COMPRESS_LEVEL_MAX,
     },
     {
         .typeId = STRID5("gz", 0x3470),
@@ -68,7 +72,9 @@ static const struct CompressHelperLocal
         .compressNew = gzCompressNew,
         .decompressType = GZ_DECOMPRESS_FILTER_TYPE,
         .decompressNew = gzDecompressNew,
-        .levelDefault = 6,
+        .levelDefault = GZ_COMPRESS_LEVEL_DEFAULT,
+        .levelMin = GZ_COMPRESS_LEVEL_MIN,
+        .levelMax = GZ_COMPRESS_LEVEL_MAX,
     },
     {
         .typeId = STRID6("lz4", 0x2068c1),
@@ -79,7 +85,9 @@ static const struct CompressHelperLocal
         .compressNew = lz4CompressNew,
         .decompressType = LZ4_DECOMPRESS_FILTER_TYPE,
         .decompressNew = lz4DecompressNew,
-        .levelDefault = 1,
+        .levelDefault = LZ4_COMPRESS_LEVEL_DEFAULT,
+        .levelMin = LZ4_COMPRESS_LEVEL_MIN,
+        .levelMax = LZ4_COMPRESS_LEVEL_MAX,
 #endif
     },
     {
@@ -91,7 +99,9 @@ static const struct CompressHelperLocal
         .compressNew = zstCompressNew,
         .decompressType = ZST_DECOMPRESS_FILTER_TYPE,
         .decompressNew = zstDecompressNew,
-        .levelDefault = 3,
+        .levelDefault = ZST_COMPRESS_LEVEL_DEFAULT,
+        .levelMin = ZST_COMPRESS_LEVEL_MIN,
+        .levelMax = ZST_COMPRESS_LEVEL_MAX,
 #endif
     },
     {
@@ -190,6 +200,34 @@ compressLevelDefault(CompressType type)
     FUNCTION_TEST_RETURN(INT, compressHelperLocal[type].levelDefault);
 }
 
+/**********************************************************************************************************************************/
+int
+compressLevelMin(CompressType type)
+{
+    FUNCTION_TEST_BEGIN();
+        FUNCTION_TEST_PARAM(ENUM, type);
+    FUNCTION_TEST_END();
+
+    ASSERT(type < LENGTH_OF(compressHelperLocal));
+    compressTypePresent(type);
+
+    FUNCTION_TEST_RETURN(INT, compressHelperLocal[type].levelMin);
+}
+
+/**********************************************************************************************************************************/
+int
+compressLevelMax(CompressType type)
+{
+    FUNCTION_TEST_BEGIN();
+        FUNCTION_TEST_PARAM(ENUM, type);
+    FUNCTION_TEST_END();
+
+    ASSERT(type < LENGTH_OF(compressHelperLocal));
+    compressTypePresent(type);
+
+    FUNCTION_TEST_RETURN(INT, compressHelperLocal[type].levelMax);
+}
+
 /**********************************************************************************************************************************/
 IoFilter *
 compressFilter(CompressType type, int level)

src/common/compress/helper.intern.h

@@ -12,4 +12,10 @@ Functions
 // Default compression level for a compression type, used while loading the configuration
 int compressLevelDefault(CompressType type);
 
+// Minimum compression level for a compression type, used while loading the configuration
+int compressLevelMin(CompressType type);
+
+// Maximum compression level for a compression type, used while loading the configuration
+int compressLevelMax(CompressType type);
+
 #endif

src/common/compress/lz4/compress.c

@@ -249,7 +249,7 @@ lz4CompressNew(int level)
         FUNCTION_LOG_PARAM(INT, level);
     FUNCTION_LOG_END();
 
-    ASSERT(level >= 0);
+    ASSERT(level >= LZ4_COMPRESS_LEVEL_MIN && level <= LZ4_COMPRESS_LEVEL_MAX);
 
     IoFilter *this = NULL;
 

src/common/compress/lz4/compress.h

@@ -15,6 +15,13 @@ Filter type constant
 ***********************************************************************************************************************************/
 #define LZ4_COMPRESS_FILTER_TYPE                                    STRID6("lz4-cmp", 0x103436e068c1)
 
+/***********************************************************************************************************************************
+Level constants
+***********************************************************************************************************************************/
+#define LZ4_COMPRESS_LEVEL_DEFAULT                                  1
+#define LZ4_COMPRESS_LEVEL_MIN                                      -5
+#define LZ4_COMPRESS_LEVEL_MAX                                      12
+
 /***********************************************************************************************************************************
 Constructors
 ***********************************************************************************************************************************/

src/common/compress/zst/compress.c

@@ -170,7 +170,7 @@ zstCompressNew(int level)
         FUNCTION_LOG_PARAM(INT, level);
     FUNCTION_LOG_END();
 
-    ASSERT(level >= 0);
+    ASSERT(level >= ZST_COMPRESS_LEVEL_MIN && level <= ZST_COMPRESS_LEVEL_MAX);
 
     IoFilter *this = NULL;
 

src/common/compress/zst/compress.h

@@ -15,6 +15,13 @@ Filter type constant
 ***********************************************************************************************************************************/
 #define ZST_COMPRESS_FILTER_TYPE                                    STRID5("zst-cmp", 0x41a3dd27a0)
 
+/***********************************************************************************************************************************
+Level constants
+***********************************************************************************************************************************/
+#define ZST_COMPRESS_LEVEL_DEFAULT                                  3
+#define ZST_COMPRESS_LEVEL_MIN                                      -7
+#define ZST_COMPRESS_LEVEL_MAX                                      22
+
 /***********************************************************************************************************************************
 Constructors
 ***********************************************************************************************************************************/

src/config/load.c

@@ -357,12 +357,26 @@ cfgLoadUpdateOption(void)
     if (cfgOptionValid(cfgOptCompressType))
         compressTypePresent(compressTypeEnum(cfgOptionStrId(cfgOptCompressType)));
 
-    // Update compress-level default based on the compression type
+    // Update compress-level default based on the compression type. Also check that level range is valid per compression type.
-    if (cfgOptionValid(cfgOptCompressLevel) && cfgOptionSource(cfgOptCompressLevel) == cfgSourceDefault)
+    if (cfgOptionValid(cfgOptCompressLevel))
     {
-        cfgOptionSet(
+        const CompressType compressType = compressTypeEnum(cfgOptionStrId(cfgOptCompressType));
-            cfgOptCompressLevel, cfgSourceDefault,
+
-            VARINT64(compressLevelDefault(compressTypeEnum(cfgOptionStrId(cfgOptCompressType)))));
+        if (cfgOptionSource(cfgOptCompressLevel) == cfgSourceDefault)
+        {
+            cfgOptionSet(cfgOptCompressLevel, cfgSourceDefault, VARINT64(compressLevelDefault(compressType)));
+        }
+        else if (compressType != compressTypeNone)
+        {
+            if (cfgOptionInt(cfgOptCompressLevel) < compressLevelMin(compressType) ||
+                cfgOptionInt(cfgOptCompressLevel) > compressLevelMax(compressType))
+            {
+                THROW_FMT(
+                    OptionInvalidValueError,
+                    "'%d' is out of range for '" CFGOPT_COMPRESS_LEVEL "' option when '" CFGOPT_COMPRESS_TYPE "' option = '%s'",
+                    cfgOptionInt(cfgOptCompressLevel), strZ(strIdToStr(cfgOptionStrId(cfgOptCompressType))));
+            }
+        }
     }
 
     FUNCTION_LOG_RETURN_VOID();

src/config/parse.auto.c.inc

@@ -1374,18 +1374,6 @@ static const ParseRuleOption parseRuleOption[CFG_OPTION_TOTAL] =
         PARSE_RULE_OPTION_COMMAND_ROLE_ASYNC_VALID_LIST                                                        // opt/compress-level
         (                                                                                                      // opt/compress-level
             PARSE_RULE_OPTION_COMMAND(cfgCmdArchivePush)                                                       // opt/compress-level
-        ),                                                                                                     // opt/compress-level
-                                                                                                               // opt/compress-level
-        PARSE_RULE_OPTIONAL                                                                                    // opt/compress-level
-        (                                                                                                      // opt/compress-level
-            PARSE_RULE_OPTIONAL_GROUP                                                                          // opt/compress-level
-            (                                                                                                  // opt/compress-level
-                PARSE_RULE_OPTIONAL_ALLOW_RANGE                                                                // opt/compress-level
-                (                                                                                              // opt/compress-level
-                    PARSE_RULE_VAL_INT(parseRuleValInt0),                                                      // opt/compress-level
-                    PARSE_RULE_VAL_INT(parseRuleValInt9),                                                      // opt/compress-level
-                ),                                                                                             // opt/compress-level
-            ),                                                                                                 // opt/compress-level
         ),                                                                                                     // opt/compress-level
     ),                                                                                                         // opt/compress-level
     // -----------------------------------------------------------------------------------------------------------------------------

test/src/module/common/compressTest.c

@@ -196,6 +196,13 @@ testRun(void)
         TEST_ERROR(gzError(Z_VERSION_ERROR), FormatError, "zlib threw error: [-6] incompatible version");
         TEST_ERROR(gzError(999), AssertError, "zlib threw error: [999] unknown error");
 
+        // -------------------------------------------------------------------------------------------------------------------------
+        TEST_TITLE("compressLevelDefault(), compressLevelMin(), and compressLevelMax()");
+
+        TEST_RESULT_INT(compressLevelDefault(compressTypeGz), 6, "level default");
+        TEST_RESULT_INT(compressLevelMin(compressTypeGz), -1, "level default");
+        TEST_RESULT_INT(compressLevelMax(compressTypeGz), 9, "level default");
+
         // -------------------------------------------------------------------------------------------------------------------------
         TEST_TITLE("gzDecompressToLog() and gzCompressToLog()");
 
@@ -234,6 +241,13 @@ testRun(void)
         TEST_ERROR(bz2Error(BZ_CONFIG_ERROR), AssertError, "bz2 error: [-9] config error");
         TEST_ERROR(bz2Error(-999), AssertError, "bz2 error: [-999] unknown error");
 
+        // -------------------------------------------------------------------------------------------------------------------------
+        TEST_TITLE("compressLevelDefault(), compressLevelMin(), and compressLevelMax()");
+
+        TEST_RESULT_INT(compressLevelDefault(compressTypeBz2), 9, "level default");
+        TEST_RESULT_INT(compressLevelMin(compressTypeBz2), 1, "level default");
+        TEST_RESULT_INT(compressLevelMax(compressTypeBz2), 9, "level default");
+
         // -------------------------------------------------------------------------------------------------------------------------
         TEST_TITLE("bz2DecompressToLog() and bz2CompressToLog()");
 
@@ -265,6 +279,13 @@ testRun(void)
         TEST_RESULT_UINT(lz4Error(0), 0, "check success");
         TEST_ERROR(lz4Error((size_t)-2), FormatError, "lz4 error: [-2] ERROR_maxBlockSize_invalid");
 
+        // -------------------------------------------------------------------------------------------------------------------------
+        TEST_TITLE("compressLevelDefault(), compressLevelMin(), and compressLevelMax()");
+
+        TEST_RESULT_INT(compressLevelDefault(compressTypeLz4), 1, "level default");
+        TEST_RESULT_INT(compressLevelMin(compressTypeLz4), -5, "level default");
+        TEST_RESULT_INT(compressLevelMax(compressTypeLz4), 12, "level default");
+
         // -------------------------------------------------------------------------------------------------------------------------
         TEST_TITLE("lz4DecompressToLog() and lz4CompressToLog()");
 
@@ -303,6 +324,13 @@ testRun(void)
         TEST_RESULT_UINT(zstError(0), 0, "check success");
         TEST_ERROR(zstError((size_t)-12), FormatError, "zst error: [-12] Version not supported");
 
+        // -------------------------------------------------------------------------------------------------------------------------
+        TEST_TITLE("compressLevelDefault(), compressLevelMin(), and compressLevelMax()");
+
+        TEST_RESULT_INT(compressLevelDefault(compressTypeZst), 3, "level default");
+        TEST_RESULT_INT(compressLevelMin(compressTypeZst), -7, "level default");
+        TEST_RESULT_INT(compressLevelMax(compressTypeZst), 22, "level default");
+
         // -------------------------------------------------------------------------------------------------------------------------
         TEST_TITLE("zstDecompressToLog() and zstCompressToLog()");
 
@@ -380,12 +408,6 @@ testRun(void)
         TEST_ERROR(compressExtStrip(STRDEF("file"), compressTypeGz), FormatError, "'file' must have '.gz' extension");
         TEST_RESULT_STR_Z(compressExtStrip(STRDEF("file"), compressTypeNone), "file", "nothing to strip");
         TEST_RESULT_STR_Z(compressExtStrip(STRDEF("file.gz"), compressTypeGz), "file", "strip gz");
-
-        // -------------------------------------------------------------------------------------------------------------------------
-        TEST_TITLE("compressLevelDefault()");
-
-        TEST_RESULT_INT(compressLevelDefault(compressTypeNone), 0, "none level=0");
-        TEST_RESULT_INT(compressLevelDefault(compressTypeGz), 6, "gz level=6");
     }
 
     FUNCTION_HARNESS_RETURN_VOID();

test/src/module/config/loadTest.c

@@ -516,6 +516,40 @@ testRun(void)
         TEST_RESULT_INT(cfgOptionInt(cfgOptCompressLevel), 9, "compress-level=9");
         TEST_RESULT_BOOL(cfgOptionValid(cfgOptCompress), false, "compress is not valid");
 
+        // -------------------------------------------------------------------------------------------------------------------------
+        TEST_TITLE("error on invalid compress level");
+
+        argList = strLstNew();
+        hrnCfgArgRawZ(argList, cfgOptStanza, "db");
+        hrnCfgArgRawZ(argList, cfgOptCompressType, "gz");
+        hrnCfgArgRawZ(argList, cfgOptCompressLevel, "-2");
+
+        TEST_ERROR(
+            hrnCfgLoadP(cfgCmdArchivePush, argList), OptionInvalidValueError,
+            "'-2' is out of range for 'compress-level' option when 'compress-type' option = 'gz'");
+
+        argList = strLstNew();
+        hrnCfgArgRawZ(argList, cfgOptStanza, "db");
+        hrnCfgArgRawZ(argList, cfgOptCompressType, "gz");
+        hrnCfgArgRawZ(argList, cfgOptCompressLevel, "10");
+
+        TEST_ERROR(
+            hrnCfgLoadP(cfgCmdArchivePush, argList), OptionInvalidValueError,
+            "'10' is out of range for 'compress-level' option when 'compress-type' option = 'gz'");
+
+        // In practice level should not be used here but preserve the prior behavior in case something depends on it
+        // -------------------------------------------------------------------------------------------------------------------------
+        TEST_TITLE("do not check range when compress-type = none");
+
+        argList = strLstNew();
+        hrnCfgArgRawZ(argList, cfgOptStanza, "db");
+        hrnCfgArgRawZ(argList, cfgOptCompressType, "none");
+        hrnCfgArgRawZ(argList, cfgOptCompressLevel, "3");
+
+        HRN_CFG_LOAD(cfgCmdArchivePush, argList);
+        TEST_RESULT_UINT(cfgOptionStrId(cfgOptCompressType), CFGOPTVAL_COMPRESS_TYPE_NONE, "compress-type=none");
+        TEST_RESULT_INT(cfgOptionInt(cfgOptCompressLevel), 3, "compress-level=3");
+
         // -------------------------------------------------------------------------------------------------------------------------
         TEST_TITLE("warn when compress-type and compress both set");