From: Justin Pryzby <pryzby@telsasoft.com>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Jacob Champion <jchampion@timescale.com>
Cc: pgsql-hackers@postgresql.org, gkokolatos@pm.me, Michael Paquier <michael@paquier.xyz>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Dipesh Pandit <dipesh.pandit@gmail.com>
Cc: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: Mark Dilger <mark.dilger@enterprisedb.com>
Subject: Re: zstd compression for pg_dump
Date: Sat, 1 Apr 2023 08:36:44 -0500
Message-ID: <ZCgzbBVKLvwpqMzT@telsasoft.com> (raw)
In-Reply-To: <2de3bcad-6da9-9022-bce5-fc0416351d6e@enterprisedb.com>
References: <CAAWbhmgm8yzKLnwK8-Wj-nhYo3R0AvLF6FSX-Pgo=062hGz4JA@mail.gmail.com>
<3d04afca-7d9d-c90f-5fef-5cf4fdb2173f@timescale.com>
<ZBKgBz+4blcKru6x@telsasoft.com>
<0115b682-58f0-ea43-9ecd-c37a4495812e@enterprisedb.com>
<ZCHSSv3DgxUa0f3L@telsasoft.com>
<0022dd9e-a439-d72f-d09b-ec512c23949b@enterprisedb.com>
<ZCdpzywVGl0BLan2@telsasoft.com>
<4029fe58-f661-7d8d-cbbb-b936a05531e2@enterprisedb.com>
<ZCd6sb7XA/W6nLaD@telsasoft.com>
<2de3bcad-6da9-9022-bce5-fc0416351d6e@enterprisedb.com>
On Sat, Apr 01, 2023 at 02:49:44PM +0200, Tomas Vondra wrote:
> On 4/1/23 02:28, Justin Pryzby wrote:
> > On Sat, Apr 01, 2023 at 02:11:12AM +0200, Tomas Vondra wrote:
> >> On 4/1/23 01:16, Justin Pryzby wrote:
> >>> On Tue, Mar 28, 2023 at 06:23:26PM +0200, Tomas Vondra wrote:
> >>>> On 3/27/23 19:28, Justin Pryzby wrote:
> >>>>> On Fri, Mar 17, 2023 at 03:43:31AM +0100, Tomas Vondra wrote:
> >>>>>> On 3/16/23 05:50, Justin Pryzby wrote:
> >>>>>>> On Fri, Mar 10, 2023 at 12:48:13PM -0800, Jacob Champion wrote:
> >>>>>>>> On Wed, Mar 8, 2023 at 10:59 AM Jacob Champion <jchampion@timescale.com> wrote:
> >>>>>>>>> I did some smoke testing against zstd's GitHub release on Windows. To
> >>>>>>>>> build against it, I had to construct an import library, and put that
> >>>>>>>>> and the DLL into the `lib` folder expected by the MSVC scripts...
> >>>>>>>>> which makes me wonder if I've chosen a harder way than necessary?
> >>>>>>>>
> >>>>>>>> It looks like pg_dump's meson.build is missing dependencies on zstd
> >>>>>>>> (meson couldn't find the headers in the subproject without them).
> >>>>>>>
> >>>>>>> I saw that this was added for LZ4, but I hadn't added it for zstd since
> >>>>>>> I didn't run into an issue without it. Could you check that what I've
> >>>>>>> added works for your case ?
> >>>>>>>
> >>>>>>>>> Parallel zstd dumps seem to work as expected, in that the resulting
> >>>>>>>>> pg_restore output is identical to uncompressed dumps and nothing
> >>>>>>>>> explodes. I haven't inspected the threading implementation for safety
> >>>>>>>>> yet, as you mentioned.
> >>>>>>>>
> >>>>>>>> Hm. Best I can tell, the CloneArchive() machinery is supposed to be
> >>>>>>>> handling safety for this, by isolating each thread's state. I don't feel
> >>>>>>>> comfortable pronouncing this new addition safe or not, because I'm not
> >>>>>>>> sure I understand what the comments in the format-specific _Clone()
> >>>>>>>> callbacks are saying yet.
> >>>>>>>
> >>>>>>> My line of reasoning for unix is that pg_dump forks before any calls to
> >>>>>>> zstd. Nothing zstd does ought to affect the pg_dump layer. But that
> >>>>>>> doesn't apply to pg_dump under windows. This is an opened question. If
> >>>>>>> there's no solid answer, I could disable/ignore the option (maybe only
> >>>>>>> under windows).
> >>>>>>
> >>>>>> I may be missing something, but why would the patch affect this? Why
> >>>>>> would it even affect safety of the parallel dump? And I don't see any
> >>>>>> changes to the clone stuff ...
> >>>>>
> >>>>> zstd supports using threads during compression, with -Z zstd:workers=N.
> >>>>> When unix forks, the child processes can't do anything to mess up the
> >>>>> state of the parent processes.
> >>>>>
> >>>>> But windows pg_dump uses threads instead of forking, so it seems
> >>>>> possible that the pg_dump -j threads that then spawn zstd threads could
> >>>>> "leak threads" and break the main thread. I suspect there's no issue,
> >>>>> but we still ought to verify that before declaring it safe.
> >>>>
> >>>> OK. I don't have access to a Windows machine so I can't test that. Is it
> >>>> possible to disable the zstd threading, until we figure this out?
> >>>
> >>> I think that's what's best. I made it issue a warning if "workers" was
> >>> specified. It could also be an error, or just ignored.
> >>>
> >>> I considered disabling workers only for windows, but realized that I
> >>> haven't tested with threads myself - my local zstd package is compiled
> >>> without threading, and I remember having some issue recompiling it with
> >>> threading. Jacob's recipe for using meson wraps works well, but it
> >>> still seems better to leave it as a future feature. I used that recipe
> >>> to enabled zstd with threading on CI (except for linux/autoconf).
> >>
> >> +1 to disable this if we're unsure it works correctly. I agree it's
> >> better to just error out if workers are requested - I rather dislike
> >> when a tool just ignores an explicit parameter. And AFAICS it's what
> >> zstd does too, when someone requests workers on incompatible build.
> >>
> >> FWIW I've been thinking about this a bit more and I don't quite see why
> >> would the threading cause issues (except for Windows). I forgot
> >> pg_basebackup already supports zstd, including the worker threading, so
> >> why would it work there and not in pg_dump? Sure, pg_basebackup is not
> >> parallel, but with separate pg_dump processes that shouldn't be an issue
> >> (although I'm not sure when zstd creates threads).
> >
> > There's no concern at all except under windows (because on windows
> > pg_dump -j is implemented using threads rather than forking).
> > Especially since zstd:workers is already allowed in the basebackup
> > backend process.
>
> If there are no concerns, why disable it outside Windows? I don't have a
> good idea how beneficial the multi-threaded compression is, so I can't
> quite judge the risk/benefits tradeoff.
Because it's a minor/fringe feature, and it's annoying to have platform
differences (would we plan on relaxing the restriction in v17, or is it
more likely we'd forget ?).
I realized how little I've tested with zstd workers myself. And I think
on cirrusci, the macos and freebsd tasks have zstd libraries with
threading support, but it wasn't being exercised (because using :workers
would cause the patch to fail unless it's supported everywhere). So I
updated the "for CI only" patch to 1) use meson wraps to compile zstd
library with threading on linux and windows; and, 2) use zstd:workers=3
"opportunistically" (but avoid failing if threads are not supported,
since the autoconf task still doesn't have access to a library with
thread support). That's a great step, but it still seems bad that the
thread stuff has been little exercised until now. (Also, the windows
task failed; I think that's due to a transient network issue).
Feel free to mess around with threads (but I'd much rather see the patch
progress for zstd:long).
--
Justin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, tomas.vondra@enterprisedb.com, jchampion@timescale.com, michael@paquier.xyz, robertmhaas@gmail.com, pg@bowt.ie, peter.eisentraut@enterprisedb.com, hlinnaka@iki.fi, thomas.munro@gmail.com, dipesh.pandit@gmail.com, x4mmm@yandex-team.ru, mark.dilger@enterprisedb.com
Subject: Re: zstd compression for pg_dump
In-Reply-To: <ZCgzbBVKLvwpqMzT@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox