public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 09/10] Add zstd compression levels 3+ messages / 3 participants [nested] [flat]
* [PATCH 09/10] Add zstd compression levels @ 2021-03-14 22:12 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Justin Pryzby @ 2021-03-14 22:12 UTC (permalink / raw) --- src/backend/access/transam/xlog.c | 6 +++++- src/backend/access/transam/xloginsert.c | 14 +++++++++++++- src/backend/access/transam/xlogreader.c | 8 ++++++++ src/backend/utils/misc/guc.c | 2 +- src/include/access/xlog_internal.h | 4 ++++ 5 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 5a8447a525..2c6c97c1b3 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -99,7 +99,7 @@ bool EnableHotStandby = false; bool fullPageWrites = true; bool wal_log_hints = false; bool wal_compression = false; -int wal_compression_method = WAL_COMPRESSION_ZSTD; +int wal_compression_method = WAL_COMPRESSION_ZSTD_FAST_10; char *wal_consistency_checking_string = NULL; bool *wal_consistency_checking = NULL; bool wal_init_zero = true; @@ -192,6 +192,10 @@ const struct config_enum_entry wal_compression_options[] = { #endif #ifdef USE_ZSTD {"zstd", WAL_COMPRESSION_ZSTD, false}, + {"zstd-1", WAL_COMPRESSION_ZSTD_1, false}, + {"zstd-fast-10", WAL_COMPRESSION_ZSTD_FAST_10, false}, + {"zstd-fast-20", WAL_COMPRESSION_ZSTD_FAST_20, false}, + {"zstd-fast-40", WAL_COMPRESSION_ZSTD_FAST_40, false}, #endif {NULL, 0, false} }; diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c index 5d1ae37dae..9e6bc77717 100644 --- a/src/backend/access/transam/xloginsert.c +++ b/src/backend/access/transam/xloginsert.c @@ -902,7 +902,18 @@ XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length, #ifdef USE_ZSTD case WAL_COMPRESSION_ZSTD: - len = ZSTD_compress(dest, PGLZ_MAX_BLCKSZ, source, orig_len, 1); + case WAL_COMPRESSION_ZSTD_1: + case WAL_COMPRESSION_ZSTD_FAST_10: + case WAL_COMPRESSION_ZSTD_FAST_20: + case WAL_COMPRESSION_ZSTD_FAST_40: + { + int level = compression == WAL_COMPRESSION_ZSTD_1 ? 1 : + compression == WAL_COMPRESSION_ZSTD_FAST_10 ? -10 : + compression == WAL_COMPRESSION_ZSTD_FAST_20 ? -20 : + compression == WAL_COMPRESSION_ZSTD_FAST_40 ? -40 : + ZSTD_CLEVEL_DEFAULT; + + len = ZSTD_compress(dest, PGLZ_MAX_BLCKSZ, source, orig_len, level); if (ZSTD_isError(len)) { ereport(ERROR, @@ -913,6 +924,7 @@ XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length, } break; + } #endif default: diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c index caa1031d63..ec795a7b9f 100644 --- a/src/backend/access/transam/xlogreader.c +++ b/src/backend/access/transam/xlogreader.c @@ -1569,6 +1569,10 @@ wal_compression_name(WalCompression compression) case WAL_COMPRESSION_LZ4: return "lz4"; case WAL_COMPRESSION_ZSTD: + case WAL_COMPRESSION_ZSTD_1: + case WAL_COMPRESSION_ZSTD_FAST_10: + case WAL_COMPRESSION_ZSTD_FAST_20: + case WAL_COMPRESSION_ZSTD_FAST_40: return "zstd"; default: return "???"; @@ -1629,6 +1633,10 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page) #ifdef USE_ZSTD case WAL_COMPRESSION_ZSTD: + case WAL_COMPRESSION_ZSTD_1: + case WAL_COMPRESSION_ZSTD_FAST_10: + case WAL_COMPRESSION_ZSTD_FAST_20: + case WAL_COMPRESSION_ZSTD_FAST_40: decomp_result = ZSTD_decompress(tmp.data, BLCKSZ-bkpb->hole_length, ptr, bkpb->bimg_len); // XXX: ZSTD_getErrorName diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 8031e027aa..667fc4c0c1 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -4728,7 +4728,7 @@ static struct config_enum ConfigureNamesEnum[] = NULL }, &wal_compression_method, - WAL_COMPRESSION_ZSTD, wal_compression_options, + WAL_COMPRESSION_ZSTD_FAST_10, wal_compression_options, NULL, NULL, NULL }, diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h index 48b16b6083..affd6defc2 100644 --- a/src/include/access/xlog_internal.h +++ b/src/include/access/xlog_internal.h @@ -330,6 +330,10 @@ typedef enum WalCompression WAL_COMPRESSION_ZLIB, WAL_COMPRESSION_LZ4, WAL_COMPRESSION_ZSTD, + WAL_COMPRESSION_ZSTD_1, + WAL_COMPRESSION_ZSTD_FAST_10, /* level = -10 */ + WAL_COMPRESSION_ZSTD_FAST_20, /* level = -20 */ + WAL_COMPRESSION_ZSTD_FAST_40, /* level = -40 */ } WalCompression; extern const char *wal_compression_name(WalCompression compression); -- 2.17.0 --0qVF/w3MHQqLSynd-- ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Missing LWLock protection in pgstat_reset_replslot() @ 2024-03-05 07:55 Heikki Linnakangas <[email protected]> 2024-03-05 13:20 ` Re: Missing LWLock protection in pgstat_reset_replslot() Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Heikki Linnakangas @ 2024-03-05 07:55 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; [email protected] On 01/03/2024 12:15, Bertrand Drouvot wrote: > Hi hackers, > > I think that pgstat_reset_replslot() is missing LWLock protection. Indeed, we > don't have any guarantee that the slot is active (then preventing it to be > dropped/recreated) when this function is executed. Yes, so it seems at quick glance. We have a similar issue in pgstat_fetch_replslot(); it might return stats for wrong slot, if the slot is dropped/recreated concurrently. Do we care? > --- a/src/backend/utils/activity/pgstat_replslot.c > +++ b/src/backend/utils/activity/pgstat_replslot.c > @@ -46,6 +46,8 @@ pgstat_reset_replslot(const char *name) > > Assert(name != NULL); > > + LWLockAcquire(ReplicationSlotControlLock, LW_SHARED); > + > /* Check if the slot exits with the given name. */ > slot = SearchNamedReplicationSlot(name, true); SearchNamedReplicationSlot() will also acquire the lock in LW_SHARED mode, when you pass need_lock=true. So that at least should be changed to false. -- Heikki Linnakangas Neon (https://neon.tech) ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Missing LWLock protection in pgstat_reset_replslot() 2024-03-05 07:55 Re: Missing LWLock protection in pgstat_reset_replslot() Heikki Linnakangas <[email protected]> @ 2024-03-05 13:20 ` Bertrand Drouvot <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Bertrand Drouvot @ 2024-03-05 13:20 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: [email protected] Hi, On Tue, Mar 05, 2024 at 09:55:32AM +0200, Heikki Linnakangas wrote: > On 01/03/2024 12:15, Bertrand Drouvot wrote: > > Hi hackers, > > > > I think that pgstat_reset_replslot() is missing LWLock protection. Indeed, we > > don't have any guarantee that the slot is active (then preventing it to be > > dropped/recreated) when this function is executed. > > Yes, so it seems at quick glance. Thanks for looking at it! > We have a similar issue in > pgstat_fetch_replslot(); it might return stats for wrong slot, if the slot > is dropped/recreated concurrently. Good catch! > Do we care? Yeah, I think we should: done in v2 attached. > > --- a/src/backend/utils/activity/pgstat_replslot.c > > +++ b/src/backend/utils/activity/pgstat_replslot.c > > @@ -46,6 +46,8 @@ pgstat_reset_replslot(const char *name) > > Assert(name != NULL); > > + LWLockAcquire(ReplicationSlotControlLock, LW_SHARED); > > + > > /* Check if the slot exits with the given name. */ > > slot = SearchNamedReplicationSlot(name, true); > > SearchNamedReplicationSlot() will also acquire the lock in LW_SHARED mode, > when you pass need_lock=true. So that at least should be changed to false. > Right, done in v2. Also had to add an extra "need_lock" argument to get_replslot_index() for the same reason while taking care of pgstat_fetch_replslot(). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com Attachments: [text/x-diff] v2-0001-Adding-LWLock-protection-in-pgstat_reset_replslot.patch (3.2K, ../../ZeccNgXdFD79GMN%[email protected]/2-v2-0001-Adding-LWLock-protection-in-pgstat_reset_replslot.patch) download | inline diff: From fabee924c3131692addfe8941e4bdb3dc5540aae Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Fri, 1 Mar 2024 10:04:17 +0000 Subject: [PATCH v2] Adding LWLock protection in pgstat_reset_replslot() and pgstat_fetch_replslot() pgstat_reset_replslot() and pgstat_fetch_replslot() are missing a LWLock protection as we don't have any guarantee that the slot is active (then preventing it to be dropped/recreated) when those functions are executed. --- src/backend/utils/activity/pgstat_replslot.c | 35 +++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) 100.0% src/backend/utils/activity/ diff --git a/src/backend/utils/activity/pgstat_replslot.c b/src/backend/utils/activity/pgstat_replslot.c index 70cabf2881..7c409af670 100644 --- a/src/backend/utils/activity/pgstat_replslot.c +++ b/src/backend/utils/activity/pgstat_replslot.c @@ -30,7 +30,7 @@ #include "utils/pgstat_internal.h" -static int get_replslot_index(const char *name); +static int get_replslot_index(const char *name, bool need_lock); /* @@ -46,8 +46,10 @@ pgstat_reset_replslot(const char *name) Assert(name != NULL); + LWLockAcquire(ReplicationSlotControlLock, LW_SHARED); + /* Check if the slot exits with the given name. */ - slot = SearchNamedReplicationSlot(name, true); + slot = SearchNamedReplicationSlot(name, false); if (!slot) ereport(ERROR, @@ -60,11 +62,16 @@ pgstat_reset_replslot(const char *name) * slots. */ if (SlotIsPhysical(slot)) + { + LWLockRelease(ReplicationSlotControlLock); return; + } /* reset this one entry */ pgstat_reset(PGSTAT_KIND_REPLSLOT, InvalidOid, ReplicationSlotIndex(slot)); + + LWLockRelease(ReplicationSlotControlLock); } /* @@ -164,13 +171,25 @@ pgstat_drop_replslot(ReplicationSlot *slot) PgStat_StatReplSlotEntry * pgstat_fetch_replslot(NameData slotname) { - int idx = get_replslot_index(NameStr(slotname)); + int idx; + PgStat_StatReplSlotEntry *slotentry; + + LWLockAcquire(ReplicationSlotControlLock, LW_SHARED); + + idx = get_replslot_index(NameStr(slotname), false); if (idx == -1) + { + LWLockRelease(ReplicationSlotControlLock); return NULL; + } + + slotentry = (PgStat_StatReplSlotEntry *) pgstat_fetch_entry(PGSTAT_KIND_REPLSLOT, + InvalidOid, idx); + + LWLockRelease(ReplicationSlotControlLock); - return (PgStat_StatReplSlotEntry *) - pgstat_fetch_entry(PGSTAT_KIND_REPLSLOT, InvalidOid, idx); + return slotentry; } void @@ -189,7 +208,7 @@ pgstat_replslot_to_serialized_name_cb(const PgStat_HashKey *key, const PgStatSha bool pgstat_replslot_from_serialized_name_cb(const NameData *name, PgStat_HashKey *key) { - int idx = get_replslot_index(NameStr(*name)); + int idx = get_replslot_index(NameStr(*name), true); /* slot might have been deleted */ if (idx == -1) @@ -209,13 +228,13 @@ pgstat_replslot_reset_timestamp_cb(PgStatShared_Common *header, TimestampTz ts) } static int -get_replslot_index(const char *name) +get_replslot_index(const char *name, bool need_lock) { ReplicationSlot *slot; Assert(name != NULL); - slot = SearchNamedReplicationSlot(name, true); + slot = SearchNamedReplicationSlot(name, need_lock); if (!slot) return -1; -- 2.34.1 ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2024-03-05 13:20 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-14 22:12 [PATCH 09/10] Add zstd compression levels Justin Pryzby <[email protected]> 2024-03-05 07:55 Re: Missing LWLock protection in pgstat_reset_replslot() Heikki Linnakangas <[email protected]> 2024-03-05 13:20 ` Re: Missing LWLock protection in pgstat_reset_replslot() Bertrand Drouvot <[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