public inbox for [email protected]
help / color / mirror / Atom feedFixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}
11+ messages / 5 participants
[nested] [flat]
* Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}
@ 2022-04-11 06:52 Michael Paquier <[email protected]>
0 siblings, 2 replies; 11+ messages in thread
From: Michael Paquier @ 2022-04-11 06:52 UTC (permalink / raw)
To: Postgres hackers <[email protected]>; +Cc: Robert Haas <[email protected]>; Georgios <[email protected]>
Hi all,
(Added Robert and Georgios in CC:)
Since babbbb5 and the introduction of LZ4, I have reworked the way
compression is controlled for pg_receivewal, with two options:
- --compress-method, settable to "gzip", "none" or "lz4".
- --compress, to pass down a compression level, where the allowed
range is 1-9. If passing down 0, we'd get an error rather than
implying no compression, contrary to what we did in ~14.
I initially thought that this was fine as-is, but then Robert and
others have worked on client/server compression for pg_basebackup,
introducing a much better design with centralized APIs where one can
use METHOD:DETAIL for as compression value, where DETAIL is a
comma-separated list of keyword=value (keyword = "level" or
"workers"), with centralized checks and an extensible design.
This is something I think we had better fix before beta1, because now
we have binaries that use an inconsistent set of options. So,
attached is a patch set aimed at rework this option set from the
ground, taking advantage of the recent work done by Robert and others
for pg_basebackup:
- 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~.
- 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.
- 0003 is the actual option rework for pg_receivewal. This takes
advantage of 0001, leading to the result of removing --compress-method
and replacing it with --compress, taking care of the backward
compatibility problems for an integer value, aka 0 implies no
compression and val > 0 implies gzip. One bonus reason to switch to
that is that this would make the addition of zstd for pg_receivewal
easier in the future.
I am going to add an open item for this stuff. Comments or thoughts?
Thanks,
--
Michael
Attachments:
[text/x-diff] v1-0001-Rename-backup_compression.-c-h-to-compression.-c-.patch (32.5K, ../../[email protected]/2-v1-0001-Rename-backup_compression.-c-h-to-compression.-c-.patch)
download | inline diff:
From ccef39021a254c3ca4d0a380f138bf47e9b9b9c7 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Mon, 11 Apr 2022 13:50:18 +0900
Subject: [PATCH v1 1/3] Rename backup_compression.{c,h} to compression.{c,h}
Compression option handling (level, algorithm or even workers) can be
used across several parts of the system and not only base backups, with
at least pg_receivewal and pg_dump. Structures, objects and routines
are renamed in consequence, to remove the concept of base backups from
that.
---
src/include/common/backup_compression.h | 46 -------------
src/include/common/compression.h | 46 +++++++++++++
src/include/replication/basebackup_sink.h | 8 +--
src/backend/replication/basebackup.c | 30 ++++-----
src/backend/replication/basebackup_gzip.c | 4 +-
src/backend/replication/basebackup_lz4.c | 4 +-
src/backend/replication/basebackup_zstd.c | 10 +--
src/common/Makefile | 2 +-
.../{backup_compression.c => compression.c} | 66 +++++++++----------
src/bin/pg_basebackup/bbstreamer.h | 8 +--
src/bin/pg_basebackup/bbstreamer_gzip.c | 4 +-
src/bin/pg_basebackup/bbstreamer_lz4.c | 4 +-
src/bin/pg_basebackup/bbstreamer_zstd.c | 6 +-
src/bin/pg_basebackup/nls.mk | 2 +-
src/bin/pg_basebackup/pg_basebackup.c | 44 ++++++-------
src/tools/msvc/Mkvcbuild.pm | 2 +-
16 files changed, 143 insertions(+), 143 deletions(-)
delete mode 100644 src/include/common/backup_compression.h
create mode 100644 src/include/common/compression.h
rename src/common/{backup_compression.c => compression.c} (80%)
diff --git a/src/include/common/backup_compression.h b/src/include/common/backup_compression.h
deleted file mode 100644
index 6a0ecaa99c..0000000000
--- a/src/include/common/backup_compression.h
+++ /dev/null
@@ -1,46 +0,0 @@
-/*-------------------------------------------------------------------------
- *
- * backup_compression.h
- *
- * Shared definitions for backup compression methods and specifications.
- *
- * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
- *
- * IDENTIFICATION
- * src/common/backup_compression.h
- *-------------------------------------------------------------------------
- */
-
-#ifndef BACKUP_COMPRESSION_H
-#define BACKUP_COMPRESSION_H
-
-typedef enum bc_algorithm
-{
- BACKUP_COMPRESSION_NONE,
- BACKUP_COMPRESSION_GZIP,
- BACKUP_COMPRESSION_LZ4,
- BACKUP_COMPRESSION_ZSTD
-} bc_algorithm;
-
-#define BACKUP_COMPRESSION_OPTION_LEVEL (1 << 0)
-#define BACKUP_COMPRESSION_OPTION_WORKERS (1 << 1)
-
-typedef struct bc_specification
-{
- bc_algorithm algorithm;
- unsigned options; /* OR of BACKUP_COMPRESSION_OPTION constants */
- int level;
- int workers;
- char *parse_error; /* NULL if parsing was OK, else message */
-} bc_specification;
-
-extern bool parse_bc_algorithm(char *name, bc_algorithm *algorithm);
-extern const char *get_bc_algorithm_name(bc_algorithm algorithm);
-
-extern void parse_bc_specification(bc_algorithm algorithm,
- char *specification,
- bc_specification *result);
-
-extern char *validate_bc_specification(bc_specification *);
-
-#endif
diff --git a/src/include/common/compression.h b/src/include/common/compression.h
new file mode 100644
index 0000000000..b54deb9ead
--- /dev/null
+++ b/src/include/common/compression.h
@@ -0,0 +1,46 @@
+/*-------------------------------------------------------------------------
+ *
+ * compression.h
+ *
+ * Shared definitions for compression methods and specifications.
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * src/common/compression.h
+ *-------------------------------------------------------------------------
+ */
+
+#ifndef PG_COMPRESSION_H
+#define PG_COMPRESSION_H
+
+typedef enum pg_compress_algorithm
+{
+ PG_COMPRESSION_NONE,
+ PG_COMPRESSION_GZIP,
+ PG_COMPRESSION_LZ4,
+ PG_COMPRESSION_ZSTD
+} pg_compress_algorithm;
+
+#define PG_COMPRESSION_OPTION_LEVEL (1 << 0)
+#define PG_COMPRESSION_OPTION_WORKERS (1 << 1)
+
+typedef struct pg_compress_specification
+{
+ pg_compress_algorithm algorithm;
+ unsigned options; /* OR of PG_COMPRESSION_OPTION constants */
+ int level;
+ int workers;
+ char *parse_error; /* NULL if parsing was OK, else message */
+} pg_compress_specification;
+
+extern bool parse_compress_algorithm(char *name, pg_compress_algorithm *algorithm);
+extern const char *get_compress_algorithm_name(pg_compress_algorithm algorithm);
+
+extern void parse_compress_specification(pg_compress_algorithm algorithm,
+ char *specification,
+ pg_compress_specification *result);
+
+extern char *validate_compress_specification(pg_compress_specification *);
+
+#endif
diff --git a/src/include/replication/basebackup_sink.h b/src/include/replication/basebackup_sink.h
index 654df28576..36278cac14 100644
--- a/src/include/replication/basebackup_sink.h
+++ b/src/include/replication/basebackup_sink.h
@@ -27,7 +27,7 @@
#define BASEBACKUP_SINK_H
#include "access/xlog_internal.h"
-#include "common/backup_compression.h"
+#include "common/compression.h"
#include "nodes/pg_list.h"
/* Forward declarations. */
@@ -284,9 +284,9 @@ extern void bbsink_forward_cleanup(bbsink *sink);
/* Constructors for various types of sinks. */
extern bbsink *bbsink_copystream_new(bool send_to_client);
-extern bbsink *bbsink_gzip_new(bbsink *next, bc_specification *);
-extern bbsink *bbsink_lz4_new(bbsink *next, bc_specification *);
-extern bbsink *bbsink_zstd_new(bbsink *next, bc_specification *);
+extern bbsink *bbsink_gzip_new(bbsink *next, pg_compress_specification *);
+extern bbsink *bbsink_lz4_new(bbsink *next, pg_compress_specification *);
+extern bbsink *bbsink_zstd_new(bbsink *next, pg_compress_specification *);
extern bbsink *bbsink_progress_new(bbsink *next, bool estimate_backup_size);
extern bbsink *bbsink_server_new(bbsink *next, char *pathname);
extern bbsink *bbsink_throttle_new(bbsink *next, uint32 maxrate);
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 27e4152446..c555b2f2ed 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -17,7 +17,7 @@
#include <time.h>
#include "access/xlog_internal.h" /* for pg_start/stop_backup */
-#include "common/backup_compression.h"
+#include "common/compression.h"
#include "common/file_perm.h"
#include "commands/defrem.h"
#include "lib/stringinfo.h"
@@ -68,8 +68,8 @@ typedef struct
bool use_copytblspc;
BaseBackupTargetHandle *target_handle;
backup_manifest_option manifest;
- bc_algorithm compression;
- bc_specification compression_specification;
+ pg_compress_algorithm compression;
+ pg_compress_specification specification;
pg_checksum_type manifest_checksum_type;
} basebackup_options;
@@ -691,8 +691,8 @@ parse_basebackup_options(List *options, basebackup_options *opt)
MemSet(opt, 0, sizeof(*opt));
opt->manifest = MANIFEST_OPTION_NO;
opt->manifest_checksum_type = CHECKSUM_TYPE_CRC32C;
- opt->compression = BACKUP_COMPRESSION_NONE;
- opt->compression_specification.algorithm = BACKUP_COMPRESSION_NONE;
+ opt->compression = PG_COMPRESSION_NONE;
+ opt->specification.algorithm = PG_COMPRESSION_NONE;
foreach(lopt, options)
{
@@ -859,7 +859,7 @@ parse_basebackup_options(List *options, basebackup_options *opt)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("duplicate option \"%s\"", defel->defname)));
- if (!parse_bc_algorithm(optval, &opt->compression))
+ if (!parse_compress_algorithm(optval, &opt->compression))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("unrecognized compression algorithm \"%s\"",
@@ -924,10 +924,10 @@ parse_basebackup_options(List *options, basebackup_options *opt)
{
char *error_detail;
- parse_bc_specification(opt->compression, compression_detail_str,
- &opt->compression_specification);
+ parse_compress_specification(opt->compression, compression_detail_str,
+ &opt->specification);
error_detail =
- validate_bc_specification(&opt->compression_specification);
+ validate_compress_specification(&opt->specification);
if (error_detail != NULL)
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
@@ -978,12 +978,12 @@ SendBaseBackup(BaseBackupCmd *cmd)
sink = bbsink_throttle_new(sink, opt.maxrate);
/* Set up server-side compression, if client requested it */
- if (opt.compression == BACKUP_COMPRESSION_GZIP)
- sink = bbsink_gzip_new(sink, &opt.compression_specification);
- else if (opt.compression == BACKUP_COMPRESSION_LZ4)
- sink = bbsink_lz4_new(sink, &opt.compression_specification);
- else if (opt.compression == BACKUP_COMPRESSION_ZSTD)
- sink = bbsink_zstd_new(sink, &opt.compression_specification);
+ if (opt.compression == PG_COMPRESSION_GZIP)
+ sink = bbsink_gzip_new(sink, &opt.specification);
+ else if (opt.compression == PG_COMPRESSION_LZ4)
+ sink = bbsink_lz4_new(sink, &opt.specification);
+ else if (opt.compression == PG_COMPRESSION_ZSTD)
+ sink = bbsink_zstd_new(sink, &opt.specification);
/* Set up progress reporting. */
sink = bbsink_progress_new(sink, opt.progress);
diff --git a/src/backend/replication/basebackup_gzip.c b/src/backend/replication/basebackup_gzip.c
index e4df57b121..b68afef86c 100644
--- a/src/backend/replication/basebackup_gzip.c
+++ b/src/backend/replication/basebackup_gzip.c
@@ -59,7 +59,7 @@ const bbsink_ops bbsink_gzip_ops = {
* Create a new basebackup sink that performs gzip compression.
*/
bbsink *
-bbsink_gzip_new(bbsink *next, bc_specification *compress)
+bbsink_gzip_new(bbsink *next, pg_compress_specification *compress)
{
#ifndef HAVE_LIBZ
ereport(ERROR,
@@ -72,7 +72,7 @@ bbsink_gzip_new(bbsink *next, bc_specification *compress)
Assert(next != NULL);
- if ((compress->options & BACKUP_COMPRESSION_OPTION_LEVEL) == 0)
+ if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) == 0)
compresslevel = Z_DEFAULT_COMPRESSION;
else
{
diff --git a/src/backend/replication/basebackup_lz4.c b/src/backend/replication/basebackup_lz4.c
index 48929321a4..310285d4c5 100644
--- a/src/backend/replication/basebackup_lz4.c
+++ b/src/backend/replication/basebackup_lz4.c
@@ -59,7 +59,7 @@ const bbsink_ops bbsink_lz4_ops = {
* Create a new basebackup sink that performs lz4 compression.
*/
bbsink *
-bbsink_lz4_new(bbsink *next, bc_specification *compress)
+bbsink_lz4_new(bbsink *next, pg_compress_specification *compress)
{
#ifndef USE_LZ4
ereport(ERROR,
@@ -72,7 +72,7 @@ bbsink_lz4_new(bbsink *next, bc_specification *compress)
Assert(next != NULL);
- if ((compress->options & BACKUP_COMPRESSION_OPTION_LEVEL) == 0)
+ if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) == 0)
compresslevel = 0;
else
{
diff --git a/src/backend/replication/basebackup_zstd.c b/src/backend/replication/basebackup_zstd.c
index f6876f4811..97fb25b99f 100644
--- a/src/backend/replication/basebackup_zstd.c
+++ b/src/backend/replication/basebackup_zstd.c
@@ -26,7 +26,7 @@ typedef struct bbsink_zstd
bbsink base;
/* Compression options */
- bc_specification *compress;
+ pg_compress_specification *compress;
ZSTD_CCtx *cctx;
ZSTD_outBuffer zstd_outBuf;
@@ -58,7 +58,7 @@ const bbsink_ops bbsink_zstd_ops = {
* Create a new basebackup sink that performs zstd compression.
*/
bbsink *
-bbsink_zstd_new(bbsink *next, bc_specification *compress)
+bbsink_zstd_new(bbsink *next, pg_compress_specification *compress)
{
#ifndef USE_ZSTD
ereport(ERROR,
@@ -90,13 +90,13 @@ bbsink_zstd_begin_backup(bbsink *sink)
bbsink_zstd *mysink = (bbsink_zstd *) sink;
size_t output_buffer_bound;
size_t ret;
- bc_specification *compress = mysink->compress;
+ pg_compress_specification *compress = mysink->compress;
mysink->cctx = ZSTD_createCCtx();
if (!mysink->cctx)
elog(ERROR, "could not create zstd compression context");
- if ((compress->options & BACKUP_COMPRESSION_OPTION_LEVEL) != 0)
+ if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) != 0)
{
ret = ZSTD_CCtx_setParameter(mysink->cctx, ZSTD_c_compressionLevel,
compress->level);
@@ -105,7 +105,7 @@ bbsink_zstd_begin_backup(bbsink *sink)
compress->level, ZSTD_getErrorName(ret));
}
- if ((compress->options & BACKUP_COMPRESSION_OPTION_WORKERS) != 0)
+ if ((compress->options & PG_COMPRESSION_OPTION_WORKERS) != 0)
{
/*
* On older versions of libzstd, this option does not exist, and trying
diff --git a/src/common/Makefile b/src/common/Makefile
index f627349835..e9af7346c9 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -47,9 +47,9 @@ LIBS += $(PTHREAD_LIBS)
OBJS_COMMON = \
archive.o \
- backup_compression.o \
base64.o \
checksum_helper.o \
+ compression.o \
config_info.o \
controldata_utils.o \
d2s.o \
diff --git a/src/common/backup_compression.c b/src/common/compression.c
similarity index 80%
rename from src/common/backup_compression.c
rename to src/common/compression.c
index 867f2f2eb5..632fae6144 100644
--- a/src/common/backup_compression.c
+++ b/src/common/compression.c
@@ -1,8 +1,8 @@
/*-------------------------------------------------------------------------
*
- * backup_compression.c
+ * compression.c
*
- * Shared code for backup compression methods and specifications.
+ * Shared code for compression methods and specifications.
*
* A compression specification specifies the parameters that should be used
* when performing compression with a specific algorithm. The simplest
@@ -12,12 +12,12 @@
* Otherwise, a compression specification is a comma-separated list of items,
* each having the form keyword or keyword=value.
*
- * Currently, the only supported keyword is "level".
+ * Currently, the only supported keywords are "level" and "workers".
*
* Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
*
* IDENTIFICATION
- * src/common/backup_compression.c
+ * src/common/compression.c
*-------------------------------------------------------------------------
*/
@@ -27,26 +27,26 @@
#include "postgres_fe.h"
#endif
-#include "common/backup_compression.h"
+#include "common/compression.h"
static int expect_integer_value(char *keyword, char *value,
- bc_specification *result);
+ pg_compress_specification *result);
/*
* Look up a compression algorithm by name. Returns true and sets *algorithm
* if the name is recognized. Otherwise returns false.
*/
bool
-parse_bc_algorithm(char *name, bc_algorithm *algorithm)
+parse_compress_algorithm(char *name, pg_compress_algorithm *algorithm)
{
if (strcmp(name, "none") == 0)
- *algorithm = BACKUP_COMPRESSION_NONE;
+ *algorithm = PG_COMPRESSION_NONE;
else if (strcmp(name, "gzip") == 0)
- *algorithm = BACKUP_COMPRESSION_GZIP;
+ *algorithm = PG_COMPRESSION_GZIP;
else if (strcmp(name, "lz4") == 0)
- *algorithm = BACKUP_COMPRESSION_LZ4;
+ *algorithm = PG_COMPRESSION_LZ4;
else if (strcmp(name, "zstd") == 0)
- *algorithm = BACKUP_COMPRESSION_ZSTD;
+ *algorithm = PG_COMPRESSION_ZSTD;
else
return false;
return true;
@@ -57,17 +57,17 @@ parse_bc_algorithm(char *name, bc_algorithm *algorithm)
* algorithm.
*/
const char *
-get_bc_algorithm_name(bc_algorithm algorithm)
+get_compress_algorithm_name(pg_compress_algorithm algorithm)
{
switch (algorithm)
{
- case BACKUP_COMPRESSION_NONE:
+ case PG_COMPRESSION_NONE:
return "none";
- case BACKUP_COMPRESSION_GZIP:
+ case PG_COMPRESSION_GZIP:
return "gzip";
- case BACKUP_COMPRESSION_LZ4:
+ case PG_COMPRESSION_LZ4:
return "lz4";
- case BACKUP_COMPRESSION_ZSTD:
+ case PG_COMPRESSION_ZSTD:
return "zstd";
/* no default, to provoke compiler warnings if values are added */
}
@@ -88,12 +88,12 @@ get_bc_algorithm_name(bc_algorithm algorithm)
* Note, however, even if there's no parse error, the string might not make
* sense: e.g. for gzip, level=12 is not sensible, but it does parse OK.
*
- * Use validate_bc_specification() to find out whether a compression
+ * Use validate_compress_specification() to find out whether a compression
* specification is semantically sensible.
*/
void
-parse_bc_specification(bc_algorithm algorithm, char *specification,
- bc_specification *result)
+parse_compress_specification(pg_compress_algorithm algorithm, char *specification,
+ pg_compress_specification *result)
{
int bare_level;
char *bare_level_endp;
@@ -113,7 +113,7 @@ parse_bc_specification(bc_algorithm algorithm, char *specification,
if (specification != bare_level_endp && *bare_level_endp == '\0')
{
result->level = bare_level;
- result->options |= BACKUP_COMPRESSION_OPTION_LEVEL;
+ result->options |= PG_COMPRESSION_OPTION_LEVEL;
return;
}
@@ -175,12 +175,12 @@ parse_bc_specification(bc_algorithm algorithm, char *specification,
if (strcmp(keyword, "level") == 0)
{
result->level = expect_integer_value(keyword, value, result);
- result->options |= BACKUP_COMPRESSION_OPTION_LEVEL;
+ result->options |= PG_COMPRESSION_OPTION_LEVEL;
}
else if (strcmp(keyword, "workers") == 0)
{
result->workers = expect_integer_value(keyword, value, result);
- result->options |= BACKUP_COMPRESSION_OPTION_WORKERS;
+ result->options |= PG_COMPRESSION_OPTION_WORKERS;
}
else
result->parse_error =
@@ -215,7 +215,7 @@ parse_bc_specification(bc_algorithm algorithm, char *specification,
* and return -1.
*/
static int
-expect_integer_value(char *keyword, char *value, bc_specification *result)
+expect_integer_value(char *keyword, char *value, pg_compress_specification *result)
{
int ivalue;
char *ivalue_endp;
@@ -247,7 +247,7 @@ expect_integer_value(char *keyword, char *value, bc_specification *result)
* compression method.
*/
char *
-validate_bc_specification(bc_specification *spec)
+validate_compress_specification(pg_compress_specification *spec)
{
/* If it didn't even parse OK, it's definitely no good. */
if (spec->parse_error != NULL)
@@ -258,24 +258,24 @@ validate_bc_specification(bc_specification *spec)
* a compression level and that the level is within the legal range for
* the algorithm.
*/
- if ((spec->options & BACKUP_COMPRESSION_OPTION_LEVEL) != 0)
+ if ((spec->options & PG_COMPRESSION_OPTION_LEVEL) != 0)
{
int min_level = 1;
int max_level;
- if (spec->algorithm == BACKUP_COMPRESSION_GZIP)
+ if (spec->algorithm == PG_COMPRESSION_GZIP)
max_level = 9;
- else if (spec->algorithm == BACKUP_COMPRESSION_LZ4)
+ else if (spec->algorithm == PG_COMPRESSION_LZ4)
max_level = 12;
- else if (spec->algorithm == BACKUP_COMPRESSION_ZSTD)
+ else if (spec->algorithm == PG_COMPRESSION_ZSTD)
max_level = 22;
else
return psprintf(_("compression algorithm \"%s\" does not accept a compression level"),
- get_bc_algorithm_name(spec->algorithm));
+ get_compress_algorithm_name(spec->algorithm));
if (spec->level < min_level || spec->level > max_level)
return psprintf(_("compression algorithm \"%s\" expects a compression level between %d and %d"),
- get_bc_algorithm_name(spec->algorithm),
+ get_compress_algorithm_name(spec->algorithm),
min_level, max_level);
}
@@ -283,11 +283,11 @@ validate_bc_specification(bc_specification *spec)
* Of the compression algorithms that we currently support, only zstd
* allows parallel workers.
*/
- if ((spec->options & BACKUP_COMPRESSION_OPTION_WORKERS) != 0 &&
- (spec->algorithm != BACKUP_COMPRESSION_ZSTD))
+ if ((spec->options & PG_COMPRESSION_OPTION_WORKERS) != 0 &&
+ (spec->algorithm != PG_COMPRESSION_ZSTD))
{
return psprintf(_("compression algorithm \"%s\" does not accept a worker count"),
- get_bc_algorithm_name(spec->algorithm));
+ get_compress_algorithm_name(spec->algorithm));
}
return NULL;
diff --git a/src/bin/pg_basebackup/bbstreamer.h b/src/bin/pg_basebackup/bbstreamer.h
index dfa3f77af4..29b48ef54d 100644
--- a/src/bin/pg_basebackup/bbstreamer.h
+++ b/src/bin/pg_basebackup/bbstreamer.h
@@ -22,7 +22,7 @@
#ifndef BBSTREAMER_H
#define BBSTREAMER_H
-#include "common/backup_compression.h"
+#include "common/compression.h"
#include "lib/stringinfo.h"
#include "pqexpbuffer.h"
@@ -201,17 +201,17 @@ bbstreamer_buffer_until(bbstreamer *streamer, const char **data, int *len,
*/
extern bbstreamer *bbstreamer_plain_writer_new(char *pathname, FILE *file);
extern bbstreamer *bbstreamer_gzip_writer_new(char *pathname, FILE *file,
- bc_specification *compress);
+ pg_compress_specification *compress);
extern bbstreamer *bbstreamer_extractor_new(const char *basepath,
const char *(*link_map) (const char *),
void (*report_output_file) (const char *));
extern bbstreamer *bbstreamer_gzip_decompressor_new(bbstreamer *next);
extern bbstreamer *bbstreamer_lz4_compressor_new(bbstreamer *next,
- bc_specification *compress);
+ pg_compress_specification *compress);
extern bbstreamer *bbstreamer_lz4_decompressor_new(bbstreamer *next);
extern bbstreamer *bbstreamer_zstd_compressor_new(bbstreamer *next,
- bc_specification *compress);
+ pg_compress_specification *compress);
extern bbstreamer *bbstreamer_zstd_decompressor_new(bbstreamer *next);
extern bbstreamer *bbstreamer_tar_parser_new(bbstreamer *next);
extern bbstreamer *bbstreamer_tar_terminator_new(bbstreamer *next);
diff --git a/src/bin/pg_basebackup/bbstreamer_gzip.c b/src/bin/pg_basebackup/bbstreamer_gzip.c
index 1ab7ee6ea9..b3bfcd62ac 100644
--- a/src/bin/pg_basebackup/bbstreamer_gzip.c
+++ b/src/bin/pg_basebackup/bbstreamer_gzip.c
@@ -77,7 +77,7 @@ const bbstreamer_ops bbstreamer_gzip_decompressor_ops = {
*/
bbstreamer *
bbstreamer_gzip_writer_new(char *pathname, FILE *file,
- bc_specification *compress)
+ pg_compress_specification *compress)
{
#ifdef HAVE_LIBZ
bbstreamer_gzip_writer *streamer;
@@ -107,7 +107,7 @@ bbstreamer_gzip_writer_new(char *pathname, FILE *file,
pg_fatal("could not open output file: %m");
}
- if ((compress->options & BACKUP_COMPRESSION_OPTION_LEVEL) != 0 &&
+ if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) != 0 &&
gzsetparams(streamer->gzfile, compress->level,
Z_DEFAULT_STRATEGY) != Z_OK)
pg_fatal("could not set compression level %d: %s",
diff --git a/src/bin/pg_basebackup/bbstreamer_lz4.c b/src/bin/pg_basebackup/bbstreamer_lz4.c
index 2f75ba5602..6070a72cdb 100644
--- a/src/bin/pg_basebackup/bbstreamer_lz4.c
+++ b/src/bin/pg_basebackup/bbstreamer_lz4.c
@@ -67,7 +67,7 @@ const bbstreamer_ops bbstreamer_lz4_decompressor_ops = {
* blocks.
*/
bbstreamer *
-bbstreamer_lz4_compressor_new(bbstreamer *next, bc_specification *compress)
+bbstreamer_lz4_compressor_new(bbstreamer *next, pg_compress_specification *compress)
{
#ifdef USE_LZ4
bbstreamer_lz4_frame *streamer;
@@ -88,7 +88,7 @@ bbstreamer_lz4_compressor_new(bbstreamer *next, bc_specification *compress)
prefs = &streamer->prefs;
memset(prefs, 0, sizeof(LZ4F_preferences_t));
prefs->frameInfo.blockSizeID = LZ4F_max256KB;
- if ((compress->options & BACKUP_COMPRESSION_OPTION_LEVEL) != 0)
+ if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) != 0)
prefs->compressionLevel = compress->level;
ctxError = LZ4F_createCompressionContext(&streamer->cctx, LZ4F_VERSION);
diff --git a/src/bin/pg_basebackup/bbstreamer_zstd.c b/src/bin/pg_basebackup/bbstreamer_zstd.c
index a5167e9fea..8a839145a6 100644
--- a/src/bin/pg_basebackup/bbstreamer_zstd.c
+++ b/src/bin/pg_basebackup/bbstreamer_zstd.c
@@ -63,7 +63,7 @@ const bbstreamer_ops bbstreamer_zstd_decompressor_ops = {
* blocks.
*/
bbstreamer *
-bbstreamer_zstd_compressor_new(bbstreamer *next, bc_specification *compress)
+bbstreamer_zstd_compressor_new(bbstreamer *next, pg_compress_specification *compress)
{
#ifdef USE_ZSTD
bbstreamer_zstd_frame *streamer;
@@ -85,7 +85,7 @@ bbstreamer_zstd_compressor_new(bbstreamer *next, bc_specification *compress)
pg_fatal("could not create zstd compression context");
/* Set compression level, if specified */
- if ((compress->options & BACKUP_COMPRESSION_OPTION_LEVEL) != 0)
+ if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) != 0)
{
ret = ZSTD_CCtx_setParameter(streamer->cctx, ZSTD_c_compressionLevel,
compress->level);
@@ -95,7 +95,7 @@ bbstreamer_zstd_compressor_new(bbstreamer *next, bc_specification *compress)
}
/* Set # of workers, if specified */
- if ((compress->options & BACKUP_COMPRESSION_OPTION_WORKERS) != 0)
+ if ((compress->options & PG_COMPRESSION_OPTION_WORKERS) != 0)
{
/*
* On older versions of libzstd, this option does not exist, and
diff --git a/src/bin/pg_basebackup/nls.mk b/src/bin/pg_basebackup/nls.mk
index cf8570ce18..298a6e02f0 100644
--- a/src/bin/pg_basebackup/nls.mk
+++ b/src/bin/pg_basebackup/nls.mk
@@ -14,7 +14,7 @@ GETTEXT_FILES = $(FRONTEND_COMMON_GETTEXT_FILES) \
receivelog.c \
streamutil.c \
walmethods.c \
- ../../common/backup_compression.c \
+ ../../common/compression.c \
../../common/fe_memutils.c \
../../common/file_utils.c \
../../fe_utils/recovery_gen.c
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 65dcfff0a0..2df86f746b 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -29,7 +29,7 @@
#include "access/xlog_internal.h"
#include "bbstreamer.h"
-#include "common/backup_compression.h"
+#include "common/compression.h"
#include "common/file_perm.h"
#include "common/file_utils.h"
#include "common/logging.h"
@@ -58,7 +58,7 @@ typedef struct TablespaceList
typedef struct ArchiveStreamState
{
int tablespacenum;
- bc_specification *compress;
+ pg_compress_specification *compress;
bbstreamer *streamer;
bbstreamer *manifest_inject_streamer;
PQExpBuffer manifest_buffer;
@@ -198,7 +198,7 @@ static bbstreamer *CreateBackupStreamer(char *archive_name, char *spclocation,
bbstreamer **manifest_inject_streamer_p,
bool is_recovery_guc_supported,
bool expect_unterminated_tarfile,
- bc_specification *compress);
+ pg_compress_specification *compress);
static void ReceiveArchiveStreamChunk(size_t r, char *copybuf,
void *callback_data);
static char GetCopyDataByte(size_t r, char *copybuf, size_t *cursor);
@@ -207,7 +207,7 @@ static uint64 GetCopyDataUInt64(size_t r, char *copybuf, size_t *cursor);
static void GetCopyDataEnd(size_t r, char *copybuf, size_t cursor);
static void ReportCopyDataParseError(size_t r, char *copybuf);
static void ReceiveTarFile(PGconn *conn, char *archive_name, char *spclocation,
- bool tablespacenum, bc_specification *compress);
+ bool tablespacenum, pg_compress_specification *compress);
static void ReceiveTarCopyChunk(size_t r, char *copybuf, void *callback_data);
static void ReceiveBackupManifest(PGconn *conn);
static void ReceiveBackupManifestChunk(size_t r, char *copybuf,
@@ -217,7 +217,7 @@ static void ReceiveBackupManifestInMemoryChunk(size_t r, char *copybuf,
void *callback_data);
static void BaseBackup(char *compression_algorithm, char *compression_detail,
CompressionLocation compressloc,
- bc_specification *client_compress);
+ pg_compress_specification *client_compress);
static bool reached_end_position(XLogRecPtr segendpos, uint32 timeline,
bool segment_finished);
@@ -1077,7 +1077,7 @@ CreateBackupStreamer(char *archive_name, char *spclocation,
bbstreamer **manifest_inject_streamer_p,
bool is_recovery_guc_supported,
bool expect_unterminated_tarfile,
- bc_specification *compress)
+ pg_compress_specification *compress)
{
bbstreamer *streamer = NULL;
bbstreamer *manifest_inject_streamer = NULL;
@@ -1193,23 +1193,23 @@ CreateBackupStreamer(char *archive_name, char *spclocation,
archive_file = NULL;
}
- if (compress->algorithm == BACKUP_COMPRESSION_NONE)
+ if (compress->algorithm == PG_COMPRESSION_NONE)
streamer = bbstreamer_plain_writer_new(archive_filename,
archive_file);
- else if (compress->algorithm == BACKUP_COMPRESSION_GZIP)
+ else if (compress->algorithm == PG_COMPRESSION_GZIP)
{
strlcat(archive_filename, ".gz", sizeof(archive_filename));
streamer = bbstreamer_gzip_writer_new(archive_filename,
archive_file, compress);
}
- else if (compress->algorithm == BACKUP_COMPRESSION_LZ4)
+ else if (compress->algorithm == PG_COMPRESSION_LZ4)
{
strlcat(archive_filename, ".lz4", sizeof(archive_filename));
streamer = bbstreamer_plain_writer_new(archive_filename,
archive_file);
streamer = bbstreamer_lz4_compressor_new(streamer, compress);
}
- else if (compress->algorithm == BACKUP_COMPRESSION_ZSTD)
+ else if (compress->algorithm == PG_COMPRESSION_ZSTD)
{
strlcat(archive_filename, ".zst", sizeof(archive_filename));
streamer = bbstreamer_plain_writer_new(archive_filename,
@@ -1288,7 +1288,7 @@ CreateBackupStreamer(char *archive_name, char *spclocation,
* manifest if present - as a single COPY stream.
*/
static void
-ReceiveArchiveStream(PGconn *conn, bc_specification *compress)
+ReceiveArchiveStream(PGconn *conn, pg_compress_specification *compress)
{
ArchiveStreamState state;
@@ -1604,7 +1604,7 @@ ReportCopyDataParseError(size_t r, char *copybuf)
*/
static void
ReceiveTarFile(PGconn *conn, char *archive_name, char *spclocation,
- bool tablespacenum, bc_specification *compress)
+ bool tablespacenum, pg_compress_specification *compress)
{
WriteTarState state;
bbstreamer *manifest_inject_streamer;
@@ -1758,7 +1758,7 @@ ReceiveBackupManifestInMemoryChunk(size_t r, char *copybuf,
static void
BaseBackup(char *compression_algorithm, char *compression_detail,
- CompressionLocation compressloc, bc_specification *client_compress)
+ CompressionLocation compressloc, pg_compress_specification *client_compress)
{
PGresult *res;
char *sysidentifier;
@@ -2025,11 +2025,11 @@ BaseBackup(char *compression_algorithm, char *compression_detail,
if (verbose)
pg_log_info("starting background WAL receiver");
- if (client_compress->algorithm == BACKUP_COMPRESSION_GZIP)
+ if (client_compress->algorithm == PG_COMPRESSION_GZIP)
{
wal_compress_method = COMPRESSION_GZIP;
wal_compress_level =
- (client_compress->options & BACKUP_COMPRESSION_OPTION_LEVEL)
+ (client_compress->options & PG_COMPRESSION_OPTION_LEVEL)
!= 0 ? client_compress->level : 0;
}
else
@@ -2316,7 +2316,7 @@ main(int argc, char **argv)
char *compression_algorithm = "none";
char *compression_detail = NULL;
CompressionLocation compressloc = COMPRESS_LOCATION_UNSPECIFIED;
- bc_specification client_compress;
+ pg_compress_specification client_compress;
pg_logging_init(argv[0]);
progname = get_progname(argv[0]);
@@ -2558,15 +2558,15 @@ main(int argc, char **argv)
*/
if (compressloc == COMPRESS_LOCATION_CLIENT)
{
- bc_algorithm alg;
+ pg_compress_algorithm alg;
char *error_detail;
- if (!parse_bc_algorithm(compression_algorithm, &alg))
+ if (!parse_compress_algorithm(compression_algorithm, &alg))
pg_fatal("unrecognized compression algorithm \"%s\"",
compression_algorithm);
- parse_bc_specification(alg, compression_detail, &client_compress);
- error_detail = validate_bc_specification(&client_compress);
+ parse_compress_specification(alg, compression_detail, &client_compress);
+ error_detail = validate_compress_specification(&client_compress);
if (error_detail != NULL)
pg_fatal("invalid compression specification: %s",
error_detail);
@@ -2574,7 +2574,7 @@ main(int argc, char **argv)
else
{
Assert(compressloc == COMPRESS_LOCATION_SERVER);
- client_compress.algorithm = BACKUP_COMPRESSION_NONE;
+ client_compress.algorithm = PG_COMPRESSION_NONE;
client_compress.options = 0;
}
@@ -2593,7 +2593,7 @@ main(int argc, char **argv)
* Client-side compression doesn't make sense unless tar format is in use.
*/
if (format == 'p' && compressloc == COMPRESS_LOCATION_CLIENT &&
- client_compress.algorithm != BACKUP_COMPRESSION_NONE)
+ client_compress.algorithm != PG_COMPRESSION_NONE)
{
pg_log_error("only tar mode backups can be compressed");
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index de8676d339..46904fee0e 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -124,7 +124,7 @@ sub mkvcbuild
}
our @pgcommonallfiles = qw(
- archive.c backup_compression.c base64.c checksum_helper.c
+ archive.c base64.c checksum_helper.c compression.c
config_info.c controldata_utils.c d2s.c encnames.c exec.c
f2s.c file_perm.c file_utils.c hashfn.c ip.c jsonapi.c
keywords.c kwlookup.c link-canary.c md5_common.c
--
2.35.1
[text/x-diff] v1-0002-Replace-WalCompressionMethod-by-common-pg_compres.patch (24.2K, ../../[email protected]/3-v1-0002-Replace-WalCompressionMethod-by-common-pg_compres.patch)
download | inline diff:
From 2a96bd519139a2eaebc993c58229386f2e525996 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Mon, 11 Apr 2022 13:28:43 +0900
Subject: [PATCH v1 2/3] Replace WalCompressionMethod by common
pg_compress_algorithm
The same structure with the same set of elements, exists in
compression.h, so let's make use of the centralized version. Some of
the variables based previously on WalCompressionMethod are renamed to
stick better with the new naming.
---
src/bin/pg_basebackup/pg_basebackup.c | 18 ++---
src/bin/pg_basebackup/pg_receivewal.c | 44 ++++++------
src/bin/pg_basebackup/receivelog.c | 2 +-
src/bin/pg_basebackup/walmethods.c | 96 +++++++++++++--------------
src/bin/pg_basebackup/walmethods.h | 16 ++---
5 files changed, 84 insertions(+), 92 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 2df86f746b..09ae27089d 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -520,7 +520,7 @@ typedef struct
char xlog[MAXPGPATH]; /* directory or tarfile depending on mode */
char *sysidentifier;
int timeline;
- WalCompressionMethod wal_compress_method;
+ pg_compress_algorithm wal_compress_algo;
int wal_compress_level;
} logstreamer_param;
@@ -550,11 +550,11 @@ LogStreamerMain(logstreamer_param *param)
stream.replication_slot = replication_slot;
if (format == 'p')
stream.walmethod = CreateWalDirectoryMethod(param->xlog,
- COMPRESSION_NONE, 0,
+ PG_COMPRESSION_NONE, 0,
stream.do_sync);
else
stream.walmethod = CreateWalTarMethod(param->xlog,
- param->wal_compress_method,
+ param->wal_compress_algo,
param->wal_compress_level,
stream.do_sync);
@@ -602,7 +602,7 @@ LogStreamerMain(logstreamer_param *param)
*/
static void
StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier,
- WalCompressionMethod wal_compress_method,
+ pg_compress_algorithm wal_compress_algo,
int wal_compress_level)
{
logstreamer_param *param;
@@ -613,7 +613,7 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier,
param = pg_malloc0(sizeof(logstreamer_param));
param->timeline = timeline;
param->sysidentifier = sysidentifier;
- param->wal_compress_method = wal_compress_method;
+ param->wal_compress_algo = wal_compress_algo;
param->wal_compress_level = wal_compress_level;
/* Convert the starting position */
@@ -2019,7 +2019,7 @@ BaseBackup(char *compression_algorithm, char *compression_detail,
*/
if (includewal == STREAM_WAL)
{
- WalCompressionMethod wal_compress_method;
+ pg_compress_algorithm wal_compress_algo;
int wal_compress_level;
if (verbose)
@@ -2027,19 +2027,19 @@ BaseBackup(char *compression_algorithm, char *compression_detail,
if (client_compress->algorithm == PG_COMPRESSION_GZIP)
{
- wal_compress_method = COMPRESSION_GZIP;
+ wal_compress_algo = PG_COMPRESSION_GZIP;
wal_compress_level =
(client_compress->options & PG_COMPRESSION_OPTION_LEVEL)
!= 0 ? client_compress->level : 0;
}
else
{
- wal_compress_method = COMPRESSION_NONE;
+ wal_compress_algo = PG_COMPRESSION_NONE;
wal_compress_level = 0;
}
StartLogStreamer(xlogstart, starttli, sysidentifier,
- wal_compress_method, wal_compress_level);
+ wal_compress_algo, wal_compress_level);
}
if (serverMajor >= 1500)
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 23e04741fd..b371bad644 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -52,7 +52,7 @@ static bool do_drop_slot = false;
static bool do_sync = true;
static bool synchronous = false;
static char *replication_slot = NULL;
-static WalCompressionMethod compression_method = COMPRESSION_NONE;
+static pg_compress_algorithm compression_algo = PG_COMPRESSION_NONE;
static XLogRecPtr endpos = InvalidXLogRecPtr;
@@ -114,7 +114,7 @@ usage(void)
*/
static bool
is_xlogfilename(const char *filename, bool *ispartial,
- WalCompressionMethod *wal_compression_method)
+ pg_compress_algorithm *wal_compression_algo)
{
size_t fname_len = strlen(filename);
size_t xlog_pattern_len = strspn(filename, "0123456789ABCDEF");
@@ -127,7 +127,7 @@ is_xlogfilename(const char *filename, bool *ispartial,
if (fname_len == XLOG_FNAME_LEN)
{
*ispartial = false;
- *wal_compression_method = COMPRESSION_NONE;
+ *wal_compression_algo = PG_COMPRESSION_NONE;
return true;
}
@@ -136,7 +136,7 @@ is_xlogfilename(const char *filename, bool *ispartial,
strcmp(filename + XLOG_FNAME_LEN, ".gz") == 0)
{
*ispartial = false;
- *wal_compression_method = COMPRESSION_GZIP;
+ *wal_compression_algo = PG_COMPRESSION_GZIP;
return true;
}
@@ -145,7 +145,7 @@ is_xlogfilename(const char *filename, bool *ispartial,
strcmp(filename + XLOG_FNAME_LEN, ".lz4") == 0)
{
*ispartial = false;
- *wal_compression_method = COMPRESSION_LZ4;
+ *wal_compression_algo = PG_COMPRESSION_LZ4;
return true;
}
@@ -154,7 +154,7 @@ is_xlogfilename(const char *filename, bool *ispartial,
strcmp(filename + XLOG_FNAME_LEN, ".partial") == 0)
{
*ispartial = true;
- *wal_compression_method = COMPRESSION_NONE;
+ *wal_compression_algo = PG_COMPRESSION_NONE;
return true;
}
@@ -163,7 +163,7 @@ is_xlogfilename(const char *filename, bool *ispartial,
strcmp(filename + XLOG_FNAME_LEN, ".gz.partial") == 0)
{
*ispartial = true;
- *wal_compression_method = COMPRESSION_GZIP;
+ *wal_compression_algo = PG_COMPRESSION_GZIP;
return true;
}
@@ -172,7 +172,7 @@ is_xlogfilename(const char *filename, bool *ispartial,
strcmp(filename + XLOG_FNAME_LEN, ".lz4.partial") == 0)
{
*ispartial = true;
- *wal_compression_method = COMPRESSION_LZ4;
+ *wal_compression_algo = PG_COMPRESSION_LZ4;
return true;
}
@@ -279,11 +279,11 @@ FindStreamingStart(uint32 *tli)
{
uint32 tli;
XLogSegNo segno;
- WalCompressionMethod wal_compression_method;
+ pg_compress_algorithm wal_compression_algo;
bool ispartial;
if (!is_xlogfilename(dirent->d_name,
- &ispartial, &wal_compression_method))
+ &ispartial, &wal_compression_algo))
continue;
/*
@@ -309,7 +309,7 @@ FindStreamingStart(uint32 *tli)
* where WAL segments could have been compressed by a different source
* than pg_receivewal, like an archive_command with lz4.
*/
- if (!ispartial && wal_compression_method == COMPRESSION_NONE)
+ if (!ispartial && wal_compression_algo == PG_COMPRESSION_NONE)
{
struct stat statbuf;
char fullpath[MAXPGPATH * 2];
@@ -325,7 +325,7 @@ FindStreamingStart(uint32 *tli)
continue;
}
}
- else if (!ispartial && wal_compression_method == COMPRESSION_GZIP)
+ else if (!ispartial && wal_compression_algo == PG_COMPRESSION_GZIP)
{
int fd;
char buf[4];
@@ -364,7 +364,7 @@ FindStreamingStart(uint32 *tli)
continue;
}
}
- else if (!ispartial && wal_compression_method == COMPRESSION_LZ4)
+ else if (!ispartial && wal_compression_algo == PG_COMPRESSION_LZ4)
{
#ifdef USE_LZ4
#define LZ4_CHUNK_SZ 64 * 1024 /* 64kB as maximum chunk size read */
@@ -590,7 +590,7 @@ StreamLog(void)
stream.do_sync = do_sync;
stream.mark_done = false;
stream.walmethod = CreateWalDirectoryMethod(basedir,
- compression_method,
+ compression_algo,
compresslevel,
stream.do_sync);
stream.partial_suffix = ".partial";
@@ -750,11 +750,11 @@ main(int argc, char **argv)
break;
case 6:
if (pg_strcasecmp(optarg, "gzip") == 0)
- compression_method = COMPRESSION_GZIP;
+ compression_algo = PG_COMPRESSION_GZIP;
else if (pg_strcasecmp(optarg, "lz4") == 0)
- compression_method = COMPRESSION_LZ4;
+ compression_algo = PG_COMPRESSION_LZ4;
else if (pg_strcasecmp(optarg, "none") == 0)
- compression_method = COMPRESSION_NONE;
+ compression_algo = PG_COMPRESSION_NONE;
else
pg_fatal("invalid value \"%s\" for option %s",
optarg, "--compression-method");
@@ -814,9 +814,9 @@ main(int argc, char **argv)
/*
* Compression-related options.
*/
- switch (compression_method)
+ switch (compression_algo)
{
- case COMPRESSION_NONE:
+ case PG_COMPRESSION_NONE:
if (compresslevel != 0)
{
pg_log_error("cannot use --compress with --compression-method=%s",
@@ -825,7 +825,7 @@ main(int argc, char **argv)
exit(1);
}
break;
- case COMPRESSION_GZIP:
+ case PG_COMPRESSION_GZIP:
#ifdef HAVE_LIBZ
if (compresslevel == 0)
{
@@ -837,7 +837,7 @@ main(int argc, char **argv)
"gzip");
#endif
break;
- case COMPRESSION_LZ4:
+ case PG_COMPRESSION_LZ4:
#ifdef USE_LZ4
if (compresslevel != 0)
{
@@ -851,7 +851,7 @@ main(int argc, char **argv)
"LZ4");
#endif
break;
- case COMPRESSION_ZSTD:
+ case PG_COMPRESSION_ZSTD:
pg_fatal("compression with %s is not yet supported", "ZSTD");
break;
}
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 42d50931d3..e122f4099b 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -114,7 +114,7 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
* When streaming to tar, no file with this name will exist before, so we
* never have to verify a size.
*/
- if (stream->walmethod->compression_method() == COMPRESSION_NONE &&
+ if (stream->walmethod->compression_algo() == PG_COMPRESSION_NONE &&
stream->walmethod->existsfile(fn))
{
size = stream->walmethod->get_file_size(fn);
diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
index acd242d2c9..2412498fed 100644
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -49,7 +49,7 @@
typedef struct DirectoryMethodData
{
char *basedir;
- WalCompressionMethod compression_method;
+ pg_compress_algorithm compression_algo;
int compression_level;
bool sync;
const char *lasterrstring; /* if set, takes precedence over lasterrno */
@@ -97,8 +97,8 @@ dir_get_file_name(const char *pathname, const char *temp_suffix)
snprintf(filename, MAXPGPATH, "%s%s%s",
pathname,
- dir_data->compression_method == COMPRESSION_GZIP ? ".gz" :
- dir_data->compression_method == COMPRESSION_LZ4 ? ".lz4" : "",
+ dir_data->compression_algo == PG_COMPRESSION_GZIP ? ".gz" :
+ dir_data->compression_algo == PG_COMPRESSION_LZ4 ? ".lz4" : "",
temp_suffix ? temp_suffix : "");
return filename;
@@ -141,7 +141,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
}
#ifdef HAVE_LIBZ
- if (dir_data->compression_method == COMPRESSION_GZIP)
+ if (dir_data->compression_algo == PG_COMPRESSION_GZIP)
{
gzfp = gzdopen(fd, "wb");
if (gzfp == NULL)
@@ -161,7 +161,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
}
#endif
#ifdef USE_LZ4
- if (dir_data->compression_method == COMPRESSION_LZ4)
+ if (dir_data->compression_algo == PG_COMPRESSION_LZ4)
{
size_t ctx_out;
size_t header_size;
@@ -202,7 +202,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
#endif
/* Do pre-padding on non-compressed files */
- if (pad_to_size && dir_data->compression_method == COMPRESSION_NONE)
+ if (pad_to_size && dir_data->compression_algo == PG_COMPRESSION_NONE)
{
PGAlignedXLogBlock zerobuf;
int bytes;
@@ -241,12 +241,12 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
{
dir_data->lasterrno = errno;
#ifdef HAVE_LIBZ
- if (dir_data->compression_method == COMPRESSION_GZIP)
+ if (dir_data->compression_algo == PG_COMPRESSION_GZIP)
gzclose(gzfp);
else
#endif
#ifdef USE_LZ4
- if (dir_data->compression_method == COMPRESSION_LZ4)
+ if (dir_data->compression_algo == PG_COMPRESSION_LZ4)
{
(void) LZ4F_compressEnd(ctx, lz4buf, lz4bufsize, NULL);
(void) LZ4F_freeCompressionContext(ctx);
@@ -262,11 +262,11 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
f = pg_malloc0(sizeof(DirectoryMethodFile));
#ifdef HAVE_LIBZ
- if (dir_data->compression_method == COMPRESSION_GZIP)
+ if (dir_data->compression_algo == PG_COMPRESSION_GZIP)
f->gzfp = gzfp;
#endif
#ifdef USE_LZ4
- if (dir_data->compression_method == COMPRESSION_LZ4)
+ if (dir_data->compression_algo == PG_COMPRESSION_LZ4)
{
f->ctx = ctx;
f->lz4buf = lz4buf;
@@ -294,7 +294,7 @@ dir_write(Walfile f, const void *buf, size_t count)
dir_clear_error();
#ifdef HAVE_LIBZ
- if (dir_data->compression_method == COMPRESSION_GZIP)
+ if (dir_data->compression_algo == PG_COMPRESSION_GZIP)
{
errno = 0;
r = (ssize_t) gzwrite(df->gzfp, buf, count);
@@ -307,7 +307,7 @@ dir_write(Walfile f, const void *buf, size_t count)
else
#endif
#ifdef USE_LZ4
- if (dir_data->compression_method == COMPRESSION_LZ4)
+ if (dir_data->compression_algo == PG_COMPRESSION_LZ4)
{
size_t chunk;
size_t remaining;
@@ -387,7 +387,7 @@ dir_close(Walfile f, WalCloseMethod method)
dir_clear_error();
#ifdef HAVE_LIBZ
- if (dir_data->compression_method == COMPRESSION_GZIP)
+ if (dir_data->compression_algo == PG_COMPRESSION_GZIP)
{
errno = 0; /* in case gzclose() doesn't set it */
r = gzclose(df->gzfp);
@@ -395,7 +395,7 @@ dir_close(Walfile f, WalCloseMethod method)
else
#endif
#ifdef USE_LZ4
- if (dir_data->compression_method == COMPRESSION_LZ4)
+ if (dir_data->compression_algo == PG_COMPRESSION_LZ4)
{
size_t compressed;
@@ -514,7 +514,7 @@ dir_sync(Walfile f)
return 0;
#ifdef HAVE_LIBZ
- if (dir_data->compression_method == COMPRESSION_GZIP)
+ if (dir_data->compression_algo == PG_COMPRESSION_GZIP)
{
if (gzflush(((DirectoryMethodFile *) f)->gzfp, Z_SYNC_FLUSH) != Z_OK)
{
@@ -524,7 +524,7 @@ dir_sync(Walfile f)
}
#endif
#ifdef USE_LZ4
- if (dir_data->compression_method == COMPRESSION_LZ4)
+ if (dir_data->compression_algo == PG_COMPRESSION_LZ4)
{
DirectoryMethodFile *df = (DirectoryMethodFile *) f;
size_t compressed;
@@ -571,10 +571,10 @@ dir_get_file_size(const char *pathname)
return statbuf.st_size;
}
-static WalCompressionMethod
-dir_compression_method(void)
+static pg_compress_algorithm
+dir_compression_algo(void)
{
- return dir_data->compression_method;
+ return dir_data->compression_algo;
}
static bool
@@ -618,7 +618,7 @@ dir_finish(void)
WalWriteMethod *
CreateWalDirectoryMethod(const char *basedir,
- WalCompressionMethod compression_method,
+ pg_compress_algorithm compression_algo,
int compression_level, bool sync)
{
WalWriteMethod *method;
@@ -629,7 +629,7 @@ CreateWalDirectoryMethod(const char *basedir,
method->get_current_pos = dir_get_current_pos;
method->get_file_size = dir_get_file_size;
method->get_file_name = dir_get_file_name;
- method->compression_method = dir_compression_method;
+ method->compression_algo = dir_compression_algo;
method->close = dir_close;
method->sync = dir_sync;
method->existsfile = dir_existsfile;
@@ -637,7 +637,7 @@ CreateWalDirectoryMethod(const char *basedir,
method->getlasterror = dir_getlasterror;
dir_data = pg_malloc0(sizeof(DirectoryMethodData));
- dir_data->compression_method = compression_method;
+ dir_data->compression_algo = compression_algo;
dir_data->compression_level = compression_level;
dir_data->basedir = pg_strdup(basedir);
dir_data->sync = sync;
@@ -672,7 +672,7 @@ typedef struct TarMethodData
{
char *tarfilename;
int fd;
- WalCompressionMethod compression_method;
+ pg_compress_algorithm compression_algo;
int compression_level;
bool sync;
TarMethodFile *currentfile;
@@ -759,7 +759,7 @@ tar_write(Walfile f, const void *buf, size_t count)
tar_clear_error();
/* Tarfile will always be positioned at the end */
- if (tar_data->compression_method == COMPRESSION_NONE)
+ if (tar_data->compression_algo == PG_COMPRESSION_NONE)
{
errno = 0;
r = write(tar_data->fd, buf, count);
@@ -773,7 +773,7 @@ tar_write(Walfile f, const void *buf, size_t count)
return r;
}
#ifdef HAVE_LIBZ
- else if (tar_data->compression_method == COMPRESSION_GZIP)
+ else if (tar_data->compression_algo == PG_COMPRESSION_GZIP)
{
if (!tar_write_compressed_data(unconstify(void *, buf), count, false))
return -1;
@@ -842,7 +842,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
}
#ifdef HAVE_LIBZ
- if (tar_data->compression_method == COMPRESSION_GZIP)
+ if (tar_data->compression_algo == PG_COMPRESSION_GZIP)
{
tar_data->zp = (z_streamp) pg_malloc(sizeof(z_stream));
tar_data->zp->zalloc = Z_NULL;
@@ -893,7 +893,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
pg_free(tmppath);
#ifdef HAVE_LIBZ
- if (tar_data->compression_method == COMPRESSION_GZIP)
+ if (tar_data->compression_algo == PG_COMPRESSION_GZIP)
{
/* Flush existing data */
if (!tar_write_compressed_data(NULL, 0, true))
@@ -918,7 +918,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
}
tar_data->currentfile->currpos = 0;
- if (tar_data->compression_method == COMPRESSION_NONE)
+ if (tar_data->compression_algo == PG_COMPRESSION_NONE)
{
errno = 0;
if (write(tar_data->fd, tar_data->currentfile->header,
@@ -932,7 +932,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
}
}
#ifdef HAVE_LIBZ
- else if (tar_data->compression_method == COMPRESSION_GZIP)
+ else if (tar_data->compression_algo == PG_COMPRESSION_GZIP)
{
/* Write header through the zlib APIs but with no compression */
if (!tar_write_compressed_data(tar_data->currentfile->header,
@@ -962,7 +962,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
if (pad_to_size)
{
tar_data->currentfile->pad_to_size = pad_to_size;
- if (tar_data->compression_method == COMPRESSION_NONE)
+ if (tar_data->compression_algo == PG_COMPRESSION_NONE)
{
/* Uncompressed, so pad now */
if (!tar_write_padding_data(tar_data->currentfile, pad_to_size))
@@ -993,10 +993,10 @@ tar_get_file_size(const char *pathname)
return -1;
}
-static WalCompressionMethod
-tar_compression_method(void)
+static pg_compress_algorithm
+tar_compression_algo(void)
{
- return tar_data->compression_method;
+ return tar_data->compression_algo;
}
static off_t
@@ -1023,7 +1023,7 @@ tar_sync(Walfile f)
* Always sync the whole tarfile, because that's all we can do. This makes
* no sense on compressed files, so just ignore those.
*/
- if (tar_data->compression_method != COMPRESSION_NONE)
+ if (tar_data->compression_algo != PG_COMPRESSION_NONE)
return 0;
r = fsync(tar_data->fd);
@@ -1044,7 +1044,7 @@ tar_close(Walfile f, WalCloseMethod method)
if (method == CLOSE_UNLINK)
{
- if (tar_data->compression_method != COMPRESSION_NONE)
+ if (tar_data->compression_algo != PG_COMPRESSION_NONE)
{
tar_set_error("unlink not supported with compression");
return -1;
@@ -1075,7 +1075,7 @@ tar_close(Walfile f, WalCloseMethod method)
*/
if (tf->pad_to_size)
{
- if (tar_data->compression_method == COMPRESSION_GZIP)
+ if (tar_data->compression_algo == PG_COMPRESSION_GZIP)
{
/*
* A compressed tarfile is padded on close since we cannot know
@@ -1116,7 +1116,7 @@ tar_close(Walfile f, WalCloseMethod method)
#ifdef HAVE_LIBZ
- if (tar_data->compression_method == COMPRESSION_GZIP)
+ if (tar_data->compression_algo == PG_COMPRESSION_GZIP)
{
/* Flush the current buffer */
if (!tar_write_compressed_data(NULL, 0, true))
@@ -1145,7 +1145,7 @@ tar_close(Walfile f, WalCloseMethod method)
tar_data->lasterrno = errno;
return -1;
}
- if (tar_data->compression_method == COMPRESSION_NONE)
+ if (tar_data->compression_algo == PG_COMPRESSION_NONE)
{
errno = 0;
if (write(tar_data->fd, tf->header, TAR_BLOCK_SIZE) != TAR_BLOCK_SIZE)
@@ -1156,7 +1156,7 @@ tar_close(Walfile f, WalCloseMethod method)
}
}
#ifdef HAVE_LIBZ
- else if (tar_data->compression_method == COMPRESSION_GZIP)
+ else if (tar_data->compression_algo == PG_COMPRESSION_GZIP)
{
/* Turn off compression */
if (deflateParams(tar_data->zp, 0, 0) != Z_OK)
@@ -1230,7 +1230,7 @@ tar_finish(void)
/* A tarfile always ends with two empty blocks */
MemSet(zerobuf, 0, sizeof(zerobuf));
- if (tar_data->compression_method == COMPRESSION_NONE)
+ if (tar_data->compression_algo == PG_COMPRESSION_NONE)
{
errno = 0;
if (write(tar_data->fd, zerobuf, sizeof(zerobuf)) != sizeof(zerobuf))
@@ -1241,7 +1241,7 @@ tar_finish(void)
}
}
#ifdef HAVE_LIBZ
- else if (tar_data->compression_method == COMPRESSION_GZIP)
+ else if (tar_data->compression_algo == PG_COMPRESSION_GZIP)
{
if (!tar_write_compressed_data(zerobuf, sizeof(zerobuf), false))
return false;
@@ -1324,18 +1324,18 @@ tar_finish(void)
}
/*
- * The argument compression_method is currently ignored. It is in place for
+ * The argument compression_algo is currently ignored. It is in place for
* symmetry with CreateWalDirectoryMethod which uses it for distinguishing
* between the different compression methods. CreateWalTarMethod and its family
* of functions handle only zlib compression.
*/
WalWriteMethod *
CreateWalTarMethod(const char *tarbase,
- WalCompressionMethod compression_method,
+ pg_compress_algorithm compression_algo,
int compression_level, bool sync)
{
WalWriteMethod *method;
- const char *suffix = (compression_method == COMPRESSION_GZIP) ?
+ const char *suffix = (compression_algo == PG_COMPRESSION_GZIP) ?
".tar.gz" : ".tar";
method = pg_malloc0(sizeof(WalWriteMethod));
@@ -1344,7 +1344,7 @@ CreateWalTarMethod(const char *tarbase,
method->get_current_pos = tar_get_current_pos;
method->get_file_size = tar_get_file_size;
method->get_file_name = tar_get_file_name;
- method->compression_method = tar_compression_method;
+ method->compression_algo = tar_compression_algo;
method->close = tar_close;
method->sync = tar_sync;
method->existsfile = tar_existsfile;
@@ -1355,11 +1355,11 @@ CreateWalTarMethod(const char *tarbase,
tar_data->tarfilename = pg_malloc0(strlen(tarbase) + strlen(suffix) + 1);
sprintf(tar_data->tarfilename, "%s%s", tarbase, suffix);
tar_data->fd = -1;
- tar_data->compression_method = compression_method;
+ tar_data->compression_algo = compression_algo;
tar_data->compression_level = compression_level;
tar_data->sync = sync;
#ifdef HAVE_LIBZ
- if (compression_method == COMPRESSION_GZIP)
+ if (compression_algo == PG_COMPRESSION_GZIP)
tar_data->zlibOut = (char *) pg_malloc(ZLIB_OUT_SIZE + 1);
#endif
@@ -1371,7 +1371,7 @@ FreeWalTarMethod(void)
{
pg_free(tar_data->tarfilename);
#ifdef HAVE_LIBZ
- if (tar_data->compression_method == COMPRESSION_GZIP)
+ if (tar_data->compression_algo == PG_COMPRESSION_GZIP)
pg_free(tar_data->zlibOut);
#endif
pg_free(tar_data);
diff --git a/src/bin/pg_basebackup/walmethods.h b/src/bin/pg_basebackup/walmethods.h
index ec54019cfc..15639607d3 100644
--- a/src/bin/pg_basebackup/walmethods.h
+++ b/src/bin/pg_basebackup/walmethods.h
@@ -9,6 +9,7 @@
*-------------------------------------------------------------------------
*/
+#include "common/compression.h"
typedef void *Walfile;
@@ -19,15 +20,6 @@ typedef enum
CLOSE_NO_RENAME
} WalCloseMethod;
-/* Types of compression supported */
-typedef enum
-{
- COMPRESSION_GZIP,
- COMPRESSION_LZ4,
- COMPRESSION_ZSTD,
- COMPRESSION_NONE
-} WalCompressionMethod;
-
/*
* A WalWriteMethod structure represents the different methods used
* to write the streaming WAL as it's received.
@@ -68,7 +60,7 @@ struct WalWriteMethod
char *(*get_file_name) (const char *pathname, const char *temp_suffix);
/* Returns the compression method */
- WalCompressionMethod (*compression_method) (void);
+ pg_compress_algorithm (*compression_algo) (void);
/*
* Write count number of bytes to the file, and return the number of bytes
@@ -104,10 +96,10 @@ struct WalWriteMethod
* not all those required for pg_receivewal)
*/
WalWriteMethod *CreateWalDirectoryMethod(const char *basedir,
- WalCompressionMethod compression_method,
+ pg_compress_algorithm compression_algo,
int compression, bool sync);
WalWriteMethod *CreateWalTarMethod(const char *tarbase,
- WalCompressionMethod compression_method,
+ pg_compress_algorithm compression_algo,
int compression, bool sync);
/* Cleanup routines for previously-created methods */
--
2.35.1
[text/x-diff] v1-0003-Rework-compression-options-of-pg_receivewal.patch (12.6K, ../../[email protected]/4-v1-0003-Rework-compression-options-of-pg_receivewal.patch)
download | inline diff:
From c77f99bf7d6cfe45fd5a2d4ee5029059d873d803 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Mon, 11 Apr 2022 15:33:27 +0900
Subject: [PATCH v1 3/3] 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=N 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 | 130 +++++++++++++------
src/bin/pg_basebackup/t/020_pg_receivewal.pl | 16 +--
doc/src/sgml/ref/pg_receivewal.sgml | 47 ++++---
3 files changed, 120 insertions(+), 73 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index b371bad644..a47b4a8ef7 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -57,6 +57,8 @@ static XLogRecPtr endpos = InvalidXLogRecPtr;
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 after writing\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -V, --version output version information, then exit\n"));
- printf(_(" --compression-method=METHOD\n"
- " method to compress logs\n"));
- printf(_(" -Z, --compress=1-9 compress logs with given compression level\n"));
+ printf(_(" -Z, --compress=METHOD[:DETAIL]\n"
+ " compress as specified\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nConnection options:\n"));
printf(_(" -d, --dbname=CONNSTR connection string\n"));
@@ -108,6 +109,66 @@ usage(void)
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
}
+/*
+ * 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 extracts
+ * 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 = strtol(option, &endp, 10);
+ if (*endp == '\0')
+ {
+ if (result == 0)
+ {
+ *algorithm = pstrdup("none");
+ *detail = NULL;
+ }
+ else
+ {
+ *algorithm = pstrdup("gzip");
+ *detail = pstrdup(option);
+ }
+ return;
+ }
+
+ /*
+ * Check whether there is a compression detail following the algorithm
+ * name.
+ */
+ sep = strchr(option, ':');
+ if (sep == NULL)
+ {
+ *algorithm = pstrdup(option);
+ *detail = NULL;
+ }
+ else
+ {
+ char *alg;
+
+ alg = palloc((sep - option) + 1);
+ memcpy(alg, option, sep - option);
+ alg[sep - option] = '\0';
+
+ *algorithm = alg;
+ *detail = 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}
};
@@ -660,6 +720,10 @@ main(int argc, char **argv)
char *db_name;
uint32 hi,
lo;
+ pg_compress_specification compression_spec;
+ char *compression_detail = NULL;
+ char *compression_algo_str = "none";
+ char *error_detail = NULL;
pg_logging_init(argv[0]);
progname = 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_algo_str,
+ &compression_detail);
break;
/* action */
case 1:
@@ -748,17 +811,6 @@ main(int argc, char **argv)
case 5:
do_sync = false;
break;
- case 6:
- if (pg_strcasecmp(optarg, "gzip") == 0)
- compression_algo = PG_COMPRESSION_GZIP;
- else if (pg_strcasecmp(optarg, "lz4") == 0)
- compression_algo = PG_COMPRESSION_LZ4;
- else if (pg_strcasecmp(optarg, "none") == 0)
- compression_algo = 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,29 @@ main(int argc, char **argv)
exit(1);
}
-
/*
- * Compression-related options.
+ * Compression options
*/
+ if (!parse_compress_algorithm(compression_algo_str, &compression_algo))
+ pg_fatal("unrecognized compression algorithm \"%s\"",
+ compression_algo_str);
+
+ parse_compress_specification(compression_algo, compression_detail,
+ &compression_spec);
+ error_detail = validate_compress_specification(&compression_spec);
+ if (error_detail != 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) != 0)
+ compresslevel = compression_spec.level;
+
switch (compression_algo)
{
- case PG_COMPRESSION_NONE:
- if (compresslevel != 0)
- {
- pg_log_error("cannot use --compress with --compression-method=%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 == 0)
+ if ((compression_spec.options & PG_COMPRESSION_OPTION_LEVEL) == 0)
{
pg_log_info("no value specified for --compress, switching to default");
compresslevel = Z_DEFAULT_COMPRESSION;
@@ -837,16 +894,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 != 0)
- {
- pg_log_error("cannot use --compress with --compression-method=%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_basebackup/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-method=none/,
- 'failure if --compress specified with --compression-method=none');
+ qr/\Qpg_receivewal: error: invalid compression specification: compression algorithm "none" does not accept a compression level/,
+ 'failure if --compress none:N (where N > 0)');
# Slot creation and drop
my $slot_name = 'test';
@@ -93,15 +92,12 @@ SKIP:
chomp($nextlsn);
$primary->psql('postgres', 'INSERT INTO test_table VALUES (2);');
- # 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=lz4');
+ 'streaming some WAL using --compress=lz4');
# 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_receivewal.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
</varlistentry>
<varlistentry>
- <term><option>--compression-method=<replaceable class="parameter">method</replaceable></option></term>
+ <term><option>-Z <replaceable class="parameter">level</replaceable></option></term>
+ <term><option>-Z <replaceable class="parameter">method</replaceable>[:<replaceable>detail</replaceable>]</option></term>
+ <term><option>--compress=<replaceable class="parameter">level</replaceable></option></term>
+ <term><option>--compress=<replaceable class="parameter">method</replaceable>[:<replaceable>detail</replaceable>]</option></term>
<listitem>
<para>
- Enables compression of write-ahead logs using the specified method.
- Supported values are <literal>gzip</literal>, <literal>lz4</literal>
- (if <productname>PostgreSQL</productname> was compiled with
- <option>--with-lz4</option>), and <literal>none</literal>.
+ Enables compression of write-ahead logs.
+ </para>
+ <para>
+ The compression method can be set to <literal>gzip</literal>,
+ <literal>lz4</literal> (if <productname>PostgreSQL</productname>
+ was compiled with <option>--with-lz4</option>) or
+ <literal>none</literal> 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 the
+ form <literal>keyword</literal> or <literal>keyword=value</literal>.
+ Currently, the only supported keyword is <literal>level</literal>.
+ </para>
+ <para>
+ If no compression level is specified, the default compression level
+ will be used. If only a level is specified without mentioning an
+ algorithm, <literal>gzip</literal> compression will be used if the
+ level is greater than 0, and no compression will be used if the level
+ is 0.
</para>
-
<para>
The suffix <filename>.gz</filename> will automatically be added to
all filenames when using <literal>gzip</literal>, and the suffix
@@ -279,24 +296,6 @@ PostgreSQL documentation
</para>
</listitem>
</varlistentry>
-
- <varlistentry>
- <term><option>-Z <replaceable class="parameter">level</replaceable></option></term>
- <term><option>--compress=<replaceable class="parameter">level</replaceable></option></term>
- <listitem>
- <para>
- Specifies the compression level (<literal>1</literal> through
- <literal>9</literal>, <literal>1</literal> being worst compression
- and <literal>9</literal> being best compression) for WAL segments
- compressed with <application>gzip</application>.
- </para>
-
- <para>
- This option requires <option>--compression-method</option> to be
- specified with <literal>gzip</literal>.
- </para>
- </listitem>
- </varlistentry>
</variablelist>
<para>
--
2.35.1
[application/pgp-signature] signature.asc (833B, ../../[email protected]/5-signature.asc)
download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}
@ 2022-04-11 12:46 [email protected]
parent: Michael Paquier <[email protected]>
1 sibling, 1 reply; 11+ messages in thread
From: [email protected] @ 2022-04-11 12:46 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]>; Robert Haas <[email protected]>
On Monday, April 11th, 2022 at 8:52 AM, Michael Paquier <[email protected]>
wrote:
> This is something I think we had better fix before beta1, because now
> we have binaries that use an inconsistent set of options. So,
> attached is a patch set aimed at rework this option set from the
> ground, taking advantage of the recent work done by Robert and others
> for pg_basebackup:
Agreed. It is rather inconsistent now.
> - 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~.
I agree with the design. If you permit me a couple of nitpicks regarding naming.
+typedef enum pg_compress_algorithm
+{
+ PG_COMPRESSION_NONE,
+ PG_COMPRESSION_GZIP,
+ PG_COMPRESSION_LZ4,
+ PG_COMPRESSION_ZSTD
+} pg_compress_algorithm;
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 mention a
few) variations of of the nomenclature "compression method" are used, like
'VARDATA_COMPRESSED_GET_COMPRESS_METHOD' or 'InvalidCompressionMethod' etc. I
feel that it would be nicer if we followed one naming rule for this and I
recommend to substitute algorithm for method throughout.
On a similar note, it would help readability to be able to distinguish at a
glance the type from the variable. Maybe uppercase or camelcase the type?
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 come to
it.
> - 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.
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.
> - 0003 is the actual option rework for pg_receivewal. This takes
> advantage of 0001, leading to the result of removing --compress-method
> and replacing it with --compress, taking care of the backward
> compatibility problems for an integer value, aka 0 implies no
> compression and val > 0 implies gzip. One bonus reason to switch to
> that is that this would make the addition of zstd for pg_receivewal
> easier in the future.
Looks good.
> I am going to add an open item for this stuff. Comments or thoughts?
I agree that it is better to not release pg_receivewal with the distinct set of
options.
Cheers,
//Georgios
> Thanks,
> --
> Michael
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}
@ 2022-04-11 15:15 Robert Haas <[email protected]>
parent: Michael Paquier <[email protected]>
1 sibling, 1 reply; 11+ messages in thread
From: Robert Haas @ 2022-04-11 15:15 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]>; Georgios <[email protected]>
On Mon, Apr 11, 2022 at 2:52 AM Michael Paquier <[email protected]> wrote:
> Since babbbb5 and the introduction of LZ4, I have reworked the way
> compression is controlled for pg_receivewal, with two options:
> - --compress-method, settable to "gzip", "none" or "lz4".
> - --compress, to pass down a compression level, where the allowed
> range is 1-9. If passing down 0, we'd get an error rather than
> implying no compression, contrary to what we did in ~14.
>
> I initially thought that this was fine as-is, but then Robert and
> others have worked on client/server compression for pg_basebackup,
> introducing a much better design with centralized APIs where one can
> use METHOD:DETAIL for as compression value, where DETAIL is a
> comma-separated list of keyword=value (keyword = "level" or
> "workers"), with centralized checks and an extensible design.
>
> This is something I think we had better fix before beta1, because now
> we have binaries that use an inconsistent set of options. So,
> attached is a patch set aimed at rework this option set from the
> ground, taking advantage of the recent work done by Robert and others
> for pg_basebackup:
+1 for this in general, but I think that naming like
"compression_algo" stinks. If you think "compression_algorithm" is too
long, I think you should use "algorithm" or "compression" or
"compression_method" or something.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}
@ 2022-04-11 22:50 Michael Paquier <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Michael Paquier @ 2022-04-11 22:50 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Postgres hackers <[email protected]>; Georgios <[email protected]>
On Mon, Apr 11, 2022 at 11:15:46AM -0400, Robert Haas wrote:
> +1 for this in general, but I think that naming like
> "compression_algo" stinks. If you think "compression_algorithm" is too
> long, I think you should use "algorithm" or "compression" or
> "compression_method" or something.
Yes, I found "compression_algorithm" to be too long initially. For
walmethods.c and pg_receivewal.c, it may be better to just stick to
"algorithm" then, at least that's consistent with pg_basebackup.c.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}
@ 2022-04-12 09:22 Michael Paquier <[email protected]>
parent: [email protected]
0 siblings, 1 reply; 11+ messages in thread
From: Michael Paquier @ 2022-04-12 09:22 UTC (permalink / raw)
To: [email protected]; +Cc: Postgres hackers <[email protected]>; Robert Haas <[email protected]>
On Mon, Apr 11, 2022 at 12:46:02PM +0000, [email protected] wrote:
> On Monday, April 11th, 2022 at 8:52 AM, Michael Paquier <[email protected]>
> 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~.
>
> I agree with the design. If you permit me a couple of nitpicks regarding naming.
>
> +typedef enum pg_compress_algorithm
> +{
> + PG_COMPRESSION_NONE,
> + PG_COMPRESSION_GZIP,
> + PG_COMPRESSION_LZ4,
> + PG_COMPRESSION_ZSTD
> +} pg_compress_algorithm;
>
> 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 mention a
> few) variations of of the nomenclature "compression method" are used, like
> 'VARDATA_COMPRESSED_GET_COMPRESS_METHOD' or 'InvalidCompressionMethod' etc. 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 come 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.
>
> 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
Attachments:
[text/x-diff] v2-0001-Rework-compression-options-of-pg_receivewal.patch (12.7K, ../../[email protected]/2-v2-0001-Rework-compression-options-of-pg_receivewal.patch)
download | inline diff:
From 94850ac604402371135e85de27a5d9074a947d4b Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
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=N 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 = InvalidXLogRecPtr;
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 after writing\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -V, --version output version information, then exit\n"));
- printf(_(" --compression-method=METHOD\n"
- " method to compress logs\n"));
- printf(_(" -Z, --compress=1-9 compress logs with given compression level\n"));
+ printf(_(" -Z, --compress=METHOD[:DETAIL]\n"
+ " compress as specified\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nConnection options:\n"));
printf(_(" -d, --dbname=CONNSTR connection string\n"));
@@ -108,6 +109,66 @@ usage(void)
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
}
+/*
+ * 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 extracts
+ * 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 = strtol(option, &endp, 10);
+ if (*endp == '\0')
+ {
+ if (result == 0)
+ {
+ *algorithm = pstrdup("none");
+ *detail = NULL;
+ }
+ else
+ {
+ *algorithm = pstrdup("gzip");
+ *detail = pstrdup(option);
+ }
+ return;
+ }
+
+ /*
+ * Check whether there is a compression detail following the algorithm
+ * name.
+ */
+ sep = strchr(option, ':');
+ if (sep == NULL)
+ {
+ *algorithm = pstrdup(option);
+ *detail = NULL;
+ }
+ else
+ {
+ char *alg;
+
+ alg = palloc((sep - option) + 1);
+ memcpy(alg, option, sep - option);
+ alg[sep - option] = '\0';
+
+ *algorithm = alg;
+ *detail = 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}
};
@@ -660,6 +720,10 @@ main(int argc, char **argv)
char *db_name;
uint32 hi,
lo;
+ pg_compress_specification compression_spec;
+ char *compression_detail = NULL;
+ char *compression_algorithm_str = "none";
+ char *error_detail = NULL;
pg_logging_init(argv[0]);
progname = 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 = false;
break;
- case 6:
- if (pg_strcasecmp(optarg, "gzip") == 0)
- compression_algorithm = PG_COMPRESSION_GZIP;
- else if (pg_strcasecmp(optarg, "lz4") == 0)
- compression_algorithm = PG_COMPRESSION_LZ4;
- else if (pg_strcasecmp(optarg, "none") == 0)
- compression_algorithm = 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);
}
-
/*
- * 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 = validate_compress_specification(&compression_spec);
+ if (error_detail != 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) != 0)
+ compresslevel = compression_spec.level;
+
switch (compression_algorithm)
{
- case PG_COMPRESSION_NONE:
- if (compresslevel != 0)
- {
- pg_log_error("cannot use --compress with --compression-method=%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 == 0)
+ if ((compression_spec.options & PG_COMPRESSION_OPTION_LEVEL) == 0)
{
pg_log_info("no value specified for --compress, switching to default");
compresslevel = 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 != 0)
- {
- pg_log_error("cannot use --compress with --compression-method=%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_basebackup/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-method=none/,
- 'failure if --compress specified with --compression-method=none');
+ qr/\Qpg_receivewal: error: invalid compression specification: compression algorithm "none" does not accept a compression level/,
+ 'failure if --compress none:N (where N > 0)');
# Slot creation and drop
my $slot_name = 'test';
@@ -93,15 +92,12 @@ SKIP:
chomp($nextlsn);
$primary->psql('postgres', 'INSERT INTO test_table VALUES (2);');
- # 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=lz4');
+ 'streaming some WAL using --compress=lz4');
# 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_receivewal.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
</varlistentry>
<varlistentry>
- <term><option>--compression-method=<replaceable class="parameter">method</replaceable></option></term>
+ <term><option>-Z <replaceable class="parameter">level</replaceable></option></term>
+ <term><option>-Z <replaceable class="parameter">method</replaceable>[:<replaceable>detail</replaceable>]</option></term>
+ <term><option>--compress=<replaceable class="parameter">level</replaceable></option></term>
+ <term><option>--compress=<replaceable class="parameter">method</replaceable>[:<replaceable>detail</replaceable>]</option></term>
<listitem>
<para>
- Enables compression of write-ahead logs using the specified method.
- Supported values are <literal>gzip</literal>, <literal>lz4</literal>
- (if <productname>PostgreSQL</productname> was compiled with
- <option>--with-lz4</option>), and <literal>none</literal>.
+ Enables compression of write-ahead logs.
+ </para>
+ <para>
+ The compression method can be set to <literal>gzip</literal>,
+ <literal>lz4</literal> (if <productname>PostgreSQL</productname>
+ was compiled with <option>--with-lz4</option>) or
+ <literal>none</literal> 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 the
+ form <literal>keyword</literal> or <literal>keyword=value</literal>.
+ Currently, the only supported keyword is <literal>level</literal>.
+ </para>
+ <para>
+ If no compression level is specified, the default compression level
+ will be used. If only a level is specified without mentioning an
+ algorithm, <literal>gzip</literal> compression will be used if the
+ level is greater than 0, and no compression will be used if the level
+ is 0.
</para>
-
<para>
The suffix <filename>.gz</filename> will automatically be added to
all filenames when using <literal>gzip</literal>, and the suffix
@@ -279,24 +296,6 @@ PostgreSQL documentation
</para>
</listitem>
</varlistentry>
-
- <varlistentry>
- <term><option>-Z <replaceable class="parameter">level</replaceable></option></term>
- <term><option>--compress=<replaceable class="parameter">level</replaceable></option></term>
- <listitem>
- <para>
- Specifies the compression level (<literal>1</literal> through
- <literal>9</literal>, <literal>1</literal> being worst compression
- and <literal>9</literal> being best compression) for WAL segments
- compressed with <application>gzip</application>.
- </para>
-
- <para>
- This option requires <option>--compression-method</option> to be
- specified with <literal>gzip</literal>.
- </para>
- </listitem>
- </varlistentry>
</variablelist>
<para>
--
2.35.1
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}
@ 2022-04-13 05:25 Michael Paquier <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Michael Paquier @ 2022-04-13 05:25 UTC (permalink / raw)
To: [email protected]; +Cc: Postgres hackers <[email protected]>; Robert Haas <[email protected]>
On Tue, Apr 12, 2022 at 06:22:48PM +0900, Michael Paquier wrote:
> On Mon, Apr 11, 2022 at 12:46:02PM +0000, [email protected] wrote:
>> 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.
This has been done with the proper renames. With that in place, I see
no reason now to not be able to set the compression level as it is
possible to pass it down with the options available. This requires
only a couple of lines, as of the attached. LZ4 has a dummy structure
called LZ4F_INIT_PREFERENCES to initialize LZ4F_preferences_t, that
holds the compression level before passing it down to
LZ4F_compressBegin(), but that's available only in v1.8.3. Using it
risks lowering down the minimal version of LZ4 we are able to use now,
but replacing that with a set of memset()s is also a way to set up
things as per its documentation.
Thoughts?
--
Michael
Attachments:
[text/x-diff] receivewal-level.patch (1.1K, ../../[email protected]/2-receivewal-level.patch)
download | inline diff:
diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
index d5bcc208a9..1b3cc5c4f7 100644
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -165,6 +165,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
{
size_t ctx_out;
size_t header_size;
+ LZ4F_preferences_t prefs;
ctx_out = LZ4F_createCompressionContext(&ctx, LZ4F_VERSION);
if (LZ4F_isError(ctx_out))
@@ -177,8 +178,13 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
lz4bufsize = LZ4F_compressBound(LZ4_IN_SIZE, NULL);
lz4buf = pg_malloc0(lz4bufsize);
+ /* assign the compression level, default is 0 */
+ memset(&prefs, 0, sizeof(prefs));
+ memset(&prefs.frameInfo, 0, sizeof(prefs.frameInfo));
+ prefs.compressionLevel = dir_data->compression_level;
+
/* add the header */
- header_size = LZ4F_compressBegin(ctx, lz4buf, lz4bufsize, NULL);
+ header_size = LZ4F_compressBegin(ctx, lz4buf, lz4bufsize, &prefs);
if (LZ4F_isError(header_size))
{
dir_data->lasterrstring = LZ4F_getErrorName(header_size);
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}
@ 2022-04-13 14:58 [email protected]
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: [email protected] @ 2022-04-13 14:58 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]>; Robert Haas <[email protected]>
------- Original Message -------
On Wednesday, April 13th, 2022 at 7:25 AM, Michael Paquier <[email protected]> wrote:
>
>
> On Tue, Apr 12, 2022 at 06:22:48PM +0900, Michael Paquier wrote:
>
> > On Mon, Apr 11, 2022 at 12:46:02PM +0000, [email protected] wrote:
> >
> > > 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.
>
> This has been done with the proper renames. With that in place, I see
> no reason now to not be able to set the compression level as it is
> possible to pass it down with the options available. This requires
> only a couple of lines, as of the attached. LZ4 has a dummy structure
> called LZ4F_INIT_PREFERENCES to initialize LZ4F_preferences_t, that
> holds the compression level before passing it down to
> LZ4F_compressBegin(), but that's available only in v1.8.3. Using it
> risks lowering down the minimal version of LZ4 we are able to use now,
> but replacing that with a set of memset()s is also a way to set up
> things as per its documentation.
>
> Thoughts?
It's really not hard to add compression level. However we had briefly
discussed it in the original thread [1] and decided against. That is why
I did not write that code. If the community thinks differently now, let
me know if you would like me to offer a patch for it.
Cheers,
//Georgios
[1] https://www.postgresql.org/message-id/flat/CABUevEwuq7XXyd4fA0W3jY9MsJu9B2WRbHumAA%2B3WzHrGAQjsg%40m...
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}
@ 2022-04-13 21:18 Michael Paquier <[email protected]>
parent: [email protected]
0 siblings, 1 reply; 11+ messages in thread
From: Michael Paquier @ 2022-04-13 21:18 UTC (permalink / raw)
To: [email protected]; +Cc: Postgres hackers <[email protected]>; Robert Haas <[email protected]>
On Wed, Apr 13, 2022 at 02:58:28PM +0000, [email protected] wrote:
> It's really not hard to add compression level. However we had briefly
> discussed it in the original thread [1] and decided against. That is why
> I did not write that code. If the community thinks differently now, let
> me know if you would like me to offer a patch for it.
The issue back then was how to design the option set to handle all
that (right? My memories may be short on that), and pg_basebackup
takes care of that with its option design.
This is roughly what has been done here, except that this was for the
contentSize:
https://www.postgresql.org/message-id/rYyZ3Fj9qayyY9-egNsV_kkLbL_BSWcOEdi3Mb6M9eQRTkcA2jrqFEHglLUEYn...
Do you think that the extra test coverage is going to be too much of a
burden? I was thinking about just adding a level to the main lz4
command, with an extra negative test in the SKIP block with a level
out of range
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}
@ 2022-04-18 02:42 Michael Paquier <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Michael Paquier @ 2022-04-18 02:42 UTC (permalink / raw)
To: [email protected]; +Cc: Postgres hackers <[email protected]>; Robert Haas <[email protected]>
On Thu, Apr 14, 2022 at 06:18:29AM +0900, Michael Paquier wrote:
> The issue back then was how to design the option set to handle all
> that (right? My memories may be short on that), and pg_basebackup
> takes care of that with its option design.
I have looked at that again this morning, and the change is
straight-forward so I saw no reason to not do it. And, applied.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH] Test to demonstrate bug in commit 0d3dba38c7 and to verify a fix.
@ 2026-05-12 10:27 Antonin Houska <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Antonin Houska @ 2026-05-12 10:27 UTC (permalink / raw)
The fix: https://www.postgresql.org/message-id/77611.1778055944%40localhost
---
src/backend/access/transam/xact.c | 2 +
src/backend/commands/repack_worker.c | 2 +
src/test/isolation/isolationtester.c | 9 +-
.../expected/repack_running_xacts.out | 81 ++++++++++++
.../specs/repack_running_xacts.spec | 119 ++++++++++++++++++
5 files changed, 212 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/injection_points/expected/repack_running_xacts.out
create mode 100644 src/test/modules/injection_points/specs/repack_running_xacts.spec
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 5586fbe5b07..b63ee166028 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -65,6 +65,7 @@
#include "utils/builtins.h"
#include "utils/combocid.h"
#include "utils/guc.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/memutils.h"
#include "utils/relmapper.h"
@@ -2428,6 +2429,7 @@ CommitTransaction(void)
* must be done _before_ releasing locks we hold and _after_
* RecordTransactionCommit.
*/
+ INJECTION_POINT("before-end-transaction", NULL);
ProcArrayEndTransaction(MyProc, latestXid);
/*
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index c40f8c98e06..6835626d677 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -26,6 +26,7 @@
#include "storage/ipc.h"
#include "storage/proc.h"
#include "tcop/tcopprot.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#define REPL_PLUGIN_NAME "pgrepack"
@@ -233,6 +234,7 @@ repack_setup_logical_decoding(Oid relid)
* Neither prepare_write nor do_write callback nor update_progress is
* useful for us.
*/
+ INJECTION_POINT("before-create-decoding-context", NULL);
ctx = CreateInitDecodingContext(REPL_PLUGIN_NAME,
NIL,
true,
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index 440c875b8ac..8f17ee412c9 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -216,15 +216,22 @@ main(int argc, char **argv)
* exactly expect concurrent use of test tables. However, autovacuum will
* occasionally take AccessExclusiveLock to truncate a table, and we must
* ignore that transient wait.
+ *
+ * If the session's backend is blocked, and if its background worker is
+ * waiting on an injection point, we assume that the injection point is
+ * the reason for the backend to be blocked. That's what we check in the
+ * second query of the UNION. XXX Should we use a separate query for that?
*/
initPQExpBuffer(&wait_query);
appendPQExpBufferStr(&wait_query,
+ "WITH blocking(res) AS ("
"SELECT pg_catalog.pg_isolation_test_session_is_blocked($1, '{");
/* The spec syntax requires at least one session; assume that here. */
appendPQExpBufferStr(&wait_query, conns[1].backend_pid_str);
for (i = 2; i < nconns; i++)
appendPQExpBuffer(&wait_query, ",%s", conns[i].backend_pid_str);
- appendPQExpBufferStr(&wait_query, "}')");
+ appendPQExpBufferStr(&wait_query, "}') UNION "
+ "SELECT pg_catalog.pg_isolation_test_session_is_blocked(pid, '{}') FROM pg_stat_activity WHERE leader_pid=$1) SELECT bool_or(res) FROM blocking");
res = PQprepare(conns[0].conn, PREP_WAITING, wait_query.data, 0, NULL);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
diff --git a/src/test/modules/injection_points/expected/repack_running_xacts.out b/src/test/modules/injection_points/expected/repack_running_xacts.out
new file mode 100644
index 00000000000..271fe2b97cb
--- /dev/null
+++ b/src/test/modules/injection_points/expected/repack_running_xacts.out
@@ -0,0 +1,81 @@
+Parsed test spec with 5 sessions
+
+starting permutation: repack s3_assign_xid wakeup_bcdc s4_changes s4_attach s4_commit s3_commit s5_assign_xid wakeup_bet s5_commit check
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step repack:
+ REPACK (CONCURRENTLY) repack_test;
+ <waiting ...>
+step s3_assign_xid:
+ BEGIN;
+ INSERT INTO aux VALUES (1);
+
+step wakeup_bcdc:
+ SELECT injection_points_wakeup('before-create-decoding-context');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s4_changes:
+ BEGIN;
+ INSERT INTO repack_test(i, j) VALUES (1, 1);
+
+step s4_attach:
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('before-end-transaction', 'wait');
+
+injection_points_set_local
+--------------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s4_commit:
+ COMMIT;
+ <waiting ...>
+step s3_commit:
+ COMMIT;
+
+step s5_assign_xid:
+ BEGIN;
+ INSERT INTO aux VALUES (2);
+
+step wakeup_bet:
+ SELECT injection_points_wakeup('before-end-transaction');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step repack: <... completed>
+step s4_commit: <... completed>
+step s5_commit:
+ COMMIT;
+
+step check:
+ TABLE repack_test;
+
+i|j
+-+-
+(0 rows)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
diff --git a/src/test/modules/injection_points/specs/repack_running_xacts.spec b/src/test/modules/injection_points/specs/repack_running_xacts.spec
new file mode 100644
index 00000000000..1f878514046
--- /dev/null
+++ b/src/test/modules/injection_points/specs/repack_running_xacts.spec
@@ -0,0 +1,119 @@
+setup
+{
+ CREATE EXTENSION injection_points;
+ CREATE TABLE repack_test(i int PRIMARY KEY, j int);
+ CREATE TABLE aux(i int);
+}
+
+teardown
+{
+ DROP TABLE repack_test;
+ DROP TABLE aux;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+setup
+{
+ SELECT injection_points_attach('before-create-decoding-context', 'wait');
+}
+step repack
+{
+ REPACK (CONCURRENTLY) repack_test;
+}
+step check
+{
+ TABLE repack_test;
+}
+teardown
+{
+ SELECT injection_points_detach('before-create-decoding-context');
+}
+
+session s2
+step wakeup_bcdc
+{
+ SELECT injection_points_wakeup('before-create-decoding-context');
+}
+step wakeup_bet
+{
+ SELECT injection_points_wakeup('before-end-transaction');
+}
+
+session s3
+step s3_assign_xid
+{
+ BEGIN;
+ INSERT INTO aux VALUES (1);
+}
+step s3_commit
+{
+ COMMIT;
+}
+
+session s4
+step s4_changes
+{
+ BEGIN;
+ INSERT INTO repack_test(i, j) VALUES (1, 1);
+}
+# Do not attach in the setup section, that would be too soon.
+step s4_attach
+{
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('before-end-transaction', 'wait');
+}
+step s4_commit
+{
+ COMMIT;
+}
+teardown
+{
+ SELECT injection_points_detach('before-end-transaction');
+}
+
+session s5
+step s5_assign_xid
+{
+ BEGIN;
+ INSERT INTO aux VALUES (2);
+}
+step s5_commit
+{
+ COMMIT;
+}
+
+permutation
+repack
+# Assign XID so that a running transaction prevents the snapshot builder from
+# reaching CONSISTENT state immediately. It will wait for this to complete
+# after having reached BUILDING_SNAPSHOT.
+s3_assign_xid
+# Let the decoding setup start.
+wakeup_bcdc
+# Likewise, the snapshot builder will wait for the s4's xact to complete after
+# having reached FULL_SNAPSHOT. This is the problematic transaction, so let it
+# do some changes.
+s4_changes
+# Attach to the 'before-end-transaction' injection point that s4 will need
+# during commit.
+s4_attach
+# Only write commit record for s4, but do not remove the xact from procarray
+# yet. Thus the snapshot builder still needs to wait.
+s4_commit
+# Let the snapshot builder proceed to FULL_SNAPSHOT.
+s3_commit
+# Start another transaction so that CONSISTENT is not reached "directly",
+# i.e. due to no running transaction. It's important here that builder->xmin
+# does not advance.
+s5_assign_xid
+# Remove s4 xact from procarray, and thus reach the CONSISTENT state. Since
+# the COMMIT appeared in WAL too early (i.e. when the snapshot builder state
+# did not allow decoding of COMMIT records yet), the snapshot builder will
+# consider s4 running. This is also due to returning from
+# SnapBuildProcessRunningXacts() too early, w/o advancing builder->xmin.
+wakeup_bet
+# s5 is not needed anymore
+s5_commit
+# Show that the data changes performed by s4 are lost.
+check
--
2.47.3
--=-=-=--
^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH] pg_upgrade: add --initdb option to create the new cluster automatically
@ 2026-07-10 10:41 Bohyun Lee <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Bohyun Lee @ 2026-07-10 10:41 UTC (permalink / raw)
To: [email protected]
Hi,
This patch adds an --initdb option to pg_upgrade that automates the initdb
step currently required before running pg_upgrade.
Problem
—————
Before running pg_upgrade, users must manually run initdb with options that
exactly match the old cluster: WAL segment size, data checksum setting,
encoding, and locale. Getting these right is error-prone. A mismatch
results in a confusing check_control_data() failure after the user has
already invested time in setting up the new cluster. A related question was
raised before [1], where Jeff Davis discussed whether pg_upgrade should
perform initdb itself rather than requiring a pre-initialized cluster.
Solution
————
With --initdb, pg_upgrade handles this automatically. It derives the WAL
segment size and checksum setting from pg_control and invokes initdb with
the correct flags. The option refuses to proceed if the new cluster already
exists.
Testing
————
All existing pg_upgrade TAP tests pass. A new test, t/007_initdb_option.pl,
verifies the happy path end-to-end and checks that --initdb refuses to
overwrite an existing cluster.
Branch: https://github.com/LeeBohyun/postgres/tree/pg_upgrade_initdb
<https://github.com/LeeBohyun/postgres/treepg_upgrade_initdb;
Patch attached.
[1]
https://www.postgresql.org/message-id/2a7feb71dbdcea31478b3974b8075982ac2326d2.camel%40j-davis.com
Regards,
Bohyun Lee
Attachments:
[application/octet-stream] v1-pg_upgrade-initdb.patch (18.2K, ../../CAMPh8Mr8FbMnPct2Zom88UkpkddT48mZoY+m4rRxG9PdW6Fo+A@mail.gmail.com/3-v1-pg_upgrade-initdb.patch)
download | inline diff:
From 5710d523900c4091ab760e142d999411435cb164 Mon Sep 17 00:00:00 2001
From: Bohyun Lee <[email protected]>
Date: Fri, 10 Jul 2026 09:12:41 +0000
Subject: [PATCH] pg_upgrade: add --initdb option to create the new cluster
automatically
Historically, pg_upgrade requires the user to manually run initdb before
invoking pg_upgrade, passing options that exactly match the old cluster's
WAL segment size, data checksum setting, encoding, and locale. Getting
these right is error-prone: a mismatch causes pg_upgrade to fail with an
opaque check_control_data() error after the user has already gone through
the trouble of running initdb.
This patch adds a --initdb option that automates the initdb step. When
given, pg_upgrade starts the old server briefly, reads template0's locale
and encoding, derives the WAL segment size and checksum setting from the
old cluster's pg_control, and runs initdb with matching options. The
new cluster data directory must not already exist; pg_upgrade exits with
an error if it does, to avoid clobbering an existing installation.
The locale inspection requires a brief start of the old postmaster, which
is already done later in the normal pg_upgrade flow; here it is done
earlier, before the new cluster exists, using a temporary log directory.
Extra initdb options can be passed via the existing -O flag and will be
forwarded to the initdb invocation.
---
doc/src/sgml/ref/pgupgrade.sgml | 25 +++
src/bin/pg_upgrade/info.c | 3 +-
src/bin/pg_upgrade/option.c | 11 +-
src/bin/pg_upgrade/pg_upgrade.c | 178 ++++++++++++++++++++--
src/bin/pg_upgrade/pg_upgrade.h | 4 +
src/bin/pg_upgrade/t/007_initdb_option.pl | 105 +++++++++++++
6 files changed, 312 insertions(+), 14 deletions(-)
create mode 100644 src/bin/pg_upgrade/t/007_initdb_option.pl
diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml
index e4e8c02e6d6..902b0233805 100644
--- a/doc/src/sgml/ref/pgupgrade.sgml
+++ b/doc/src/sgml/ref/pgupgrade.sgml
@@ -262,6 +262,25 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--initdb</option></term>
+ <listitem>
+ <para>
+ Create the new cluster automatically by running
+ <command>initdb</command> before upgrading, instead of requiring the
+ user to have created it manually. The WAL segment size, data checksum
+ setting, encoding, and locale are derived from the old cluster so that
+ <application>pg_upgrade</application> can verify compatibility.
+ </para>
+ <para>
+ The new cluster data directory specified with
+ <option>-D</option>/<option>--new-datadir</option> must not already
+ exist when this option is given; if it does,
+ <application>pg_upgrade</application> will exit with an error.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>--no-statistics</option></term>
<listitem>
@@ -462,6 +481,12 @@ make prefix=/usr/local/pgsql.new install
prebuilt installers do this step automatically. There is no need to
start the new cluster.
</para>
+ <para>
+ Alternatively, pass <option>--initdb</option> to
+ <application>pg_upgrade</application> to have it run
+ <command>initdb</command> automatically, deriving the required settings
+ from the old cluster. In that case this manual step can be skipped.
+ </para>
</step>
<step>
diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
index 37fff93892f..65ae97cdc1f 100644
--- a/src/bin/pg_upgrade/info.c
+++ b/src/bin/pg_upgrade/info.c
@@ -21,7 +21,6 @@ static void create_rel_filename_map(const char *old_data, const char *new_data,
static void report_unmatched_relation(const RelInfo *rel, const DbInfo *db,
bool is_new_db);
static void free_db_and_rel_infos(DbInfoArr *db_arr);
-static void get_template0_info(ClusterInfo *cluster);
static void get_db_infos(ClusterInfo *cluster);
static char *get_rel_infos_query(void);
static void process_rel_infos(DbInfo *dbinfo, PGresult *res, void *arg);
@@ -328,7 +327,7 @@ get_db_rel_and_slot_infos(ClusterInfo *cluster)
* Get information about template0, which will be copied from the old cluster
* to the new cluster.
*/
-static void
+void
get_template0_info(ClusterInfo *cluster)
{
PGconn *conn = connectToServer(cluster, "template1");
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index f01d2f92d95..daaf48d47bc 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -63,6 +63,7 @@ parseCommandLine(int argc, char *argv[])
{"no-statistics", no_argument, NULL, 5},
{"set-char-signedness", required_argument, NULL, 6},
{"swap", no_argument, NULL, 7},
+ {"initdb", no_argument, NULL, 8},
{NULL, 0, NULL, 0}
};
@@ -234,6 +235,10 @@ parseCommandLine(int argc, char *argv[])
user_opts.transfer_mode = TRANSFER_MODE_SWAP;
break;
+ case 8:
+ user_opts.initdb_new_cluster = true;
+ break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
os_info.progname);
@@ -328,6 +333,8 @@ usage(void)
printf(_(" --clone clone instead of copying files to new cluster\n"));
printf(_(" --copy copy files to new cluster (default)\n"));
printf(_(" --copy-file-range copy files to new cluster with copy_file_range\n"));
+ printf(_(" --initdb create the new cluster with initdb before\n"
+ " upgrading (settings derived from old cluster)\n"));
printf(_(" --no-statistics do not import statistics from old cluster\n"));
printf(_(" --set-char-signedness=OPTION set new cluster char signedness to \"signed\" or\n"
" \"unsigned\"\n"));
@@ -336,7 +343,9 @@ usage(void)
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\n"
"Before running pg_upgrade you must:\n"
- " create a new database cluster (using the new version of initdb)\n"
+ " create a new database cluster (using the new version of initdb),\n"
+ " unless the --initdb option is given, in which case pg_upgrade\n"
+ " creates the new cluster for you\n"
" shutdown the postmaster servicing the old cluster\n"
" shutdown the postmaster servicing the new cluster\n"));
printf(_("\n"
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 7366fd4627c..b108d3beb0d 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -45,10 +45,13 @@
#include "access/multixact.h"
#include "catalog/pg_class_d.h"
+#include "catalog/pg_collation_d.h"
#include "common/file_perm.h"
#include "common/logging.h"
#include "common/restricted_token.h"
#include "fe_utils/string_utils.h"
+#include "fe_utils/version.h"
+#include "mb/pg_wchar.h"
#include "pg_upgrade.h"
/*
@@ -67,6 +70,8 @@ static void copy_xact_xlog_xid(void);
static void set_frozenxids(void);
static void make_outputdirs(char *pgdata);
static void setup(char *argv0);
+static void resolve_new_bindir(const char *argv0);
+static void create_new_cluster_via_initdb(void);
static void create_logical_replication_slots(void);
static void create_conflict_detection_slot(void);
@@ -107,6 +112,10 @@ main(int argc, char **argv)
get_restricted_token();
adjust_data_dir(&old_cluster);
+
+ if (user_opts.initdb_new_cluster)
+ create_new_cluster_via_initdb();
+
adjust_data_dir(&new_cluster);
/*
@@ -358,6 +367,163 @@ make_outputdirs(char *pgdata)
}
+/*
+ * resolve_new_bindir()
+ *
+ * Idempotent helper: if new_cluster.bindir has not been set by the user via
+ * -B, derive it from the path of the currently executing pg_upgrade binary.
+ * Called early by create_new_cluster_via_initdb() so that the initdb path
+ * is available before verify_directories() runs.
+ */
+static void
+resolve_new_bindir(const char *argv0)
+{
+ if (!new_cluster.bindir)
+ {
+ char exec_path[MAXPGPATH];
+
+ if (find_my_exec(argv0, exec_path) < 0)
+ pg_fatal("%s: could not find own program executable", argv0);
+ /* Trim off program name and keep just the directory */
+ *last_dir_separator(exec_path) = '\0';
+ canonicalize_path(exec_path);
+ new_cluster.bindir = pg_strdup(exec_path);
+ }
+}
+
+
+/*
+ * create_new_cluster_via_initdb()
+ *
+ * Implements --initdb: run initdb to create the new cluster before upgrading,
+ * deriving WAL segment size, data checksums, encoding, and locale settings
+ * from the old cluster so that check_control_data() passes.
+ *
+ * This runs before the normal verify_directories() / setup() path, so we
+ * use a temporary log directory under the new bindir for the early server
+ * start; make_outputdirs() will replace log_opts.logdir later.
+ */
+static void
+create_new_cluster_via_initdb(void)
+{
+ DbLocaleInfo *locale;
+ PQExpBufferData cmd;
+ char tmp_logdir[MAXPGPATH];
+ char *saved_logdir = log_opts.logdir;
+ const char *encoding_name;
+
+ resolve_new_bindir(os_info.progname);
+
+ /*
+ * Verify that initdb is present and executable before doing any work.
+ * The normal path checks this later inside verify_directories(), but we
+ * run before that, so fail early with a useful message.
+ */
+ {
+ char initdb_path[MAXPGPATH];
+
+ snprintf(initdb_path, sizeof(initdb_path), "%s/initdb",
+ new_cluster.bindir);
+ if (validate_exec(initdb_path) != 0)
+ pg_fatal("could not find \"initdb\" in \"%s\": %m\n"
+ "The --initdb option requires initdb to be present in the new cluster's bin directory.",
+ new_cluster.bindir);
+ }
+
+ old_cluster.major_version = get_pg_version(old_cluster.pgdata,
+ &old_cluster.major_version_str);
+
+ /*
+ * get_control_data() selects pg_resetwal vs. pg_resetxlog via
+ * bin_version, which check_bindir() normally fills in later. Seed it
+ * now so the right binary name is used in this early call.
+ */
+ if (old_cluster.bin_version == 0)
+ old_cluster.bin_version = old_cluster.major_version;
+
+ /* Refuse to overwrite an existing cluster. */
+ {
+ char verfile[MAXPGPATH];
+ struct stat st;
+
+ snprintf(verfile, sizeof(verfile), "%s/PG_VERSION",
+ new_cluster.pgdata);
+ if (stat(verfile, &st) == 0)
+ pg_fatal("new cluster data directory \"%s\" already contains a database system; "
+ "--initdb requires an empty or nonexistent directory",
+ new_cluster.pgdata);
+ }
+
+ get_control_data(&old_cluster);
+
+ /* Set up a temporary log directory for the early server start. */
+ snprintf(tmp_logdir, sizeof(tmp_logdir), "%s/pg_upgrade_initdb.log.d",
+ new_cluster.bindir);
+ if (mkdir(tmp_logdir, pg_dir_create_mode) < 0 && errno != EEXIST)
+ pg_fatal("could not create temporary log directory \"%s\": %m",
+ tmp_logdir);
+ log_opts.logdir = tmp_logdir;
+
+ if (!old_cluster.sockdir)
+ old_cluster.sockdir = user_opts.socketdir ? user_opts.socketdir : ".";
+
+ prep_status("Inspecting old cluster locale for new cluster creation");
+ start_postmaster(&old_cluster, true);
+ get_template0_info(&old_cluster);
+ stop_postmaster(false);
+ check_ok();
+
+ locale = old_cluster.template0;
+ encoding_name = pg_encoding_to_char(locale->db_encoding);
+
+ prep_status("Creating new cluster with initdb");
+
+ initPQExpBuffer(&cmd);
+ appendPQExpBuffer(&cmd, "\"%s/initdb\" -D \"%s\" -N",
+ new_cluster.bindir, new_cluster.pgdata);
+ appendPQExpBuffer(&cmd, " -U \"%s\"", os_info.user);
+ appendPQExpBuffer(&cmd, " --wal-segsize=%u",
+ old_cluster.controldata.walseg / (1024 * 1024));
+
+ /*
+ * Pass --data-checksums or --no-data-checksums explicitly. Starting
+ * from PG18, initdb enables checksums by default, so we must mirror the
+ * old cluster's setting to avoid a mismatch that check_control_data()
+ * would reject.
+ */
+ if (old_cluster.controldata.data_checksum_version != 0)
+ appendPQExpBufferStr(&cmd, " --data-checksums");
+ else
+ appendPQExpBufferStr(&cmd, " --no-data-checksums");
+
+ appendPQExpBuffer(&cmd, " --encoding=%s", encoding_name);
+ appendPQExpBuffer(&cmd, " --locale-provider=%s",
+ collprovider_name(locale->db_collprovider));
+ appendPQExpBuffer(&cmd, " --lc-collate=\"%s\" --lc-ctype=\"%s\"",
+ locale->db_collate, locale->db_ctype);
+
+ if (locale->db_locale)
+ {
+ if (locale->db_collprovider == COLLPROVIDER_ICU)
+ appendPQExpBuffer(&cmd, " --icu-locale=\"%s\"",
+ locale->db_locale);
+ else if (locale->db_collprovider == COLLPROVIDER_BUILTIN)
+ appendPQExpBuffer(&cmd, " --builtin-locale=\"%s\"",
+ locale->db_locale);
+ }
+
+ if (new_cluster.pgopts)
+ appendPQExpBuffer(&cmd, " %s", new_cluster.pgopts);
+
+ exec_prog(UTILITY_LOG_FILE, NULL, true, true, "%s", cmd.data);
+
+ termPQExpBuffer(&cmd);
+ log_opts.logdir = saved_logdir;
+
+ check_ok();
+}
+
+
static void
setup(char *argv0)
{
@@ -372,17 +538,7 @@ setup(char *argv0)
* with -B, default to using the path of the currently executed pg_upgrade
* binary.
*/
- if (!new_cluster.bindir)
- {
- char exec_path[MAXPGPATH];
-
- if (find_my_exec(argv0, exec_path) < 0)
- pg_fatal("%s: could not find own program executable", argv0);
- /* Trim off program name and keep just path */
- *last_dir_separator(exec_path) = '\0';
- canonicalize_path(exec_path);
- new_cluster.bindir = pg_strdup(exec_path);
- }
+ resolve_new_bindir(argv0);
verify_directories();
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index d6e5bca5792..199998e0ab1 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -325,6 +325,9 @@ typedef struct
int char_signedness; /* default char signedness: -1 for initial
* value, 1 for "signed" and 0 for
* "unsigned" */
+ bool initdb_new_cluster; /* run initdb to create the new cluster
+ * before upgrading, instead of requiring
+ * the user to have created it manually */
} UserOpts;
typedef struct
@@ -423,6 +426,7 @@ FileNameMap *gen_db_file_maps(DbInfo *old_db,
DbInfo *new_db, int *nmaps, const char *old_pgdata,
const char *new_pgdata);
void get_db_rel_and_slot_infos(ClusterInfo *cluster);
+void get_template0_info(ClusterInfo *cluster);
int count_old_cluster_logical_slots(void);
void get_subscription_info(ClusterInfo *cluster);
diff --git a/src/bin/pg_upgrade/t/007_initdb_option.pl b/src/bin/pg_upgrade/t/007_initdb_option.pl
new file mode 100644
index 00000000000..577ce7340a4
--- /dev/null
+++ b/src/bin/pg_upgrade/t/007_initdb_option.pl
@@ -0,0 +1,105 @@
+# Copyright (c) 2022-2025, PostgreSQL Global Development Group
+
+# Test the --initdb option of pg_upgrade: pg_upgrade creates the new cluster
+# itself via initdb, instead of requiring the user to have run initdb first.
+
+use strict;
+use warnings FATAL => 'all';
+
+use File::Path qw(rmtree);
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Initialize and populate the old cluster.
+my $oldnode = PostgreSQL::Test::Cluster->new('old_node');
+$oldnode->init;
+$oldnode->start;
+$oldnode->safe_psql('postgres',
+ "CREATE TABLE t (id int primary key, note text); "
+ . "INSERT INTO t SELECT g, 'row ' || g FROM generate_series(1, 100) g; "
+ . "CREATE DATABASE extra_db;");
+my $rows_before =
+ $oldnode->safe_psql('postgres', 'SELECT count(*) FROM t');
+is($rows_before, '100', 'old cluster has expected rows before upgrade');
+$oldnode->stop;
+
+# Create the new node object but do NOT init() it: pg_upgrade --initdb is
+# responsible for creating the data directory. Only new() runs, which
+# allocates the port/host/basedir the framework needs.
+my $newnode = PostgreSQL::Test::Cluster->new('new_node');
+
+my $oldbindir = $oldnode->config_data('--bindir');
+my $newbindir = $newnode->config_data('--bindir');
+
+# Sanity: the new data directory must not exist yet.
+ok(!-d $newnode->data_dir,
+ 'new cluster data directory does not exist before --initdb');
+
+# Run pg_upgrade with --initdb. We must run in a writable directory because
+# pg_upgrade writes output files relative to the current directory.
+chdir ${PostgreSQL::Test::Utils::tmp_check};
+
+command_ok(
+ [
+ 'pg_upgrade', '--no-sync',
+ '--old-datadir' => $oldnode->data_dir,
+ '--new-datadir' => $newnode->data_dir,
+ '--old-bindir' => $oldbindir,
+ '--new-bindir' => $newbindir,
+ '--socketdir' => $newnode->host,
+ '--old-port' => $oldnode->port,
+ '--new-port' => $newnode->port,
+ '--initdb',
+ ],
+ 'run of pg_upgrade --initdb creates and upgrades the new cluster');
+
+# The new data directory should now exist and be a v18+ cluster.
+ok(-f $newnode->data_dir . '/PG_VERSION',
+ 'new cluster data directory created by --initdb');
+
+# The framework's init() would normally write port/socket settings into
+# postgresql.conf; since we skipped it, append them now so we can start the
+# upgraded cluster through the test harness.
+my $conf = $newnode->data_dir . '/postgresql.conf';
+open(my $fh, '>>', $conf) or die "could not open $conf: $!";
+print $fh "\n# added by test to start the --initdb-created cluster\n";
+print $fh "port = " . $newnode->port . "\n";
+print $fh "listen_addresses = ''\n";
+print $fh "unix_socket_directories = '" . $newnode->host . "'\n";
+close($fh);
+
+$newnode->start;
+
+# Verify the user data survived the upgrade.
+my $rows_after = $newnode->safe_psql('postgres', 'SELECT count(*) FROM t');
+is($rows_after, '100', 'user data survived --initdb upgrade');
+
+# Verify the extra database carried over too.
+my $has_extra = $newnode->safe_psql('postgres',
+ "SELECT count(*) FROM pg_database WHERE datname = 'extra_db'");
+is($has_extra, '1', 'user database carried over by --initdb upgrade');
+
+# Verify the new cluster is a newer major version than the old one.
+my $newver = $newnode->safe_psql('postgres',
+ "SELECT current_setting('server_version_num')::int / 10000");
+ok($newver >= 18, "new cluster reports target major version ($newver)");
+
+$newnode->stop;
+
+# --initdb must refuse to clobber an already-populated data directory.
+command_fails(
+ [
+ 'pg_upgrade', '--no-sync',
+ '--old-datadir' => $oldnode->data_dir,
+ '--new-datadir' => $newnode->data_dir,
+ '--old-bindir' => $oldbindir,
+ '--new-bindir' => $newbindir,
+ '--socketdir' => $newnode->host,
+ '--old-port' => $oldnode->port,
+ '--new-port' => $newnode->port,
+ '--initdb',
+ ],
+ '--initdb refuses to overwrite an existing cluster');
+
+done_testing();
--
2.54.0
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2026-07-10 10:41 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-04-11 06:52 Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h} Michael Paquier <[email protected]>
2022-04-11 12:46 ` [email protected]
2022-04-12 09:22 ` Michael Paquier <[email protected]>
2022-04-13 05:25 ` Michael Paquier <[email protected]>
2022-04-13 14:58 ` [email protected]
2022-04-13 21:18 ` Michael Paquier <[email protected]>
2022-04-18 02:42 ` Michael Paquier <[email protected]>
2022-04-11 15:15 ` Robert Haas <[email protected]>
2022-04-11 22:50 ` Michael Paquier <[email protected]>
2026-05-12 10:27 [PATCH] Test to demonstrate bug in commit 0d3dba38c7 and to verify a fix. Antonin Houska <[email protected]>
2026-07-10 10:41 [PATCH] pg_upgrade: add --initdb option to create the new cluster automatically Bohyun Lee <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox