public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 01/10] Allow alternate compression methods for wal_compression 6+ messages / 5 participants [nested] [flat]
* [PATCH 01/10] Allow alternate compression methods for wal_compression @ 2021-02-27 04:03 Andrey Borodin <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Andrey Borodin @ 2021-02-27 04:03 UTC (permalink / raw) TODO: bump XLOG_PAGE_MAGIC --- doc/src/sgml/config.sgml | 17 +++++ src/backend/Makefile | 2 +- src/backend/access/transam/xlog.c | 10 +++ src/backend/access/transam/xloginsert.c | 52 +++++++++++++-- src/backend/access/transam/xlogreader.c | 63 ++++++++++++++++++- src/backend/utils/misc/guc.c | 11 ++++ src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/access/xlog.h | 1 + src/include/access/xlog_internal.h | 8 +++ src/include/access/xlogreader.h | 1 + src/include/access/xlogrecord.h | 9 +-- 11 files changed, 163 insertions(+), 12 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index a218d78bef..7fb2a84626 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -3072,6 +3072,23 @@ include_dir 'conf.d' </listitem> </varlistentry> + <varlistentry id="guc-wal-compression-method" xreflabel="wal_compression_method"> + <term><varname>wal_compressionion_method</varname> (<type>enum</type>) + <indexterm> + <primary><varname>wal_compression_method</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + This parameter selects the compression method used to compress WAL when + <varname>wal_compression</varname> is enabled. + The supported methods are pglz and zlib. + The default value is <literal>pglz</literal>. + Only superusers can change this setting. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-wal-init-zero" xreflabel="wal_init_zero"> <term><varname>wal_init_zero</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/Makefile b/src/backend/Makefile index 0da848b1fd..3af216ddfc 100644 --- a/src/backend/Makefile +++ b/src/backend/Makefile @@ -48,7 +48,7 @@ OBJS = \ LIBS := $(filter-out -lpgport -lpgcommon, $(LIBS)) $(LDAP_LIBS_BE) $(ICU_LIBS) # The backend doesn't need everything that's in LIBS, however -LIBS := $(filter-out -lz -lreadline -ledit -ltermcap -lncurses -lcurses, $(LIBS)) +LIBS := $(filter-out -lreadline -ledit -ltermcap -lncurses -lcurses, $(LIBS)) ifeq ($(with_systemd),yes) LIBS += -lsystemd diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index e04250f4e9..04192b7add 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -99,6 +99,7 @@ bool EnableHotStandby = false; bool fullPageWrites = true; bool wal_log_hints = false; bool wal_compression = false; +int wal_compression_method = WAL_COMPRESSION_PGLZ; char *wal_consistency_checking_string = NULL; bool *wal_consistency_checking = NULL; bool wal_init_zero = true; @@ -180,6 +181,15 @@ const struct config_enum_entry recovery_target_action_options[] = { {NULL, 0, false} }; +/* Note that due to conditional compilation, offsets within the array are not static */ +const struct config_enum_entry wal_compression_options[] = { + {"pglz", WAL_COMPRESSION_PGLZ, false}, +#ifdef HAVE_LIBZ + {"zlib", WAL_COMPRESSION_ZLIB, false}, +#endif + {NULL, 0, false} +}; + /* * Statistics for current checkpoint are collected in this global struct. * Because only the checkpointer or a stand-alone backend can perform diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c index 7052dc245e..34e1227381 100644 --- a/src/backend/access/transam/xloginsert.c +++ b/src/backend/access/transam/xloginsert.c @@ -33,6 +33,10 @@ #include "storage/proc.h" #include "utils/memutils.h" +#ifdef HAVE_LIBZ +#include <zlib.h> +#endif + /* Buffer size required to store a compressed version of backup block image */ #define PGLZ_MAX_BLCKSZ PGLZ_MAX_OUTPUT(BLCKSZ) @@ -113,7 +117,8 @@ static XLogRecData *XLogRecordAssemble(RmgrId rmid, uint8 info, XLogRecPtr RedoRecPtr, bool doPageWrites, XLogRecPtr *fpw_lsn, int *num_fpi); static bool XLogCompressBackupBlock(char *page, uint16 hole_offset, - uint16 hole_length, char *dest, uint16 *dlen); + uint16 hole_length, char *dest, + uint16 *dlen, WalCompression compression); /* * Begin constructing a WAL record. This must be called before the @@ -630,11 +635,12 @@ XLogRecordAssemble(RmgrId rmid, uint8 info, */ if (wal_compression) { + bimg.compression_method = wal_compression_method; is_compressed = XLogCompressBackupBlock(page, bimg.hole_offset, cbimg.hole_length, regbuf->compressed_page, - &compressed_len); + &compressed_len, bimg.compression_method); } /* @@ -827,7 +833,7 @@ XLogRecordAssemble(RmgrId rmid, uint8 info, */ static bool XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length, - char *dest, uint16 *dlen) + char *dest, uint16 *dlen, WalCompression compression) { int32 orig_len = BLCKSZ - hole_length; int32 len; @@ -853,12 +859,48 @@ XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length, else source = page; + switch (compression) + { + case WAL_COMPRESSION_PGLZ: + len = pglz_compress(source, orig_len, dest, PGLZ_strategy_default); + break; + +#ifdef HAVE_LIBZ + case WAL_COMPRESSION_ZLIB: + { + unsigned long len_l = PGLZ_MAX_BLCKSZ; + int ret; + ret = compress2((Bytef*)dest, &len_l, (Bytef*)source, orig_len, 1); + if (ret != Z_OK) + { + // XXX: using an interface other than compress() would allow giving a better error message + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("failed compressing zlib (%d)", ret))); + len_l = -1; + } + len = len_l; + break; + } +#endif + + default: + /* + * It should be impossible to get here for unsupported algorithms, + * which cannot be assigned if they're not enabled at compile time. + */ + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("unknown compression method requested: %d(%s)", + compression, wal_compression_name(compression)))); + + } + /* - * We recheck the actual size even if pglz_compress() reports success and + * We recheck the actual size even if compression reports success and * see if the number of bytes saved by compression is larger than the * length of extra data needed for the compressed version of block image. */ - len = pglz_compress(source, orig_len, dest, PGLZ_strategy_default); if (len >= 0 && len + extra_bytes < orig_len) { diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c index 42738eb940..afca22a26c 100644 --- a/src/backend/access/transam/xlogreader.c +++ b/src/backend/access/transam/xlogreader.c @@ -33,6 +33,10 @@ #include "utils/memutils.h" #endif +#ifdef HAVE_LIBZ +#include <zlib.h> +#endif + static void report_invalid_record(XLogReaderState *state, const char *fmt,...) pg_attribute_printf(2, 3); static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength); @@ -1286,6 +1290,7 @@ DecodeXLogRecord(XLogReaderState *state, XLogRecord *record, char **errormsg) { COPY_HEADER_FIELD(&blk->bimg_len, sizeof(uint16)); COPY_HEADER_FIELD(&blk->hole_offset, sizeof(uint16)); + COPY_HEADER_FIELD(&blk->compression_method, sizeof(uint8)); COPY_HEADER_FIELD(&blk->bimg_info, sizeof(uint8)); blk->apply_image = ((blk->bimg_info & BKPIMAGE_APPLY) != 0); @@ -1535,6 +1540,29 @@ XLogRecGetBlockData(XLogReaderState *record, uint8 block_id, Size *len) } } +/* + * Return a statically allocated string associated with the given compression + * method. This is similar to the guc, but isn't subject to conditional + * compilation. + */ +const char * +wal_compression_name(WalCompression compression) +{ + /* + * This could index into the guc array, except that it's compiled + * conditionally and unsupported methods are elided. + */ + switch (compression) + { + case WAL_COMPRESSION_PGLZ: + return "pglz"; + case WAL_COMPRESSION_ZLIB: + return "zlib"; + default: + return "???"; + } +} + /* * Restore a full-page image from a backup block attached to an XLOG record. * @@ -1558,8 +1586,39 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page) if (bkpb->bimg_info & BKPIMAGE_IS_COMPRESSED) { /* If a backup block image is compressed, decompress it */ - if (pglz_decompress(ptr, bkpb->bimg_len, tmp.data, - BLCKSZ - bkpb->hole_length, true) < 0) + int32 decomp_result = -1; + switch (bkpb->compression_method) + { + case WAL_COMPRESSION_PGLZ: + decomp_result = pglz_decompress(ptr, bkpb->bimg_len, tmp.data, + BLCKSZ - bkpb->hole_length, true); + break; + +#ifdef HAVE_LIBZ + case WAL_COMPRESSION_ZLIB: + { + unsigned long decomp_result_l; + decomp_result_l = BLCKSZ - bkpb->hole_length; + if (uncompress((Bytef*)tmp.data, &decomp_result_l, + (Bytef*)ptr, bkpb->bimg_len) == Z_OK) + decomp_result = decomp_result_l; + else + decomp_result = -1; + break; + } +#endif + + default: + report_invalid_record(record, "image at %X/%X is compressed with unsupported codec, block %d (%d/%s)", + (uint32) (record->ReadRecPtr >> 32), + (uint32) record->ReadRecPtr, + block_id, + bkpb->compression_method, + wal_compression_name(bkpb->compression_method)); + return false; + } + + if (decomp_result < 0) { report_invalid_record(record, "invalid compressed image at %X/%X, block %d", LSN_FORMAT_ARGS(record->ReadRecPtr), diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 855076b1fd..8084027465 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -508,6 +508,7 @@ extern const struct config_enum_entry archive_mode_options[]; extern const struct config_enum_entry recovery_target_action_options[]; extern const struct config_enum_entry sync_method_options[]; extern const struct config_enum_entry dynamic_shared_memory_options[]; +extern const struct config_enum_entry wal_compression_options[]; /* * GUC option variables that are exported from this module @@ -4721,6 +4722,16 @@ static struct config_enum ConfigureNamesEnum[] = NULL, NULL, NULL }, + { + {"wal_compression_method", PGC_SIGHUP, WAL_SETTINGS, + gettext_noop("Set the method used to compress full page images in the WAL."), + NULL + }, + &wal_compression_method, + WAL_COMPRESSION_PGLZ, wal_compression_options, + NULL, NULL, NULL + }, + { {"dynamic_shared_memory_type", PGC_POSTMASTER, RESOURCES_MEM, gettext_noop("Selects the dynamic shared memory implementation used."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index f46c2dd7a8..ef69a94492 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -213,6 +213,7 @@ # open_sync #full_page_writes = on # recover from partial page writes #wal_compression = off # enable compression of full-page writes +#wal_compression_method = pglz # pglz, zlib #wal_log_hints = off # also do full page writes of non-critical updates # (change requires restart) #wal_init_zero = on # zero-fill new WAL files diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 6d384d3ce6..fa2e5c611f 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -117,6 +117,7 @@ extern bool EnableHotStandby; extern bool fullPageWrites; extern bool wal_log_hints; extern bool wal_compression; +extern int wal_compression_method; extern bool wal_init_zero; extern bool wal_recycle; extern bool *wal_consistency_checking; diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h index b23e286406..d653839b97 100644 --- a/src/include/access/xlog_internal.h +++ b/src/include/access/xlog_internal.h @@ -324,4 +324,12 @@ extern bool InArchiveRecovery; extern bool StandbyMode; extern char *recoveryRestoreCommand; +typedef enum WalCompression +{ + WAL_COMPRESSION_PGLZ, + WAL_COMPRESSION_ZLIB, +} WalCompression; + +extern const char *wal_compression_name(WalCompression compression); + #endif /* XLOG_INTERNAL_H */ diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h index 21d200d3df..3d19c315d7 100644 --- a/src/include/access/xlogreader.h +++ b/src/include/access/xlogreader.h @@ -133,6 +133,7 @@ typedef struct bool apply_image; /* has image that should be restored */ char *bkp_image; uint16 hole_offset; + uint8 compression_method; uint16 hole_length; uint16 bimg_len; uint8 bimg_info; diff --git a/src/include/access/xlogrecord.h b/src/include/access/xlogrecord.h index 80c92a2498..0d4c212f15 100644 --- a/src/include/access/xlogrecord.h +++ b/src/include/access/xlogrecord.h @@ -114,7 +114,7 @@ typedef struct XLogRecordBlockHeader * present is (BLCKSZ - <length of "hole" bytes>). * * Additionally, when wal_compression is enabled, we will try to compress full - * page images using the PGLZ compression algorithm, after removing the "hole". + * page images, after removing the "hole". * This can reduce the WAL volume, but at some extra cost of CPU spent * on the compression during WAL logging. In this case, since the "hole" * length cannot be calculated by subtracting the number of page image bytes @@ -129,9 +129,10 @@ typedef struct XLogRecordBlockHeader */ typedef struct XLogRecordBlockImageHeader { - uint16 length; /* number of page image bytes */ - uint16 hole_offset; /* number of bytes before "hole" */ - uint8 bimg_info; /* flag bits, see below */ + uint16 length; /* number of page image bytes */ + uint16 hole_offset; /* number of bytes before "hole" */ + uint8 compression_method; /* compression method used for image */ + uint8 bimg_info; /* flag bits, see below */ /* * If BKPIMAGE_HAS_HOLE and BKPIMAGE_IS_COMPRESSED, an -- 2.17.0 --0qVF/w3MHQqLSynd Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0002-Run-011_crash_recovery.pl-with-wal_level-minimal.patch" ^ permalink raw reply [nested|flat] 6+ messages in thread
* Dump/Restore of non-default PKs @ 2022-04-18 19:59 Simon Riggs <[email protected]> 0 siblings, 2 replies; 6+ messages in thread From: Simon Riggs @ 2022-04-18 19:59 UTC (permalink / raw) To: pgsql-hackers At the moment you cannot create a unique index other than a btree. (As discussed on other threads, I am pursuing unique hash indexes for PostgreSQL, one step at a time). You get "ERROR index foo_idx is not a btree" According to parse_utilcmd.c line 2310, this is because it would break pg_dump, which needs ADD CONSTRAINT to create the same kind of index again. Fair enough. This is needed because ADD CONSTRAINT just uses the defaults index type. We could simply allow a GUC for default_primary_key_access_method, but that is overkill and there seems to be an easy and more general solution: I propose that we change pg_dump so that when it creates a PK it does so in 2 commands: 1. CREATE [UNIQUE] INDEX iname ... 2. ALTER TABLE .. ADD PRIMARY KEY USING INDEX iname; Step (1) recreates the index, respecting its AM, even if that is not a btree (2) works and there is no problem with defaults Doing this as 2 steps instead of one doesn't add any more time because (2) is just a metadata-only change, not an index build. Any objections to a patch to implement this thought? -- Simon Riggs http://www.EnterpriseDB.com/ ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Dump/Restore of non-default PKs @ 2022-04-18 20:20 David G. Johnston <[email protected]> parent: Simon Riggs <[email protected]> 1 sibling, 1 reply; 6+ messages in thread From: David G. Johnston @ 2022-04-18 20:20 UTC (permalink / raw) To: Simon Riggs <[email protected]>; +Cc: pgsql-hackers On Mon, Apr 18, 2022 at 1:00 PM Simon Riggs <[email protected]> wrote: > At the moment you cannot create a unique index other than a btree. (As > discussed on other threads, I am pursuing unique hash indexes for > PostgreSQL, one step at a time). > You get "ERROR index foo_idx is not a btree" > > According to parse_utilcmd.c line 2310, this is because it would break > pg_dump, which needs ADD CONSTRAINT to create the same kind of index > again. Fair enough. > > This is needed because ADD CONSTRAINT just uses the defaults index > type. We could simply allow a GUC for > default_primary_key_access_method, but that is overkill and there > seems to be an easy and more general solution: > > I propose that we change pg_dump so that when it creates a PK it does > so in 2 commands: > 1. CREATE [UNIQUE] INDEX iname ... > 2. ALTER TABLE .. ADD PRIMARY KEY USING INDEX iname; > > Step > (1) recreates the index, respecting its AM, even if that is not a btree > (2) works and there is no problem with defaults > > Doing this as 2 steps instead of one doesn't add any more time because > (2) is just a metadata-only change, not an index build. > > Any objections to a patch to implement this thought? > Why not just get rid of the limitation that constraint definitions don't support non-default methods? I.e., add syntax to index_parameters so that the underlying index can be defined directly. index_parameters in UNIQUE, PRIMARY KEY, and EXCLUDE constraints are: [ INCLUDE ( column_name [, ... ] ) ] [ WITH ( storage_parameter [= value] [, ... ] ) ] [ USING INDEX TABLESPACE tablespace_name ] We should add: [ USING INDEX METHOD index_method ] index_method := { BTREE | GIN | GIST | HASH | SPGIST | BRIN } David J. ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Dump/Restore of non-default PKs @ 2022-04-18 20:27 Tom Lane <[email protected]> parent: Simon Riggs <[email protected]> 1 sibling, 0 replies; 6+ messages in thread From: Tom Lane @ 2022-04-18 20:27 UTC (permalink / raw) To: Simon Riggs <[email protected]>; +Cc: pgsql-hackers Simon Riggs <[email protected]> writes: > I propose that we change pg_dump so that when it creates a PK it does > so in 2 commands: > 1. CREATE [UNIQUE] INDEX iname ... > 2. ALTER TABLE .. ADD PRIMARY KEY USING INDEX iname; > Step > (1) recreates the index, respecting its AM, even if that is not a btree > (2) works and there is no problem with defaults > Doing this as 2 steps instead of one doesn't add any more time because > (2) is just a metadata-only change, not an index build. I don't believe the claim that this adds no cost. Maybe it's negligible in context, but you've provided no evidence of that. (Parallel restore, where the ALTER would have to be a separate worker task, would probably be the worst case here.) Also, I assume your ambition would extend to supporting UNIQUE (but non-PKEY) constraints, so that would have to be done this way too. A potential advantage of doing things this way is that if we make pg_dump treat the index and the constraint as fully independent objects, that might allow some logic simplifications in pg_dump. Right now I think there are various weirdnesses in there that exist precisely because we don't want to dump them separately. One concern is that this'd create a hard compatibility break for loading dump output into servers that predate whenever we added ADD PRIMARY KEY USING INDEX. However, it looks like that syntax is accepted back to 9.1, so probably that's no issue in practice. Maybe a bigger concern for people who want to port to other RDBMSes? regards, tom lane ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Dump/Restore of non-default PKs @ 2022-04-18 20:48 Tom Lane <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Tom Lane @ 2022-04-18 20:48 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Simon Riggs <[email protected]>; pgsql-hackers "David G. Johnston" <[email protected]> writes: > On Mon, Apr 18, 2022 at 1:00 PM Simon Riggs <[email protected]> > wrote: >> I propose that we change pg_dump so that when it creates a PK it does >> so in 2 commands: >> 1. CREATE [UNIQUE] INDEX iname ... >> 2. ALTER TABLE .. ADD PRIMARY KEY USING INDEX iname; > Why not just get rid of the limitation that constraint definitions don't > support non-default methods? That approach would be doubling down on the assumption that we can always shoehorn more custom options into SQL-standard constraint clauses, and we'll never fall foul of shift/reduce problems or future spec additions. I think for example that USING INDEX TABLESPACE is a blot on humanity, and I'd be very glad to see pg_dump stop using it in favor of doing things as Simon suggests. regards, tom lane ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Dump/Restore of non-default PKs @ 2022-04-20 20:46 Peter Eisentraut <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Peter Eisentraut @ 2022-04-20 20:46 UTC (permalink / raw) To: Tom Lane <[email protected]>; David G. Johnston <[email protected]>; +Cc: Simon Riggs <[email protected]>; pgsql-hackers On 18.04.22 22:48, Tom Lane wrote: >> Why not just get rid of the limitation that constraint definitions don't >> support non-default methods? > That approach would be doubling down on the assumption that we can always > shoehorn more custom options into SQL-standard constraint clauses, and > we'll never fall foul of shift/reduce problems or future spec additions. When we do get the ability to create a table with a primary key with an underlying hash index, how would that be done? Would the only way be 1. create the table without primary key 2. create the index 3. attach the index as primary key constraint That doesn't sound attractive. ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2022-04-20 20:46 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-02-27 04:03 [PATCH 01/10] Allow alternate compression methods for wal_compression Andrey Borodin <[email protected]> 2022-04-18 19:59 Dump/Restore of non-default PKs Simon Riggs <[email protected]> 2022-04-18 20:20 ` Re: Dump/Restore of non-default PKs David G. Johnston <[email protected]> 2022-04-18 20:48 ` Re: Dump/Restore of non-default PKs Tom Lane <[email protected]> 2022-04-20 20:46 ` Re: Dump/Restore of non-default PKs Peter Eisentraut <[email protected]> 2022-04-18 20:27 ` Re: Dump/Restore of non-default PKs Tom Lane <[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