Reapply graceful socket shutdown on Windows (revert 29992a6a509)

On Thu, Oct 08, 2026 at 09:32:30PM +0100, Zsolt Parragi wrote:

I did extensive local testing, and I was able to reproduce the
walreceiver failure with both 29992a6a509 and a8458f508a7 reverted,
but not with only reverting 29992a6a509.

Okay, noted. Did you run some of the in-core TAP tests locally or did
something else? Could it make sense to stress the CI a few times
without a8458f508a7a, with the two unstable tests, with one of your
earlier tricks to increase the reproduction rate and confirm that a
failure shows up? Then repeat the same process with a8458f508a7a and
confirm that the problems are gone?

The point is to be able to test on my end and confirm manually your
report, and be able to do that without me having to spin a WIN32
environment just for this purpose. :)

Based on this, and the earlier discussion already in [1], I think we
should try reverting e0f633ca6bc (the windows specific test fix) and
29992a6a509 (the revert of the proper fix) on master, so that we can
watch the buildfarm and see if my analysis is correct.

The attached patches are simple clean reverts of these two commits.

Perhaps merging both of them together makes the most sense, so as we
never finish with a point in the tree where a failure shows up. Or
just reverse the order of your v1-0001 and your v1-0002 if both things
are not merged together.
--
Michael

Okay, noted. Did you run some of the in-core TAP tests locally or did
something else?

My initial repro was a custom tap test with a 2% failure rate, but
later I also got commit_ts/002_standby to reliably fail with a mostly
unmodified postgres: it needs wal_receiver_status_interval=0 and an
additional sleep. 300-500ms seems to work both on github actions and
on my pc, above that 002_standby can start failing differently
unrelated to the changes. In that case, the version with the required
fixes reverted still fails the same way as it did a few years ago, but
it also fails with all fixes applied, just differently.

I attached the patch for testing. The version I ran on github actions
is a bit more complex because of the additional debugging of the above
additional failure, but the idea is the same, and the github logs link
to the specific commit used there.

GitHub CI runs on my fork:
1000ms, proposed version failing differently:
https://github.com/dutow/postgres/actions/runs/38007567970
500ms: https://github.com/dutow/postgres/actions/runs/38008340610
300ms: https://github.com/dutow/postgres/actions/runs/38008343536

In all 3 runs master doesn't reproduce, proposed doesn't reproduce
(1000ms sleep version fails differently), proposed-nopeek (required
fix reverted) fails reliably, proposed-nopeek-nosleep (same, but no
sleep added) doesn't fail reliably.

On Thu, Oct 8, 2026 at 1:32 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

Based on this, and the earlier discussion already in [1], I think we
should try reverting e0f633ca6bc (the windows specific test fix) and
29992a6a509 (the revert of the proper fix) on master, so that we can
watch the buildfarm and see if my analysis is correct.

Comments on the second revert (I realize you weren't the original author):

+ * However, that does not work on Windows: if the kernel closes the
+ * socket it will invoke an "abortive shutdown" that discards any data
+ * not yet sent to the client.  (This is a flat-out violation of the
+ * TCP RFCs, but count on Microsoft not to care about that.)

*Does* this violate any RFCs? It seems like Windows is welcome to do
this from the point of view of TCP, annoying and underdocumented
though it may be.

+ ... When using OpenSSL, it seems that clean
+ * shutdown also requires an explicit shutdown() call.
...
+#ifdef WIN32
+ shutdown(MyProcPort->sock, SD_SEND);
+ closesocket(MyProcPort->sock);
+#endif

This still smells wrong to me. (Better, maybe.) Are we certain that
the RST we see without a shutdown(), when using OpenSSL, is the same
as the RST we see when we don't linger? Tom's last comment from the
original thread [1]/messages/by-id/3504774.1638901327@sss.pgh.pa.us is

Hmm. I wonder whether using SD_BOTH behaves any differently.

With shutdown(MyProcPort->sock, SD_BOTH) the test failed for me on
iterations 1, 2, 3, 16 (just as without shutdown() at all).
So shutdown with the SD_SEND flag definitely behaves much better (I've
seen over 200 successful iterations).

Fun. Well, I'll put in shutdown with SD_SEND for the moment,
and we'll have to see whether we can improve further than that.
It does sound like we may be running into OpenSSL bugs/oddities,
not only kernel issues, so it may be impossible to do better
on our side.

That really makes it seem like we haven't navigated the SSL_shutdown()
dance. Sending a TLS close_notify is presumably going to induce the
peer to send one back, if it hasn't already; if we immediately slam
the socket shut before reading that off the wire, then yeah, the OS is
going to reset the connection. (I say "presumably" because libpq is
using nonblocking sockets, and we don't check the result of
SSL_shutdown there, so we may not actually guarantee that the client
ever sends it. We don't check the result on the server side, either,
so if OpenSSL is trying to tell us that we're not supposed to be
calling SSL_shutdown right now, or there's some other strange problem,
we won't know.)

So to me it looks there may be multiple races/interactions going on
here. I don't want to block the patch if it gives us an empirical
improvement, but the comments may need to be adjusted, at least, while
we figure this out.

Thanks,
--Jacob

[1]: /messages/by-id/3504774.1638901327@sss.pgh.pa.us