| From: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
|---|---|
| To: | Bryan Green <dbryan(dot)green(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Cc: | Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net> |
| Subject: | Re: pg_dump: use threads for parallel workers on all platforms |
| Date: | 2026-07-05 23:21:56 |
| Message-ID: | 3cccb1c0-4852-478c-9c5d-fbffea56ad53@iki.fi |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On 02/07/2026 19:30, Bryan Green wrote:
> None of this is broken; it works. It's threads pretending to be
> processes because the code was written for processes, and the port kept
> the protocol rather than rethinking it. I'd like to stop.
>
> One model everywhere should be threads on all platforms, coordinated by
> an in-process work queue-- a mutex and a couple of condition variables--
> instead of two worker models bridged by an inter-process protocol.
+1
> To be clear, the unification is on the queue, not on what Windows does
> today. Teaching the non-Windows side to talk to its own threads over a
> socket, a byte at a time, would just be the same trick on more
> platforms-- that's the part worth deleting, not copying.
>
> I've done the Windows half, both to prove it out and because it's the
> coordination layer the non-Windows side would adopt. ...
I see why you developed this that way, and it makes a lot of sense.
However, it has one downside: the Windows-only code cannot be tested
without Windows. At quick glance, it looks reasonable, but I'm a little
nervous committing more Windows-only code without being able to easily
play with it myself.
Once you have the final patch ready to switch non-Windows systems to
threaded model too, that gets easier.
> With the thread rework, fmtId's static return value, is now
> _Thread_local.
+1. This is the first _Thread_local in our codebase. It's in C11, so it
should just work, but we'll see if the buildfarm shows any surprises...
With this, the getLocalPQExpBuffer hook is never set. I think we can
just remove it, and rename defaultGetLocalPQExpBuffer() to
getLocalPQExpBuffer() directly.
I noticed that we currently call setFmtEncoding() in multiple places in
src/bin/pg_dump. I haven't looked at them closely, but I wonder if
there's some kind of thread-safety hazards there even without these patches.
And what about the 'quote_all_identifiers' global variable? Is that set
correctly in both models? I guess it's inherited through fork().
- Heikki
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Fujii Masao | 2026-07-05 23:37:19 | Re: Clear base backup progress reporting on error |
| Previous Message | Heikki Linnakangas | 2026-07-05 22:27:45 | Replace pg_atomic_flag with pg_atomic_bool |