Date: Tue, 10 Jan 2023 17:26:34 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: pgsql-hackers@postgresql.org
Cc: Michael Paquier <michael@paquier.xyz>,
	Robert Haas <robertmhaas@gmail.com>,
	Matthias van de Meent <boekewurm+postgres@gmail.com>
Subject: wal_compression = method:level
Message-ID: <20230110232634.GF9837@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="ULyIDA2m8JTe+TiX"
Content-Disposition: inline
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: <https://www.postgresql.org/message-id/20230110232634.GF9837%40telsasoft.com>
Precedence: bulk


--ULyIDA2m8JTe+TiX
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Is it desirable to support specifying a level ?

Maybe there's a concern about using high compression levels, but 
I'll start by asking if the feature is wanted at all.

Previous discussion at: 20210614012412.GA31772@telsasoft.com

--ULyIDA2m8JTe+TiX
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="0001-Use-GUC-hooks-to-support-compression-level.patch"

From cb30e17cf19fffa370a887d28d6d7e683d588b71 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 14 Mar 2021 17:12:07 -0500
Subject: [PATCH] Use GUC hooks to support compression:level..

..which is useful for zstd, but less so for lz4.

TODO:

windows, pglz, zlib case
---
 doc/src/sgml/config.sgml                    |   6 +-
 src/backend/access/transam/xlog.c           |  20 ++++
 src/backend/access/transam/xloginsert.c     |   6 +-
 src/backend/access/transam/xlogreader.c     | 124 ++++++++++++++++++++
 src/backend/utils/misc/guc_tables.c         |  39 ++----
 src/common/compression.c                    |   3 -
 src/include/access/xlog.h                   |   6 +
 src/include/access/xlogreader.h             |   2 +
 src/include/utils/guc_hooks.h               |   3 +
 src/test/regress/expected/compression.out   |  19 +++
 src/test/regress/expected/compression_1.out |  19 +++
 src/test/regress/sql/compression.sql        |  10 ++
 12 files changed, 220 insertions(+), 37 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 77574e2d4ec..7b34ed630d5 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3161,7 +3161,7 @@ include_dir 'conf.d'
      </varlistentry>
 
      <varlistentry id="guc-wal-compression" xreflabel="wal_compression">
-      <term><varname>wal_compression</varname> (<type>enum</type>)
+      <term><varname>wal_compression</varname> (<type>string</type>)
       <indexterm>
        <primary><varname>wal_compression</varname> configuration parameter</primary>
       </indexterm>
@@ -3169,7 +3169,7 @@ include_dir 'conf.d'
       <listitem>
        <para>
         This parameter enables compression of WAL using the specified
-        compression method.
+        compression method and optional compression level.
         When enabled, the <productname>PostgreSQL</productname>
         server compresses full page images written to WAL when
         <xref linkend="guc-full-page-writes"/> is on or during a base backup.
@@ -3179,6 +3179,8 @@ include_dir 'conf.d'
         was compiled with <option>--with-lz4</option>) and
         <literal>zstd</literal> (if <productname>PostgreSQL</productname>
         was compiled with <option>--with-zstd</option>).
+        A compression level may optionally be specified, by appending the level
+        number after a colon (<literal>:</literal>)
         The default value is <literal>off</literal>.
         Only superusers and users with the appropriate <literal>SET</literal>
         privilege can change this setting.
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 74bd1f5fbe2..e2d81129549 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -125,6 +125,8 @@ bool		EnableHotStandby = false;
 bool		fullPageWrites = true;
 bool		wal_log_hints = false;
 int			wal_compression = WAL_COMPRESSION_NONE;
+char		*wal_compression_string = "no"; /* Overwritten by GUC */
+int			wal_compression_level = 1;
 char	   *wal_consistency_checking_string = NULL;
 bool	   *wal_consistency_checking = NULL;
 bool		wal_init_zero = true;
