Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1neCjm-0008By-7v for pgsql-hackers@arkaria.postgresql.org; Tue, 12 Apr 2022 09:23:02 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1neCjl-0005nK-6T for pgsql-hackers@arkaria.postgresql.org; Tue, 12 Apr 2022 09:23:01 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1neCjk-0005nA-AT for pgsql-hackers@lists.postgresql.org; Tue, 12 Apr 2022 09:23:00 +0000 Received: from wout2-smtp.messagingengine.com ([64.147.123.25]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1neCjh-00038f-BB for pgsql-hackers@lists.postgresql.org; Tue, 12 Apr 2022 09:22:59 +0000 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.west.internal (Postfix) with ESMTP id 0E6EB3200C14; Tue, 12 Apr 2022 05:22:54 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Tue, 12 Apr 2022 05:22:55 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=paquier.xyz; h= cc:cc:content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to; s=fm1; t=1649755374; x=1649841774; bh=WPM45xb0Yo zGO2ezo4FJeo0KFpGd/L6CoS5QdAXg+30=; b=GqPfvgO7jIb9kRoCIFPayGZICD V8w790aMPQbq06LhNrvUJ6d2BhTI4d2B2WNqtzP0QkfIWWovnvAGrfseokKXiP2j yUZK68incRgmAgEZXlHP2Q8vYduRs7uQ8NDkh22HcBUVMkyaJO8DR5A6VtFLxSHw z2Gw8n3RL3zrbUPy1iWZNLanFt2SvhRaWpLc4rGbPDqeg098Z6TiH0Udx1DKWW97 fBesOUzJ9T0vCGyxfqagT058UbnN6iNBIaPlm8TTQcBfdUoqd84vmkt3P1XjQr9J +U09LYtFo6FmVwLpX8HjHO/psevjhq2GI1C9200DhRSMjdKph3OzEJqdIthQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm3; t=1649755374; x= 1649841774; bh=WPM45xb0YozGO2ezo4FJeo0KFpGd/L6CoS5QdAXg+30=; b=Y At14SY61h5Bd3NXeZAYC5DYLPUtU0eTiOmvQ1bwbkv4z+VxJbZMvZ7n/mbCERzly ZEMLyvnMVi82LVenyqG1AJnsSjZR1ET5d+0mQ+xenjh6AH+IPeAZFf7K4t/lglIH 6zzU0YJNyaPARZqjeMDJqdSI3lUEioSAn9jmaPbKCFEsKCSYIGbM8mpTJshn5Ap9 RtzyS1lC4PH6dlGUwwCVtZ6DEVCzHV7i0oid1wSJNNCMIJOlE5kCN0gAfMugnR4l RvimRIB2nSXUCu+J1zG6Qkf8S2qHrvUwhIOkQheN3jh5DGBbsXTTcDG+4KmIeFto H4zZToOKkmhjuxjrksPzQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvvddrudekkedgudefucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne gfrhhlucfvnfffucdlfeehmdenucfjughrpeffhffvuffkfhggtggujgesghdtreertddt vdenucfhrhhomhepofhitghhrggvlhcurfgrqhhuihgvrhcuoehmihgthhgrvghlsehprg hquhhivghrrdighiiiqeenucggtffrrghtthgvrhhnpedvgeduuefhtdeuleettdevjeeh heeiveeuieegleetgeeljeelieeuieehgeevhfenucevlhhushhtvghrufhiiigvpedtne curfgrrhgrmhepmhgrihhlfhhrohhmpehmihgthhgrvghlsehprghquhhivghrrdighiii X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 12 Apr 2022 05:22:52 -0400 (EDT) Date: Tue, 12 Apr 2022 18:22:48 +0900 From: Michael Paquier To: gkokolatos@pm.me Cc: Postgres hackers , Robert Haas Subject: Re: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h} Message-ID: References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="UzwpHTlr9M9sOHB2" Content-Disposition: inline In-Reply-To: List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --UzwpHTlr9M9sOHB2 Content-Type: multipart/mixed; boundary="uvrtGjk8UF1B13yc" Content-Disposition: inline --uvrtGjk8UF1B13yc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Apr 11, 2022 at 12:46:02PM +0000, gkokolatos@pm.me wrote: > On Monday, April 11th, 2022 at 8:52 AM, Michael Paquier > wrote: >> - 0001 is a simple rename of backup_compression.{c,h} to >> compression.{c,h}, removing anything related to base backups from >> that. One extra reason behind this renaming is that I would like to >> use this infra for pg_dump, but that's material for 16~. >=20 > I agree with the design. If you permit me a couple of nitpicks regarding = naming. >=20 > +typedef enum pg_compress_algorithm > +{ > + PG_COMPRESSION_NONE, > + PG_COMPRESSION_GZIP, > + PG_COMPRESSION_LZ4, > + PG_COMPRESSION_ZSTD > +} pg_compress_algorithm; >=20 > Elsewhere in the codebase, (e.g. create_table.sgml, alter_table.sgml, > brin_tuple.c, detoast.c, toast_compression.c, tupdesc.c, gist.c to mentio= n a > few) variations of of the nomenclature "compression method" are used, like > 'VARDATA_COMPRESSED_GET_COMPRESS_METHOD' or 'InvalidCompressionMethod' et= c. I > feel that it would be nicer if we followed one naming rule for this and I > recommend to substitute algorithm for method throughout. Technically and as far as I know, both are correct and hold more or less the same meaning. pg_basebackup's code exposes algorithm in a more extended way, so I have just stuck to it for the internal variables and such. Perhaps we could rename the whole, but I see no strong reason to do that. > Last, even though it is not needed now, it will be helpful to have a > PG_COMPRESSION_INVALID in some scenarios. Though we can add it when we co= me to > it. Perhaps. There is no need for it yet, though. pg_dump would not need that, as well. >> - 0002 removes WalCompressionMethod, replacing it by >> pg_compress_algorithm as these are the same enums. Robert complained >> about the confusion that WalCompressionMethod could lead to as this >> could be used for the compression of data, and not only WAL. I have >> renamed some variables to be more consistent, while on it. >=20 > It looks good. If you choose to discard the comment regarding the use of > 'method' over 'algorithm' from above, can you please use the full word in= the > variable, e.g. 'wal_compress_algorithm' instead of 'wal_compress_algo'. I= can > not really explain it, the later reads a bit rude. Then again that may be= just > me. Thanks. I have been able to do an extra pass on 0001 and 0002, fixing those naming inconsistencies with "algo" vs "algorithm" that you and Robert have reported, and applied them. For 0003, I'll look at it later. Attached is a rebase with improvements about the variable names. -- Michael --uvrtGjk8UF1B13yc Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v2-0001-Rework-compression-options-of-pg_receivewal.patch" Content-Transfer-Encoding: quoted-printable =46rom 94850ac604402371135e85de27a5d9074a947d4b Mon Sep 17 00:00:00 2001 =46rom: Michael Paquier Date: Tue, 12 Apr 2022 18:14:28 +0900 Subject: [PATCH v2] Rework compression options of pg_receivewal Since babbbb5 and the introduction of LZ4 in pg_receivewal, the compression of archived WAL is controlled by two options: - --compression-method with "gzip", "none" or "lz4" as possible value. - --compress=3DN to specify a compression level, with a backward-incompatible change where a value of zero leads to a failure instead of no compression. This commit takes advantage of the previous refactoring done to rework the compression options of pg_receivewal: - --compression-method is removed. - --compress is extended to use the same grammar as pg_basebackup, as of a METHOD:DETAIL specification, where a METHOD is "gzip", "none" or "lz4" and a DETAIL is a comma-separated list of options, the only keyword supported is now "level" to control the compression level. If only an integer is specified as value of this option, "none" is implied for 0 and "gzip" is implied. So this brings back --compress to be backward-compatible with ~14, while still supporting LZ4. This has the advantage of centralizing the set of checks used by pg_basebackup. --- src/bin/pg_basebackup/pg_receivewal.c | 131 +++++++++++++------ src/bin/pg_basebackup/t/020_pg_receivewal.pl | 16 +-- doc/src/sgml/ref/pg_receivewal.sgml | 47 ++++--- 3 files changed, 121 insertions(+), 73 deletions(-) diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/= pg_receivewal.c index ede1d4648d..f942883332 100644 --- a/src/bin/pg_basebackup/pg_receivewal.c +++ b/src/bin/pg_basebackup/pg_receivewal.c @@ -57,6 +57,8 @@ static XLogRecPtr endpos =3D InvalidXLogRecPtr; =20 =20 static void usage(void); +static void parse_compress_options(char *option, char **algorithm, + char **detail); static DIR *get_destination_dir(char *dest_folder); static void close_destination_dir(DIR *dest_dir, char *dest_folder); static XLogRecPtr FindStreamingStart(uint32 *tli); @@ -90,9 +92,8 @@ usage(void) printf(_(" --synchronous flush write-ahead log immediately afte= r writing\n")); printf(_(" -v, --verbose output verbose messages\n")); printf(_(" -V, --version output version information, then exit\= n")); - printf(_(" --compression-method=3DMETHOD\n" - " method to compress logs\n")); - printf(_(" -Z, --compress=3D1-9 compress logs with given compression= level\n")); + printf(_(" -Z, --compress=3DMETHOD[:DETAIL]\n" + " compress as specified\n")); printf(_(" -?, --help show this help, then exit\n")); printf(_("\nConnection options:\n")); printf(_(" -d, --dbname=3DCONNSTR connection string\n")); @@ -108,6 +109,66 @@ usage(void) printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL); } =20 +/* + * Basic parsing of a value specified for -Z/--compress + * + * The parsing consists of a METHOD:DETAIL string fed later on to a more + * advanced routine in charge of proper validation checks. This only extr= acts + * METHOD and DETAIL. If only an integer is found, the method is implied = by + * the value specified. + */ +static void +parse_compress_options(char *option, char **algorithm, char **detail) +{ + char *sep; + char *endp; + long result; + + /* + * Check whether the compression specification consists of a bare integer. + * + * For backward-compatibility, assume "none" if the integer found is zero + * and "gzip" otherwise. + */ + result =3D strtol(option, &endp, 10); + if (*endp =3D=3D '\0') + { + if (result =3D=3D 0) + { + *algorithm =3D pstrdup("none"); + *detail =3D NULL; + } + else + { + *algorithm =3D pstrdup("gzip"); + *detail =3D pstrdup(option); + } + return; + } + + /* + * Check whether there is a compression detail following the algorithm + * name. + */ + sep =3D strchr(option, ':'); + if (sep =3D=3D NULL) + { + *algorithm =3D pstrdup(option); + *detail =3D NULL; + } + else + { + char *alg; + + alg =3D palloc((sep - option) + 1); + memcpy(alg, option, sep - option); + alg[sep - option] =3D '\0'; + + *algorithm =3D alg; + *detail =3D pstrdup(sep + 1); + } +} + /* * Check if the filename looks like a WAL file, letting caller know if this * WAL segment is partial and/or compressed. @@ -651,7 +712,6 @@ main(int argc, char **argv) {"if-not-exists", no_argument, NULL, 3}, {"synchronous", no_argument, NULL, 4}, {"no-sync", no_argument, NULL, 5}, - {"compression-method", required_argument, NULL, 6}, {NULL, 0, NULL, 0} }; =20 @@ -660,6 +720,10 @@ main(int argc, char **argv) char *db_name; uint32 hi, lo; + pg_compress_specification compression_spec; + char *compression_detail =3D NULL; + char *compression_algorithm_str =3D "none"; + char *error_detail =3D NULL; =20 pg_logging_init(argv[0]); progname =3D get_progname(argv[0]); @@ -728,9 +792,8 @@ main(int argc, char **argv) verbose++; break; case 'Z': - if (!option_parse_int(optarg, "-Z/--compress", 1, 9, - &compresslevel)) - exit(1); + parse_compress_options(optarg, &compression_algorithm_str, + &compression_detail); break; /* action */ case 1: @@ -748,17 +811,6 @@ main(int argc, char **argv) case 5: do_sync =3D false; break; - case 6: - if (pg_strcasecmp(optarg, "gzip") =3D=3D 0) - compression_algorithm =3D PG_COMPRESSION_GZIP; - else if (pg_strcasecmp(optarg, "lz4") =3D=3D 0) - compression_algorithm =3D PG_COMPRESSION_LZ4; - else if (pg_strcasecmp(optarg, "none") =3D=3D 0) - compression_algorithm =3D PG_COMPRESSION_NONE; - else - pg_fatal("invalid value \"%s\" for option %s", - optarg, "--compression-method"); - break; default: /* getopt_long already emitted a complaint */ pg_log_error_hint("Try \"%s --help\" for more information.", progname); @@ -810,24 +862,30 @@ main(int argc, char **argv) exit(1); } =20 - /* - * Compression-related options. + * Compression options */ + if (!parse_compress_algorithm(compression_algorithm_str, + &compression_algorithm)) + pg_fatal("unrecognized compression algorithm \"%s\"", + compression_algorithm_str); + + parse_compress_specification(compression_algorithm, compression_detail, + &compression_spec); + error_detail =3D validate_compress_specification(&compression_spec); + if (error_detail !=3D NULL) + pg_fatal("invalid compression specification: %s", + error_detail); + + /* Extract the compression level, if found in the specification */ + if ((compression_spec.options & PG_COMPRESSION_OPTION_LEVEL) !=3D 0) + compresslevel =3D compression_spec.level; + switch (compression_algorithm) { - case PG_COMPRESSION_NONE: - if (compresslevel !=3D 0) - { - pg_log_error("cannot use --compress with --compression-method=3D%s", - "none"); - pg_log_error_hint("Try \"%s --help\" for more information.", progname); - exit(1); - } - break; case PG_COMPRESSION_GZIP: #ifdef HAVE_LIBZ - if (compresslevel =3D=3D 0) + if ((compression_spec.options & PG_COMPRESSION_OPTION_LEVEL) =3D=3D 0) { pg_log_info("no value specified for --compress, switching to default"); compresslevel =3D Z_DEFAULT_COMPRESSION; @@ -837,16 +895,11 @@ main(int argc, char **argv) "gzip"); #endif break; + case PG_COMPRESSION_NONE: + /* nothing to do */ + break; case PG_COMPRESSION_LZ4: -#ifdef USE_LZ4 - if (compresslevel !=3D 0) - { - pg_log_error("cannot use --compress with --compression-method=3D%s", - "lz4"); - pg_log_error_hint("Try \"%s --help\" for more information.", progname); - exit(1); - } -#else +#ifndef USE_LZ4 pg_fatal("this build does not support compression with %s", "LZ4"); #endif diff --git a/src/bin/pg_basebackup/t/020_pg_receivewal.pl b/src/bin/pg_base= backup/t/020_pg_receivewal.pl index 8c38816b22..57e274cd34 100644 --- a/src/bin/pg_basebackup/t/020_pg_receivewal.pl +++ b/src/bin/pg_basebackup/t/020_pg_receivewal.pl @@ -35,11 +35,10 @@ $primary->command_fails( 'failure if --synchronous specified with --no-sync'); $primary->command_fails_like( [ - 'pg_receivewal', '-D', $stream_dir, '--compression-method', 'none', - '--compress', '1' + 'pg_receivewal', '-D', $stream_dir, '--compress', 'none:1', ], - qr/\Qpg_receivewal: error: cannot use --compress with --compression-metho= d=3Dnone/, - 'failure if --compress specified with --compression-method=3Dnone'); + qr/\Qpg_receivewal: error: invalid compression specification: compression= algorithm "none" does not accept a compression level/, + 'failure if --compress none:N (where N > 0)'); =20 # Slot creation and drop my $slot_name =3D 'test'; @@ -93,15 +92,12 @@ SKIP: chomp($nextlsn); $primary->psql('postgres', 'INSERT INTO test_table VALUES (2);'); =20 - # Note the trailing whitespace after the value of --compress, that is - # a valid value. $primary->command_ok( [ 'pg_receivewal', '-D', $stream_dir, '--verbose', '--endpos', $nextlsn, - '--compression-method', 'gzip', - '--compress', '1 ', + '--compress', 'gzip:1', '--no-loop' ], "streaming some WAL using ZLIB compression"); @@ -156,10 +152,10 @@ SKIP: 'pg_receivewal', '-D', $stream_dir, '--verbose', '--endpos', $nextlsn, - '--no-loop', '--compression-method', + '--no-loop', '--compress', 'lz4' ], - 'streaming some WAL using --compression-method=3Dlz4'); + 'streaming some WAL using --compress=3Dlz4'); =20 # Verify that the stored files are generated with their expected # names. diff --git a/doc/src/sgml/ref/pg_receivewal.sgml b/doc/src/sgml/ref/pg_rece= ivewal.sgml index b846213fb7..4fe9e1a874 100644 --- a/doc/src/sgml/ref/pg_receivewal.sgml +++ b/doc/src/sgml/ref/pg_receivewal.sgml @@ -263,15 +263,32 @@ PostgreSQL documentation =20 - + + + + - Enables compression of write-ahead logs using the specified method. - Supported values are gzip, lz4 - (if PostgreSQL was compiled with - ), and none. + Enables compression of write-ahead logs. + + + The compression method can be set to gzip, + lz4 (if PostgreSQL + was compiled with ) or + none for no compression. + A compression detail string can optionally be specified. If the + detail string is an integer, it specifies the compression level. + Otherwise, it should be a comma-separated list of items, each of t= he + form keyword or keyword=3Dvalue. + Currently, the only supported keyword is level. + + + If no compression level is specified, the default compression level + will be used. If only a level is specified without mentioning an + algorithm, gzip compression will be used if the + level is greater than 0, and no compression will be used if the le= vel + is 0. - The suffix .gz will automatically be added to all filenames when using gzip, and the suffix @@ -279,24 +296,6 @@ PostgreSQL documentation - - - - - - - Specifies the compression level (1 through - 9, 1 being worst compression - and 9 being best compression) for WAL segments - compressed with gzip. - - - - This option requires to be - specified with gzip. - - - =20 --=20 2.35.1 --uvrtGjk8UF1B13yc-- --UzwpHTlr9M9sOHB2 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmJVROgACgkQnvQgOdby QH1GHA//Yl4HSPXJC/1vfu7DxMbdGqEutk3v5zOP8U+C9/LpaSAmPBkEJcL7HZn9 B0q22nwpjVjN4k8B1tTlXBCaOfROO9o0YMZ67TMv+PGRIv6yQEUC39ZSPiQmvD0W LUVuCqHV5HNMH8t4aoFmryC3ixQWHphk9tGXH0moLNr5GMdL3gfxu2VH7k7Mvm9n nYRW2QVkGGXIesQcHEJ/kTnnnLkfFY+ZPmtu54VCj/ETItOWClg5c66erevhJiKO FcA7Vpg4CwHpf8FBO6ycZLaRCd2BYFGvdYhlba7vqeDx/6dvcwOkoIDYBx94RJkv h/Wcm1QfEWPZBnJn8pj1n2vBNKvxav1zuu2ctJ/K46OnXBGsRlOm2SHRcDpg1BP8 xeFRCCO0Zysbetq/jjLPJLe+DjhNYZmvQBdojHdqHhFoB6VPhT6YYPZesHF+ShUd qNCImZeVH/qu5piSEh8XfYjYydOrVUnI9ppA8hhJwB+r/60Of43+rFXjfrkNV/2w mO673BVYKrGyvVCJ38yOPT0+qpLovQl2wOwWB5P/Bscwx161mXgYWuH7UaTNS6FW 7UJ+DGiP1hdxxVsBli2lPIi3+eHNstPBH3qm3JoljiBKNiOyMZJankNvEbh/JGu2 torJQwHMokoeHoqZU/MOf2vkGPnD5Nk5Dh8XtV7D39l/C8zKSBY= =UVj7 -----END PGP SIGNATURE----- --UzwpHTlr9M9sOHB2--