On 7/5/2026 6:21 PM, Heikki Linnakangas wrote:
> 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.
>
I have reworked the patches: 0001-0002 are standalone for Windows.
0003-0005 are dependent on Thomas Munro's pthread.h. >> 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.
>
Agreed.
> 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.
>
There is fmtIdEncoding which is a plain static, and on Windows each dump
worker already calls setup_connection() -> setFmtEncoding() on its own
thread. Every writer stores the same value (the encoding is fixed for
the whole archive), so it's a same-value race. On the restore side the
leader sets it once (processEncodingEntry) and the workers only read it.
> And what about the 'quote_all_identifiers' global variable? Is that set
> correctly in both models? I guess it's inherited through fork().
>
getopt_long writes it during option parsing (--quote-all-identifiers
points straight at the global), before any worker or connection exists,
and nothing touches it again.
> - Heikki
>
I expect changes to be needed. Thomas, I hope this is more of a help
than not.
0001-0002 stand alone and touch only Windows: fmtId()'s scratch buffer
becomes _Thread_local, and the Windows workers exchange work over an
in-process channel rather than a loopback socket driven by select().
They're useful on their own and don't depend on the rest.
0003-0005 do the unification on top of Thomas Munro's pg_threads.h, so
you will need his patch
[v1-0002-port-Provide-minimal-pg_threads.h-API.patch] to install
pg_threads API: 0003 ports the channel's locks to that API (mechanical,
no behavior change); 0004 switches all platforms to threads and deletes
the fork(), pgpipe() and select() code; 0005 drops the string command
protocol for plain structs, now that the leader and workers share an
address space.
Changes since v1:
- v1's _beginthreadex-failure check was committed separately as
75e201bf95, so it's dropped here.
- Dropped the standalone 64-worker-cap patch; 0004 removes that limit
for free by joining workers individually.
- Extended the series past Windows: 0003-0005 unify all platforms on
threads atop pg_threads.h.
- Per review, fmtId()'s getLocalPQExpBuffer indirection is removed in
0001 now that nothing overrides it.
--
Bryan Green
EDB: https://www.enterprisedb.com