@@ -8118,6 +8120,24 @@ assign_xlog_sync_method(int new_sync_method, void *extra)
 	}
 }
 
+bool
+check_wal_compression(char **newval, void **extra, GucSource source)
+{
+	int tmp;
+	if (get_compression_level(*newval, &tmp) != -1)
+		return true;
+
+	return false;
+}
+
+/* Parse the GUC into integers for wal_compression and wal_compression_level */
+void
+assign_wal_compression(const char *newval, void *extra)
+{
+	wal_compression = get_compression_level(newval, &wal_compression_level);
+	Assert(wal_compression >= 0);
+}
+
 
 /*
  * Issue appropriate kind of fsync (if any) for an XLOG output file.
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 47b6d16eaef..93003744ed0 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -906,8 +906,8 @@ XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length,
 
 		case WAL_COMPRESSION_LZ4:
 #ifdef USE_LZ4
-			len = LZ4_compress_default(source, dest, orig_len,
-									   COMPRESS_BUFSIZE);
+			len = LZ4_compress_fast(source, dest, orig_len,
+									   COMPRESS_BUFSIZE, wal_compression_level);
 			if (len <= 0)
 				len = -1;		/* failure */
 #else
@@ -918,7 +918,7 @@ XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length,
 		case WAL_COMPRESSION_ZSTD:
 #ifdef USE_ZSTD
 			len = ZSTD_compress(dest, COMPRESS_BUFSIZE, source, orig_len,
-								ZSTD_CLEVEL_DEFAULT);
+								wal_compression_level);
 			if (ZSTD_isError(len))
 				len = -1;		/* failure */
 #else
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index a61b4a99219..728ca9f18c4 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -17,7 +17,9 @@
  */
 #include "postgres.h"
 
+#include <limits.h>
 #include <unistd.h>
+
 #ifdef USE_LZ4
 #include <lz4.h>
 #endif
@@ -30,6 +32,7 @@
 #include "access/xlogreader.h"
 #include "access/xlogrecord.h"
 #include "catalog/pg_control.h"
+#include "common/compression.h"
 #include "common/pg_lzcompress.h"
 #include "replication/origin.h"
 
@@ -56,6 +59,30 @@ static void ResetDecoder(XLogReaderState *state);
 static void WALOpenSegmentInit(WALOpenSegment *seg, WALSegmentContext *segcxt,
 							   int segsize, const char *waldir);
 
+static const struct {
+	char *name;
+	enum WalCompression compress_id; /* The internal ID */
+} wal_compression_options[] = {
+	{"pglz", WAL_COMPRESSION_PGLZ},
+
+#ifdef USE_LZ4
+	{"lz4", WAL_COMPRESSION_LZ4},
+#endif
+
+#ifdef USE_ZSTD
+	{"zstd", WAL_COMPRESSION_ZSTD},
+#endif
+
+	{"on", WAL_COMPRESSION_PGLZ},
+	{"off", WAL_COMPRESSION_NONE},
+	{"true", WAL_COMPRESSION_PGLZ},
+	{"false", WAL_COMPRESSION_NONE},
+	{"yes", WAL_COMPRESSION_PGLZ},
+	{"no", WAL_COMPRESSION_NONE},
+	{"1", WAL_COMPRESSION_PGLZ},
+	{"0", WAL_COMPRESSION_NONE},
+};
+
 /* size of the buffer allocated for error message. */
 #define MAX_ERRORMSG_LEN 1000
 
@@ -2035,6 +2062,103 @@ XLogRecGetBlockData(XLogReaderState *record, uint8 block_id, Size *len)
 	}
 }
 
