I agree here. v2 (attached) passes a separate exception set through select_loop() and checks it on all platforms. The existing EINTR loop is unchanged.
All seven Windows assertions pass on native x64 and ARM64. Linux passes eight existing upgrade scripts plus EINTR, exceptional-readiness & refused-connect controls. I also tested it alongside the --initdb v8 proposal on 89e06d while retaining the test registrations. It applies to 1d45c5d. Best, Harrison
From 8eba156b6ca09882b14daa91cf46e8d1c17903cb Mon Sep 17 00:00:00 2001 From: Harrison Booth <[email protected]> Date: Wed, 14 Oct 2026 19:38:23 +0000 Subject: [PATCH v2] pg_upgrade: handle failed nonblocking connects on Windows Winsock reports a failed nonblocking connection in the exception set. Monitor it so pg_upgrade can pass the failure back to PQconnectPoll and try the next resolved address instead of waiting indefinitely. Following Heikki Linnakangas's suggestion, pass a distinct exception set through select_loop and check it independently of writability on all platforms. Leave the existing EINTR retry behavior unchanged. Add a Windows regression with successful-first IPv4 and refused-first localhost controls. Establish refusal with a bounded raw-socket Perl subprocess, preserving the connect error before socket cleanup. Discussion: https://www.postgresql.org/message-id/CAAH-eSVf=nvwb6p_heplihdkpunnzanlygq7chlsej54tau...@mail.gmail.com Discussion: https://www.postgresql.org/message-id/[email protected] --- src/bin/pg_upgrade/meson.build | 1 + src/bin/pg_upgrade/t/009_connect_failure.pl | 136 ++++++++++++++++++++ src/bin/pg_upgrade/task.c | 16 ++- 3 files changed, 149 insertions(+), 4 deletions(-) create mode 100644 src/bin/pg_upgrade/t/009_connect_failure.pl diff --git a/src/bin/pg_upgrade/meson.build b/src/bin/pg_upgrade/meson.build index ffbf6ae8d759bf8d95559c735d846ba1869347d3..9b5f2688626ff89504e089ec3399313d7db7e5da 100644 --- a/src/bin/pg_upgrade/meson.build +++ b/src/bin/pg_upgrade/meson.build @@ -69,6 +69,7 @@ tests += { 't/006_transfer_modes.pl', 't/007_multixact_conversion.pl', 't/008_extension_control_path.pl', + 't/009_connect_failure.pl', ], 'deps': [test_ext], 'test_kwargs': {'priority': 40}, # pg_upgrade tests are slow diff --git a/src/bin/pg_upgrade/t/009_connect_failure.pl b/src/bin/pg_upgrade/t/009_connect_failure.pl new file mode 100644 index 0000000000000000000000000000000000000000..55c8a62a2d3e660984ade6cd88044b4d62f2ae14 --- /dev/null +++ b/src/bin/pg_upgrade/t/009_connect_failure.pl @@ -0,0 +1,136 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Failed nonblocking connects must wake pg_upgrade's Windows task loop. +use strict; +use warnings FATAL => 'all'; + +use IPC::Run; +use Errno qw(ECONNREFUSED); +use Socket + qw(AF_UNSPEC AF_INET6 SOCK_STREAM IPPROTO_TCP NI_NUMERICHOST + getaddrinfo getnameinfo sockaddr_in6 inet_pton); +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +plan skip_all => 'Windows select() connection-failure behavior' + unless $windows_os; + +# This trigger needs localhost to try IPv6 before the IPv4-only server. +# Skip explicitly when the resolver cannot supply that ordering. +my ($lookup_error, @addresses) = getaddrinfo('localhost', '5432', + { family => AF_UNSPEC, socktype => SOCK_STREAM }); +plan skip_all => 'localhost address lookup failed' if $lookup_error; +my @hosts; +for my $address (@addresses) +{ + my ($name_error, $host) = getnameinfo($address->{addr}, NI_NUMERICHOST); + plan skip_all => 'numeric localhost lookup failed' if $name_error; + push @hosts, $host; +} +plan skip_all => 'localhost must resolve to ::1 first and include 127.0.0.1' + unless @hosts && $hosts[0] eq '::1' && grep { $_ eq '127.0.0.1' } @hosts; + +my $oldnode = PostgreSQL::Test::Cluster->new('old_node'); +my $newnode = PostgreSQL::Test::Cluster->new('new_node'); +$oldnode->init(extra => ['--locale=C']); +$newnode->init(extra => ['--locale=C']); +$oldnode->append_conf('postgresql.conf', "listen_addresses = '127.0.0.1'"); +$newnode->append_conf('postgresql.conf', "listen_addresses = '127.0.0.1'"); +$oldnode->start; + +# Confirm refusal in a bounded subprocess. Capture the native connect error +# before closing the socket; IO::Socket::IP can overwrite it during cleanup. +my $refusal_probe = <<'PERL'; +use strict; +use warnings; +use Errno qw(ECONNREFUSED); +use Socket qw(AF_INET6 SOCK_STREAM IPPROTO_TCP sockaddr_in6 inet_pton); +socket(my $socket, AF_INET6, SOCK_STREAM, IPPROTO_TCP) + or die "cannot create IPv6 socket: $!"; +my $connected = connect($socket, + sockaddr_in6($ARGV[0], inet_pton(AF_INET6, '::1'))); +my $refused = $!{ECONNREFUSED}; +my $error = "$!"; +close $socket; +if ($connected) { print "ACCEPTED\n"; exit 1; } +if ($refused) { print "REFUSED\n"; exit 0; } +print STDERR "IPv6 endpoint did not refuse the connection: $error\n"; +exit 2; +PERL +my ($probe_stdout, $probe_stderr, $probe_status); +my $probe_ok = eval { + IPC::Run::run([$^X, '-e', $refusal_probe, $oldnode->port], + '>' => \$probe_stdout, '2>' => \$probe_stderr, + IPC::Run::timeout(3)); + $probe_status = $?; + 1; +}; +my $probe_error = $@; +if (!$probe_ok || !defined $probe_status || $probe_status != 0 + || $probe_stdout ne "REFUSED\n") +{ + $oldnode->stop; + plan skip_all => "IPv6 refusal precondition not established: " + . ($probe_error || $probe_stderr || $probe_stdout); +} + +# pg_upgrade writes its logs relative to its working directory. +chdir $PostgreSQL::Test::Utils::tmp_check; + +my @command = ( + 'pg_upgrade', '--check', '--no-sync', '--jobs=2', + '--old-datadir' => $oldnode->data_dir, + '--new-datadir' => $newnode->data_dir, + '--old-bindir' => $oldnode->config_data('--bindir'), + '--new-bindir' => $newnode->config_data('--bindir'), + '--old-port' => $oldnode->port, + '--new-port' => $newnode->port); +my @checks; +for my $host ('127.0.0.1', 'localhost') +{ + my ($stdout, $stderr) = ('', ''); + my ($harness, $error, $status); + { + # IPv4 is the successful-first control; localhost must fall back. + local $ENV{PGHOST} = $host; + local $ENV{PGHOSTADDR}; + delete $ENV{PGHOSTADDR}; + my $timer = + IPC::Run::timeout($PostgreSQL::Test::Utils::timeout_default); + eval { + $harness = IPC::Run::start( + \@command, + '>' => \$stdout, + '2>' => \$stderr, + $timer); + $harness->finish; + $status = $?; + }; + $error = $@; + $harness->kill_kill if $error && defined $harness; + } + push @checks, [ $host, $stdout, $stderr, $error, $status ]; + last if $error || !defined $status || $status; +} + +# Cleanup precedes assertions and includes a server left after a timeout. +$newnode->_update_pid(-1); +$newnode->stop; +$oldnode->stop; + +for my $check (@checks) +{ + my ($host, $stdout, $stderr, $error, $status) = @$check; + ok( !$error && defined $status && $status == 0, + "pg_upgrade --check completes with host=$host"); + like( + $stdout, + qr/Clusters are compatible/, + "pg_upgrade checks both clusters with host=$host"); + diag("$error\n$stdout\n$stderr") if $error || !defined $status || $status; +} +is(scalar @checks, 2, 'both successful-first and refused-first cases ran'); +ok(!-e $oldnode->data_dir . '/postmaster.pid', 'old server is stopped'); +ok(!-e $newnode->data_dir . '/postmaster.pid', 'new server is stopped'); +done_testing(); diff --git a/src/bin/pg_upgrade/task.c b/src/bin/pg_upgrade/task.c index b6eb29e1f3a635870e5c3abd4077d450be389099..12e118bfcab35b95b3baf6cf02aeae222ffafe23 100644 --- a/src/bin/pg_upgrade/task.c +++ b/src/bin/pg_upgrade/task.c @@ -329,10 +329,11 @@ process_slot(const ClusterInfo *cluster, UpgradeTaskSlot *slot, const UpgradeTas * Returns -1 on error, else the number of ready descriptors. */ static int -select_loop(int maxFd, fd_set *input, fd_set *output) +select_loop(int maxFd, fd_set *input, fd_set *output, fd_set *exceptions) { fd_set save_input = *input; fd_set save_output = *output; + fd_set save_exceptions = *exceptions; if (maxFd == 0) return 0; @@ -344,7 +345,8 @@ select_loop(int maxFd, fd_set *input, fd_set *output) *input = save_input; *output = save_output; - i = select(maxFd + 1, input, output, NULL, NULL); + *exceptions = save_exceptions; + i = select(maxFd + 1, input, output, exceptions, NULL); #ifndef WIN32 if (i < 0 && errno == EINTR) @@ -366,10 +368,12 @@ wait_on_slots(UpgradeTaskSlot *slots, int numslots) { fd_set input; fd_set output; + fd_set exceptions; int maxFd = 0; FD_ZERO(&input); FD_ZERO(&output); + FD_ZERO(&exceptions); for (int i = 0; i < numslots; i++) { @@ -396,13 +400,16 @@ wait_on_slots(UpgradeTaskSlot *slots, int numslots) if (slots[i].sock < 0) pg_fatal("invalid socket"); FD_SET(slots[i].sock, slots[i].select_mode ? &input : &output); + /* Failed nonblocking connects are exceptional on Windows. */ + if (!slots[i].select_mode) + FD_SET(slots[i].sock, &exceptions); maxFd = Max(maxFd, slots[i].sock); } /* * If we found socket(s) to wait on, wait. */ - if (select_loop(maxFd, &input, &output) == -1) + if (select_loop(maxFd, &input, &output, &exceptions) == -1) pg_fatal("%s() failed: %m", "select"); /* @@ -410,7 +417,8 @@ wait_on_slots(UpgradeTaskSlot *slots, int numslots) */ for (int i = 0; i < numslots; i++) slots[i].ready |= (FD_ISSET(slots[i].sock, &input) || - FD_ISSET(slots[i].sock, &output)); + FD_ISSET(slots[i].sock, &output) || + FD_ISSET(slots[i].sock, &exceptions)); } /* base-commit: 158b6d5109fad9d5f0c30693fa85041a1c621fe1
