Content-Type: multipart/mixed; boundary="------------rL4iKe0sAhy13aHkfDO6jk02"
Message-ID: <da4f9ec1-a272-bd2d-4ca1-6eb58d6dc79a@gmail.com>
Date: Sun, 8 Sep 2024 18:00:00 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: Yet another way for pg_ctl stop to fail on Windows
Content-Language: en-US
To: Noah Misch <noah@leadboat.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
References: <ba498e06-d22a-5bfb-389b-031f8db62d7d@gmail.com>
 <20240907181143.11.nmisch@google.com>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: <20240907181143.11.nmisch@google.com>
Archived-At: <https://www.postgresql.org/message-id/da4f9ec1-a272-bd2d-4ca1-6eb58d6dc79a%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------rL4iKe0sAhy13aHkfDO6jk02
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

07.09.2024 21:11, Noah Misch wrote:

>
>> Noah, what do you think of handling this error in line with handling of
>> ERROR_BROKEN_PIPE and ERROR_BAD_PIPE (which was done in 0ea1f2a3a)?
>>
>> I tried the following change:
>>          switch (GetLastError())
>>          {
>>                  case ERROR_BROKEN_PIPE:
>>                  case ERROR_BAD_PIPE:
>> +               case ERROR_PIPE_BUSY:
>> and saw no issues.
> That would be a strict improvement over returning EINVAL like we do today.  We
> do use PIPE_UNLIMITED_INSTANCES, so I expect the causes of ERROR_PIPE_BUSY are
> process exit and ENOMEM-like situations.  While that change is the best thing
> if the process is exiting, it could silently drop the signal in ENOMEM-like
> situations.  Consider the following alternative.  If sig==0, just return 0
> like you propose, because the process isn't completely gone.  Otherwise, sleep
> and retry the signal, like pgwin32_open_handle() retries after certain errors.
> What do you think of that?

Thank you for your attention to the issue!

I agree with your approach. It looks like Microsoft recommends to loop on
ERROR_PIPE_BUSY: [1] (they say "Calling CallNamedPipe is equivalent to
calling the CreateFile ..." at [2]).

So if we aim to not only fix "pg_ctl stop", but to make pgkill() robust,
it's the way to go, IMHO. I'm not sure about an infinite loop they show,
I'd vote for a loop with the same characteristics as in
pgwin32_open_handle().

I've managed to trigger ERROR_PIPE_BUSY with "pg_ctl reload", when running
20 TAP tests (see attached) in parallel (with 20 threads executing
$node->reload() in each), and with the kill() call inside do_reload()
repeated x100 (see the modification patch attached too):
...
# Running: pg_ctl -D .../099_test_pgkill\data/t_099_test_pgkill_node_data/pgdata reload
### Reloading node "node"
# Running: pg_ctl -D .../099_test_pgkill\data/t_099_test_pgkill_node_data/pgdata reload
[13:41:46.850](2.400s) # 18
server signaled
server signaled
server signaled
server signaled
server signaled
server signaled
server signaled
server signaled
!!!pgkill| GetLastError(): 231
pg_ctl: could not send reload signal (PID: 3912), iteration: 81, res: -1, errno: 22: Invalid argument
server signaled
[13:41:52.594](5.744s) # 19
...

[1] https://learn.microsoft.com/en-us/windows/win32/ipc/named-pipe-client
[2] https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-callnamedpipea

Best regards,
Alexander
--------------rL4iKe0sAhy13aHkfDO6jk02
Content-Type: text/x-patch; charset=UTF-8; name="pgkill-debugging.patch"
Content-Disposition: attachment; filename="pgkill-debugging.patch"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iaW4vcGdfY3RsL3BnX2N0bC5jIGIvc3JjL2Jpbi9wZ19jdGwv
cGdfY3RsLmMKaW5kZXggZTdlODc4YzIyZi4uY2FiNmViOTQ3MiAxMDA2NDQKLS0tIGEvc3Jj
L2Jpbi9wZ19jdGwvcGdfY3RsLmMKKysrIGIvc3JjL2Jpbi9wZ19jdGwvcGdfY3RsLmMKQEAg
LTExNjcsMTIgKzExNzQsMTcgQEAgZG9fcmVsb2FkKHZvaWQpCiAJCWV4aXQoMSk7CiAJfQog
Ci0JaWYgKGtpbGwocGlkLCBzaWcpICE9IDApCitmb3IgKGludCBpID0gMTsgaSA8PSAxMDA7
IGkrKykKK3sKK2ludCByZXMsIGVuOworCWlmICgocmVzID0ga2lsbChwaWQsIHNpZykpICE9
IDApCiAJewotCQl3cml0ZV9zdGRlcnIoXygiJXM6IGNvdWxkIG5vdCBzZW5kIHJlbG9hZCBz
aWduYWwgKFBJRDogJWQpOiAlbVxuIiksCi0JCQkJCSBwcm9nbmFtZSwgKGludCkgcGlkKTsK
K2VuID0gZXJybm87CisJCXdyaXRlX3N0ZGVycihfKCIlczogY291bGQgbm90IHNlbmQgcmVs
b2FkIHNpZ25hbCAoUElEOiAlZCksIGl0ZXJhdGlvbjogJWQsIHJlczogJWQsIGVycm5vOiAl
ZDogJW1cbiIpLAorCQkJCQkgcHJvZ25hbWUsIChpbnQpIHBpZCwgaSwgcmVzLCBlbik7CiAJ
CWV4aXQoMSk7CiAJfQorfQogCiAJcHJpbnRfbXNnKF8oInNlcnZlciBzaWduYWxlZFxuIikp
OwogfQpkaWZmIC0tZ2l0IGEvc3JjL3BvcnQva2lsbC5jIGIvc3JjL3BvcnQva2lsbC5jCmlu
ZGV4IDQxMmMyZjE5YzEuLmIzZDJjMDA3OTYgMTAwNjQ0Ci0tLSBhL3NyYy9wb3J0L2tpbGwu
YworKysgYi9zcmMvcG9ydC9raWxsLmMKQEAgLTcwLDYgKzcwLDcgQEAgcGdraWxsKGludCBw
aWQsIGludCBzaWcpCiAJCXJldHVybiAwOwogCX0KIAoraW50IGxlID0gR2V0TGFzdEVycm9y
KCk7CiAJc3dpdGNoIChHZXRMYXN0RXJyb3IoKSkKIAl7CiAJCWNhc2UgRVJST1JfQlJPS0VO
X1BJUEU6CkBAIC04OSw2ICs5MCw3IEBAIHBna2lsbChpbnQgcGlkLCBpbnQgc2lnKQogCQkJ
ZXJybm8gPSBFUEVSTTsKIAkJCXJldHVybiAtMTsKIAkJZGVmYXVsdDoKK2ZwcmludGYoc3Rk
ZXJyLCAiISEhcGdraWxsfCBHZXRMYXN0RXJyb3IoKTogJWRcbiIsIGxlKTsKIAkJCWVycm5v
ID0gRUlOVkFMOwkJLyogdW5leHBlY3RlZCAqLwogCQkJcmV0dXJuIC0xOwogCX0K
--------------rL4iKe0sAhy13aHkfDO6jk02
Content-Type: application/x-perl; name="099_test_pgkill.pl"
Content-Disposition: attachment; filename="099_test_pgkill.pl"
Content-Transfer-Encoding: 7bit

use strict;
use warnings;

use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More tests => 1;

use threads;
use threads::shared;

sub reload {
    my @args = @_;
    my ($thread_num, $node) = @args;
    my $i = 1;
    $SIG{'KILL'} = sub { print("Thread killed on iteration $i.\n"); threads->exit(); };
    print("Thread $thread_num started\n");
    while (1) {
		$node->reload();
		$i++;
	}
}

sub shutdown_threads {
    my @args = @_;
    my (@threads) = @args;
    for my $thread (@threads) {
        if ($thread->is_running()) {
            $thread->kill('KILL')->detach;
        }
    }
    return;
}


my $node = PostgreSQL::Test::Cluster->new("node");
$node->init;
$node->start;

my $numthreads = 20;
my @threads;
foreach my $ti (1 .. $numthreads) {
    push @threads, threads->create('reload', $ti, $node);
}
my $max_duration = 100;

my $failed_threads = 0;
my $duration = 0;
while (1) {
    foreach my $ti (1 .. $numthreads) {
        my $thread = $threads[$ti - 1];
        if ($thread->is_joinable()) {
            my $result = $thread->join();
            $failed_threads += 1;
            diag("Thread $ti failed:\n$result");
        } elsif (!$thread->is_running()) {
            $failed_threads += 1;
            diag("Thread $ti is not running:\n");
        }
    }
    if ($failed_threads > 0) {
        last;
    }
    sleep(1);
    diag("$duration\n");
    $duration += 1;
    if ($duration >= $max_duration) {
        last;
    }
}

diag($failed_threads);
shutdown_threads(@threads);
ok($failed_threads == 0);

--------------rL4iKe0sAhy13aHkfDO6jk02--