+/*
+ * Return the wal compression ID, or -1 if the input is
+ * invalid/unrecognized/unsupported.
+ * The compression level is stored in *level.
+ */
+int
+get_compression_level(const char *in, int *level)
+{
+	pg_compress_algorithm alg;
+	pg_compress_specification algdetail;
+	char *algorithm, *detail;
+	char *error_detail;
+	int compress_id = -1;
+
+	/* Parse the algorithm and any detail suffix */
+	parse_compress_options(in, &algorithm, &detail);
+
+	/* Try to find the WAL_COMPRESSION_* value for the algorithm */
+	for (int idx = 0; idx < lengthof(wal_compression_options); ++idx)
+	{
+		if (strcmp(wal_compression_options[idx].name, algorithm) == 0)
+			compress_id = wal_compression_options[idx].compress_id;
+	}
+
+	if (!parse_compress_algorithm(algorithm, &alg))
+	{
+		/*
+		 * The compression algorithm wasn't a commonly-used algorithm name, but
+		 * may be a supported wal_compression: pglz/on/off/etc.
+		 */
+
+		if (compress_id >= 0 && detail != NULL)
+		{
+			/*
+			 * Algorithms which aren't known by the generic parser don't
+			 * support compression level or other options.
+			 */
+#ifndef FRONTEND
+			GUC_check_errdetail("Compression algorithm does not support options: %s",
+					algorithm);
+#endif
+			return -1;
+		}
+
+		*level = 0; /* ignored */
+		return compress_id;
+	}
+	else
+	{
+		/* Show a useful error if the wal compression is known, but not supported */
+		if ((alg == PG_COMPRESSION_ZSTD || alg != PG_COMPRESSION_LZ4) &&
+			compress_id == -1)
+
+		{
+#ifndef FRONTEND
+			GUC_check_errdetail("Compression algorithm is not supported by this build: %s",
+					algorithm);
+#endif
+			return -1;
+		}
+
+		/*
+		 * If the algorithm *is* known, then parse its compression level.  Note
+		 * that "none" is a supported compression algorithm, but not a valid
+		 * option for wal_compression
+		 */
+		parse_compress_specification(alg, detail, &algdetail);
+		error_detail = validate_compress_specification(&algdetail);
+		if (error_detail != NULL)
+		{
+#ifndef FRONTEND
+			GUC_check_errdetail("Could not parse compression detail: %s",
+					error_detail);
+#endif
+			return -1;
+		}
+
+		*level = algdetail.level;
+		if (algdetail.options != 0)
+		{
+#ifndef FRONTEND
+			GUC_check_errdetail("Compression options other than level= are not supported by wal_compression: %s",
+					detail);
+#endif
+			return -1;
+		}
+	}
+
+	return compress_id;
+
+	/*
+	 * gzip or anything parsed by the generic parser which isn't a valid
+	 * wal_compression value will get here.
+	 */
+	return -1;
+}
+
 /*
  * Restore a full-page image from a backup block attached to an XLOG record.
  *
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 5025e80f89d..4a956ea11a3 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -433,25 +433,6 @@ static const struct config_enum_entry default_toast_compression_options[] = {
 	{NULL, 0, false}
 };
 
-static const struct config_enum_entry wal_compression_options[] = {
-	{"pglz", WAL_COMPRESSION_PGLZ, false},
-#ifdef USE_LZ4
-	{"lz4", WAL_COMPRESSION_LZ4, false},
-#endif
-#ifdef USE_ZSTD
-	{"zstd", WAL_COMPRESSION_ZSTD, false},
-#endif
-	{"on", WAL_COMPRESSION_PGLZ, false},
-	{"off", WAL_COMPRESSION_NONE, false},
-	{"true", WAL_COMPRESSION_PGLZ, true},
-	{"false", WAL_COMPRESSION_NONE, true},
-	{"yes", WAL_COMPRESSION_PGLZ, true},
-	{"no", WAL_COMPRESSION_NONE, true},
-	{"1", WAL_COMPRESSION_PGLZ, true},
-	{"0", WAL_COMPRESSION_NONE, true},
-	{NULL, 0, false}
-};
-
 /*
  * Options for enum values stored in other modules
  */
@@ -4491,6 +4472,16 @@ struct config_string ConfigureNamesString[] =
 		check_wal_consistency_checking, assign_wal_consistency_checking, NULL
 	},
 
