public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 04/10] wal_compression_method: default to zlib.. 5+ messages / 5 participants [nested] [flat]
* [PATCH 04/10] wal_compression_method: default to zlib.. @ 2021-03-11 23:36 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Justin Pryzby @ 2021-03-11 23:36 UTC (permalink / raw) this is meant to exercise the CIs, and not meant to be merged --- src/backend/access/transam/xlog.c | 2 +- src/backend/utils/misc/guc.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 6eb46ea8a7..f5d4450654 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_PGLZ; +int wal_compression_method = WAL_COMPRESSION_ZLIB; char *wal_consistency_checking_string = NULL; bool *wal_consistency_checking = NULL; bool wal_init_zero = true; diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 8084027465..c37a8313d3 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1269,7 +1269,7 @@ static struct config_bool ConfigureNamesBool[] = NULL }, &wal_compression, - false, + true, NULL, NULL, NULL }, @@ -4728,7 +4728,7 @@ static struct config_enum ConfigureNamesEnum[] = NULL }, &wal_compression_method, - WAL_COMPRESSION_PGLZ, wal_compression_options, + WAL_COMPRESSION_ZLIB, wal_compression_options, NULL, NULL, NULL }, -- 2.17.0 --XsQoSWH+UP9D9v3l Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0005-re-add-wal_compression_method-lz4.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Atomic ops for unlogged LSN @ 2023-05-24 21:49 Andres Freund <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Andres Freund @ 2023-05-24 21:49 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Michael Paquier <[email protected]>; John Morris <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]> Hi, On 2023-05-24 08:22:08 -0400, Robert Haas wrote: > On Tue, May 23, 2023 at 6:26 PM Michael Paquier <[email protected]> wrote: > > Spinlocks provide a full memory barrier, which may not the case with > > add_u64() or read_u64(). Could memory ordering be a problem in these > > code paths? > > It could be a huge problem if what you say were true, but unless I'm > missing something, the comments in atomics.h say that it isn't. The > comments for the 64-bit functions say to look at the 32-bit functions, > and there it says: > > /* > * pg_atomic_add_fetch_u32 - atomically add to variable > * > * Returns the value of ptr after the arithmetic operation. > * > * Full barrier semantics. > */ > > Which is probably a good thing, because I'm not sure what good it > would be to have an operation that both reads and modifies an atomic > variable but has no barrier semantics. I was a bit confused by Michael's comment as well, due to the section of code quoted. But he does have a point: pg_atomic_read_u32() does indeed *not* have barrier semantics (it'd be way more expensive), and the patch does contain this hunk: > @@ -6784,9 +6775,7 @@ CreateCheckPoint(int flags) > * unused on non-shutdown checkpoints, but seems useful to store it always > * for debugging purposes. > */ > - SpinLockAcquire(&XLogCtl->ulsn_lck); > - ControlFile->unloggedLSN = XLogCtl->unloggedLSN; > - SpinLockRelease(&XLogCtl->ulsn_lck); > + ControlFile->unloggedLSN = pg_atomic_read_u64(&XLogCtl->unloggedLSN); > > UpdateControlFile(); > LWLockRelease(ControlFileLock); So we indeed loose some "barrier strength" - but I think that's fine. For one, it's a debugging-only value. But more importantly, I don't see what reordering the barrier could prevent - a barrier is useful for things like sequencing two memory accesses to happen in the intended order - but unloggedLSN doesn't interact with another variable that's accessed within the ControlFileLock afaict. > That's not to say that I entirely understand the point of this patch. > It seems like a generally reasonable thing to do AFAICT but somehow I > wonder if there's more to the story here, because it doesn't feel like > it would be easy to measure the benefit of this patch in isolation. > That's not a reason to reject it, though, just something that makes me > curious. I doubt it's a meaningful, if even measurable win. But removing atomic ops and reducing shared memory space isn't a bad thing, even if there's no immediate benefit... Greetings, Andres Freund ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Atomic ops for unlogged LSN @ 2023-05-24 22:41 Michael Paquier <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Michael Paquier @ 2023-05-24 22:41 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; John Morris <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]> On Wed, May 24, 2023 at 02:49:58PM -0700, Andres Freund wrote: > I was a bit confused by Michael's comment as well, due to the section of code > quoted. But he does have a point: pg_atomic_read_u32() does indeed *not* have > barrier semantics (it'd be way more expensive), and the patch does contain > this hunk: Thanks for the correction. The part about _add was incorrect. > So we indeed loose some "barrier strength" - but I think that's fine. For one, > it's a debugging-only value. But more importantly, I don't see what reordering > the barrier could prevent - a barrier is useful for things like sequencing two > memory accesses to happen in the intended order - but unloggedLSN doesn't > interact with another variable that's accessed within the ControlFileLock > afaict. This stuff is usually tricky enough that I am never completely sure whether it is fine without reading again README.barrier, which is where unloggedLSN is saved in the control file under its LWLock. Something that I find confusing in the patch is that it does not document the reason why this is OK. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Atomic ops for unlogged LSN @ 2023-06-12 23:05 Nathan Bossart <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Nathan Bossart @ 2023-06-12 23:05 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; John Morris <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]> On Thu, May 25, 2023 at 07:41:21AM +0900, Michael Paquier wrote: > On Wed, May 24, 2023 at 02:49:58PM -0700, Andres Freund wrote: >> So we indeed loose some "barrier strength" - but I think that's fine. For one, >> it's a debugging-only value. Is it? I see uses in GiST indexing (62401db), so it's not immediately obvious to me how it is debugging-only. If it is, then I think this patch ought to clearly document it so that nobody else tries to use it for non-debugging-only stuff. >> But more importantly, I don't see what reordering >> the barrier could prevent - a barrier is useful for things like sequencing two >> memory accesses to happen in the intended order - but unloggedLSN doesn't >> interact with another variable that's accessed within the ControlFileLock >> afaict. > > This stuff is usually tricky enough that I am never completely sure > whether it is fine without reading again README.barrier, which is > where unloggedLSN is saved in the control file under its LWLock. > Something that I find confusing in the patch is that it does not > document the reason why this is OK. My concern would be whether GetFakeLSNForUnloggedRel or CreateCheckPoint might see an old value of unloggedLSN. From the following note in README.barrier, it sounds like this would be a problem even if we ensured full barrier semantics: 3. No ordering guarantees. While memory barriers ensure that any given process performs loads and stores to shared memory in order, they don't guarantee synchronization. In the queue example above, we can use memory barriers to be sure that readers won't see garbage, but there's nothing to say whether a given reader will run before or after a given writer. If this matters in a given situation, some other mechanism must be used instead of or in addition to memory barriers. IIUC we know that shared memory accesses cannot be reordered to precede aquisition or follow release of a spinlock (thanks to 0709b7e), which is why this isn't a problem in the current implementation. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Atomic ops for unlogged LSN @ 2023-06-12 23:24 Stephen Frost <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Stephen Frost @ 2023-06-12 23:24 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Robert Haas <[email protected]>; John Morris <[email protected]>; pgsql-hackers Greetings, * Nathan Bossart ([email protected]) wrote: > On Thu, May 25, 2023 at 07:41:21AM +0900, Michael Paquier wrote: > > On Wed, May 24, 2023 at 02:49:58PM -0700, Andres Freund wrote: > >> So we indeed loose some "barrier strength" - but I think that's fine. For one, > >> it's a debugging-only value. > > Is it? I see uses in GiST indexing (62401db), so it's not immediately > obvious to me how it is debugging-only. If it is, then I think this patch > ought to clearly document it so that nobody else tries to use it for > non-debugging-only stuff. I don't see it as a debugging value. I'm not sure where that came from..? We do use it in places and if anything, I expect it to be used more, not less, in the future as a persistant generally increasing value (could go backwards on a crash-restart but in such case all unlogged tables are truncated). > >> But more importantly, I don't see what reordering > >> the barrier could prevent - a barrier is useful for things like sequencing two > >> memory accesses to happen in the intended order - but unloggedLSN doesn't > >> interact with another variable that's accessed within the ControlFileLock > >> afaict. > > > > This stuff is usually tricky enough that I am never completely sure > > whether it is fine without reading again README.barrier, which is > > where unloggedLSN is saved in the control file under its LWLock. > > Something that I find confusing in the patch is that it does not > > document the reason why this is OK. > > My concern would be whether GetFakeLSNForUnloggedRel or CreateCheckPoint > might see an old value of unloggedLSN. From the following note in > README.barrier, it sounds like this would be a problem even if we ensured > full barrier semantics: > > 3. No ordering guarantees. While memory barriers ensure that any given > process performs loads and stores to shared memory in order, they don't > guarantee synchronization. In the queue example above, we can use memory > barriers to be sure that readers won't see garbage, but there's nothing to > say whether a given reader will run before or after a given writer. If this > matters in a given situation, some other mechanism must be used instead of > or in addition to memory barriers. > > IIUC we know that shared memory accesses cannot be reordered to precede > aquisition or follow release of a spinlock (thanks to 0709b7e), which is > why this isn't a problem in the current implementation. The concern around unlogged LSN, imv anyway, is less about shared memory access and making sure that all callers understand that it can move backwards on a crash/restart. I don't think that's an issue for current users but we just need to make sure to try and comment sufficiently regarding that such that new users understand that about this particular value. Thanks, Stephen Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-06-12 23:24 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-11 23:36 [PATCH 04/10] wal_compression_method: default to zlib.. Justin Pryzby <[email protected]> 2023-05-24 21:49 Re: Atomic ops for unlogged LSN Andres Freund <[email protected]> 2023-05-24 22:41 ` Re: Atomic ops for unlogged LSN Michael Paquier <[email protected]> 2023-06-12 23:05 ` Re: Atomic ops for unlogged LSN Nathan Bossart <[email protected]> 2023-06-12 23:24 ` Re: Atomic ops for unlogged LSN Stephen Frost <[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