+	{
+		{"wal_compression", PGC_SUSET, WAL_SETTINGS,
+			gettext_noop("Compresses full-page writes written in WAL file with specified method."),
+			NULL
+		},
+		&wal_compression_string,
+		"no",
+		check_wal_compression, assign_wal_compression, NULL
+	},
+
 	{
 		{"jit_provider", PGC_POSTMASTER, CLIENT_CONN_PRELOAD,
 			gettext_noop("JIT provider to use."),
@@ -4749,16 +4740,6 @@ struct config_enum ConfigureNamesEnum[] =
 		NULL, NULL, NULL
 	},
 
-	{
-		{"wal_compression", PGC_SUSET, WAL_SETTINGS,
-			gettext_noop("Compresses full-page writes written in WAL file with specified method."),
-			NULL
-		},
-		&wal_compression,
-		WAL_COMPRESSION_NONE, wal_compression_options,
-		NULL, NULL, NULL
-	},
-
 	{
 		{"wal_level", PGC_POSTMASTER, WAL_SETTINGS,
 			gettext_noop("Sets the level of information written to the WAL."),
diff --git a/src/common/compression.c b/src/common/compression.c
index 2d3e56b4d62..a2e7abb6a47 100644
--- a/src/common/compression.c
+++ b/src/common/compression.c
@@ -357,8 +357,6 @@ validate_compress_specification(pg_compress_specification *spec)
 	return NULL;
 }
 
-#ifdef FRONTEND
-
 /*
  * Basic parsing of a value specified through a command-line option, commonly
  * -Z/--compress.
@@ -418,4 +416,3 @@ parse_compress_options(const char *option, char **algorithm, char **detail)
 		*detail = pstrdup(sep + 1);
 	}
 }
-#endif							/* FRONTEND */
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index cfe5409738c..115257e52bb 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -16,6 +16,8 @@
 #include "datatype/timestamp.h"
 #include "lib/stringinfo.h"
 #include "nodes/pg_list.h"
+#include "storage/fd.h"
+#include "utils/guc.h"
 
 
 /* Sync methods */
@@ -54,6 +56,10 @@ extern PGDLLIMPORT int wal_decode_buffer_size;
 
 extern PGDLLIMPORT int CheckPointSegments;
 
+extern PGDLLIMPORT char *wal_compression_string;
+extern PGDLLIMPORT int wal_compression;
+extern PGDLLIMPORT int wal_compression_level;
+
 /* Archive modes */
 typedef enum ArchiveMode
 {
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index da64f99f0b3..f371fbc0e85 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -440,4 +440,6 @@ extern bool XLogRecGetBlockTagExtended(XLogReaderState *record, uint8 block_id,
 									   BlockNumber *blknum,
 									   Buffer *prefetch_buffer);
 
+extern int get_compression_level(const char *in, int *level);
+
 #endif							/* XLOGREADER_H */
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index aeb3663071f..fa87d8ae12c 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -155,4 +155,7 @@ extern bool check_wal_consistency_checking(char **newval, void **extra,
 extern void assign_wal_consistency_checking(const char *newval, void *extra);
 extern void assign_xlog_sync_method(int new_sync_method, void *extra);
 
+extern bool check_wal_compression(char **newval, void **extra, GucSource source);
+extern void assign_wal_compression(const char *newval, void *extra);
+
 #endif							/* GUC_HOOKS_H */
diff --git a/src/test/regress/expected/compression.out b/src/test/regress/expected/compression.out
index 4c997e2602f..72735f4f91a 100644
--- a/src/test/regress/expected/compression.out
+++ b/src/test/regress/expected/compression.out
@@ -360,3 +360,22 @@ ALTER TABLE badcompresstbl ALTER a SET COMPRESSION I_Do_Not_Exist_Compression; -
 ERROR:  invalid compression method "i_do_not_exist_compression"
 DROP TABLE badcompresstbl;
 \set HIDE_TOAST_COMPRESSION true
+-- pglz doesn't accept a compression level:
+SET wal_compression = 'pglz:1';
+ERROR:  invalid value for parameter "wal_compression": "pglz:1"
+DETAIL:  Compression algorithm does not support options: pglz
+-- none and gzip aren't supported wal_compression algorithms:
+SET wal_compression = 'none';
+ERROR:  invalid value for parameter "wal_compression": "none"
+DETAIL:  Compression algorithm is not supported by this build: none
+SET wal_compression = 'gzip';
+ERROR:  invalid value for parameter "wal_compression": "gzip"
+DETAIL:  Compression algorithm is not supported by this build: gzip
+SET wal_compression = 'gzip:1';
+ERROR:  invalid value for parameter "wal_compression": "gzip:1"
+DETAIL:  Compression algorithm is not supported by this build: gzip
+-- wrong spelling with a dash instead of a colon:
+SET wal_compression = 'zstd-1';
+ERROR:  invalid value for parameter "wal_compression": "zstd-1"
+SET wal_compression = 'lz4-1';
+ERROR:  invalid value for parameter "wal_compression": "lz4-1"
diff --git a/src/test/regress/expected/compression_1.out b/src/test/regress/expected/compression_1.out
index c0a47646eb2..e9c053cb56b 100644
--- a/src/test/regress/expected/compression_1.out
+++ b/src/test/regress/expected/compression_1.out
@@ -354,3 +354,22 @@ ALTER TABLE badcompresstbl ALTER a SET COMPRESSION I_Do_Not_Exist_Compression; -
 ERROR:  invalid compression method "i_do_not_exist_compression"
 DROP TABLE badcompresstbl;
 \set HIDE_TOAST_COMPRESSION true
+-- pglz doesn't accept a compression level:
+SET wal_compression = 'pglz:1';
+ERROR:  invalid value for parameter "wal_compression": "pglz:1"
+DETAIL:  Compression algorithm does not support options: pglz
+-- none and gzip aren't supported wal_compression algorithms:
+SET wal_compression = 'none';
+ERROR:  invalid value for parameter "wal_compression": "none"
+DETAIL:  Compression algorithm is not supported by this build: none
+SET wal_compression = 'gzip';
+ERROR:  invalid value for parameter "wal_compression": "gzip"
+DETAIL:  Compression algorithm is not supported by this build: gzip
+SET wal_compression = 'gzip:1';
+ERROR:  invalid value for parameter "wal_compression": "gzip:1"
+DETAIL:  Compression algorithm is not supported by this build: gzip
+-- wrong spelling with a dash instead of a colon:
+SET wal_compression = 'zstd-1';
+ERROR:  invalid value for parameter "wal_compression": "zstd-1"
+SET wal_compression = 'lz4-1';
+ERROR:  invalid value for parameter "wal_compression": "lz4-1"
diff --git a/src/test/regress/sql/compression.sql b/src/test/regress/sql/compression.sql
index 86332dcc510..5a3cb324220 100644
--- a/src/test/regress/sql/compression.sql
+++ b/src/test/regress/sql/compression.sql
@@ -151,3 +151,13 @@ ALTER TABLE badcompresstbl ALTER a SET COMPRESSION I_Do_Not_Exist_Compression; -
 DROP TABLE badcompresstbl;
 
 \set HIDE_TOAST_COMPRESSION true
+
+-- pglz doesn't accept a compression level:
+SET wal_compression = 'pglz:1';
+-- none and gzip aren't supported wal_compression algorithms:
+SET wal_compression = 'none';
+SET wal_compression = 'gzip';
+SET wal_compression = 'gzip:1';
+-- wrong spelling with a dash instead of a colon:
+SET wal_compression = 'zstd-1';
+SET wal_compression = 'lz4-1';
-- 
2.25.1


--ULyIDA2m8JTe+TiX--