On 06/09/2024 12:52, Heikki Linnakangas wrote:
Unless you have comments on these first two patches which just add
tests, I'll commit them shortly. Still processing the rest of your
comments...
Didn't happen as "shortly" as I thought..
My test for dead-end backends opens 20 TCP (or unix domain) connections
to the server, in quick succession. That works fine my system, and it
passed cirrus CI on other platforms, but on FreeBSD it failed
repeatedly. The behavior in that scenario is apparently
platform-dependent: it depends on the accept queue size, but what
happens when you reach the queue size also seems to depend on the
platform. On my Linux system, the connect() calls in the client are
blocked, if the server is doesn't call accept() fast enough, but
apparently you get an error on *BSD systems.
I'm not sure of the exact details, but in any case, platform-dependent
behavior needs to be avoided in tests. So I changed the test so that it
sends an SSLRequest packet on each connection and waits for reply (which
is always 'N' to reject it in this test), before opening the next
connection. This way, each connection is still left hanging, which is
what I want in this test, but only after postmaster has successfully
accept()ed it and forked the backend.
So here are these test patches again, with that addition.
--
Heikki Linnakangas
Neon (https://neon.tech)
From 16993fde8153b4feced18e6a49d8b470e0a49241 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Fri, 4 Oct 2024 20:15:36 +0300
Subject: [PATCH v5 1/3] Add test for connection limits
Reviewed-by: Andres Freund <and...@anarazel.de>
Discussion: https://www.postgresql.org/message-id/a102f15f-eac4-4ff2-af02-f9ff209ec...@iki.fi
---
src/test/Makefile | 2 +-
src/test/meson.build | 1 +
src/test/postmaster/Makefile | 23 ++++++
src/test/postmaster/README | 27 +++++++
src/test/postmaster/meson.build | 12 +++
.../postmaster/t/001_connection_limits.pl | 79 +++++++++++++++++++
6 files changed, 143 insertions(+), 1 deletion(-)
create mode 100644 src/test/postmaster/Makefile
create mode 100644 src/test/postmaster/README
create mode 100644 src/test/postmaster/meson.build
create mode 100644 src/test/postmaster/t/001_connection_limits.pl
diff --git a/src/test/Makefile b/src/test/Makefile
index dbd3192874..abdd6e5a98 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -12,7 +12,7 @@ subdir = src/test
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
-SUBDIRS = perl regress isolation modules authentication recovery subscription
+SUBDIRS = perl postmaster regress isolation modules authentication recovery subscription
ifeq ($(with_icu),yes)
SUBDIRS += icu
diff --git a/src/test/meson.build b/src/test/meson.build
index c3d0dfedf1..67376e4b7f 100644
--- a/src/test/meson.build
+++ b/src/test/meson.build
@@ -4,6 +4,7 @@ subdir('regress')
subdir('isolation')
subdir('authentication')
+subdir('postmaster')
subdir('recovery')
subdir('subscription')
subdir('modules')
diff --git a/src/test/postmaster/Makefile b/src/test/postmaster/Makefile
new file mode 100644
index 0000000000..dfcce9c9ee
--- /dev/null
+++ b/src/test/postmaster/Makefile
@@ -0,0 +1,23 @@
+#-------------------------------------------------------------------------
+#
+# Makefile for src/test/postmaster
+#
+# Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
+# Portions Copyright (c) 1994, Regents of the University of California
+#
+# src/test/postmaster/Makefile
+#
+#-------------------------------------------------------------------------
+
+subdir = src/test/postmaster
+top_builddir = ../../..
+include $(top_builddir)/src/Makefile.global
+
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
+
+clean distclean:
+ rm -rf tmp_check
diff --git a/src/test/postmaster/README b/src/test/postmaster/README
new file mode 100644
index 0000000000..7e47bf5cff
--- /dev/null
+++ b/src/test/postmaster/README
@@ -0,0 +1,27 @@
+src/test/postmaster/README
+
+Regression tests for postmaster
+===============================
+
+This directory contains a test suite for postmaster's handling of
+connections, connection limits, and startup/shutdown sequence.
+
+
+Running the tests
+=================
+
+NOTE: You must have given the --enable-tap-tests argument to configure.
+
+Run
+ make check
+or
+ make installcheck
+You can use "make installcheck" if you previously did "make install".
+In that case, the code in the installation tree is tested. With
+"make check", a temporary installation tree is built from the current
+sources and then tested.
+
+Either way, this test initializes, starts, and stops a test Postgres
+cluster.
+
+See src/test/perl/README for more info about running these tests.
diff --git a/src/test/postmaster/meson.build b/src/test/postmaster/meson.build
new file mode 100644
index 0000000000..c2de2e0eb5
--- /dev/null
+++ b/src/test/postmaster/meson.build
@@ -0,0 +1,12 @@
+# Copyright (c) 2022-2024, PostgreSQL Global Development Group
+
+tests += {
+ 'name': 'postmaster',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'tap': {
+ 'tests': [
+ 't/001_connection_limits.pl',
+ ],
+ },
+}
diff --git a/src/test/postmaster/t/001_connection_limits.pl b/src/test/postmaster/t/001_connection_limits.pl
new file mode 100644
index 0000000000..f50aae4949
--- /dev/null
+++ b/src/test/postmaster/t/001_connection_limits.pl
@@ -0,0 +1,79 @@
+
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
+# Test connection limits, i.e. max_connections, reserved_connections
+# and superuser_reserved_connections.
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Initialize the server with specific low connection limits
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init;
+$node->append_conf('postgresql.conf', "max_connections = 6");
+$node->append_conf('postgresql.conf', "reserved_connections = 2");
+$node->append_conf('postgresql.conf', "superuser_reserved_connections = 1");
+$node->append_conf('postgresql.conf', "log_connections = on");
+$node->start;
+
+$node->safe_psql(
+ 'postgres', qq{
+CREATE USER regress_regular LOGIN;
+CREATE USER regress_reserved LOGIN;
+GRANT pg_use_reserved_connections TO regress_reserved;
+CREATE USER regress_superuser LOGIN SUPERUSER;
+});
+
+# With the limits we set in postgresql.conf, we can establish:
+# - 3 connections for any user with no special privileges
+# - 2 more connections for users belonging to "pg_use_reserved_connections"
+# - 1 more connection for superuser
+
+sub background_psql_as_user
+{
+ my $user = shift;
+
+ return $node->background_psql(
+ 'postgres',
+ on_error_die => 1,
+ extra_params => [ '-U', $user ]);
+}
+
+my @sessions = ();
+
+push(@sessions, background_psql_as_user('regress_regular'));
+push(@sessions, background_psql_as_user('regress_regular'));
+push(@sessions, background_psql_as_user('regress_regular'));
+$node->connect_fails(
+ "dbname=postgres user=regress_regular",
+ "reserved_connections limit",
+ expected_stderr =>
+ qr/FATAL: remaining connection slots are reserved for roles with privileges of the "pg_use_reserved_connections" role/
+);
+
+push(@sessions, background_psql_as_user('regress_reserved'));
+push(@sessions, background_psql_as_user('regress_reserved'));
+$node->connect_fails(
+ "dbname=postgres user=regress_regular",
+ "reserved_connections limit",
+ expected_stderr =>
+ qr/FATAL: remaining connection slots are reserved for roles with the SUPERUSER attribute/
+);
+
+push(@sessions, background_psql_as_user('regress_superuser'));
+$node->connect_fails(
+ "dbname=postgres user=regress_superuser",
+ "superuser_reserved_connections limit",
+ expected_stderr => qr/FATAL: sorry, too many clients already/);
+
+# TODO: test that query cancellation is still possible
+
+foreach my $session (@sessions)
+{
+ $session->quit;
+}
+
+done_testing();
--
2.39.5
From cd53192ea66a5b07c93941bd36e9f38bf8005956 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Fri, 4 Oct 2024 20:15:44 +0300
Subject: [PATCH v5 2/3] Add test for dead-end backends
The code path for launching a dead-end backend because we're out of
slots was not covered by any tests, so add one. (Some tests did hit
the case of launching a dead-end backend because the server is still
starting up, though, so the gap in our test coverage wasn't as big as
it sounds.)
Reviewed-by: Andres Freund <and...@anarazel.de>
Discussion: https://www.postgresql.org/message-id/a102f15f-eac4-4ff2-af02-f9ff209ec...@iki.fi
---
src/test/perl/PostgreSQL/Test/Cluster.pm | 38 +++++++++++++++++++
.../postmaster/t/001_connection_limits.pl | 37 +++++++++++++++++-
2 files changed, 74 insertions(+), 1 deletion(-)
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 90a842f96a..c278765fb0 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -104,6 +104,7 @@ use File::Path qw(rmtree mkpath);
use File::Spec;
use File::stat qw(stat);
use File::Temp ();
+use IO::Socket::INET;
use IPC::Run;
use PostgreSQL::Version;
use PostgreSQL::Test::RecursiveCopy;
@@ -286,6 +287,43 @@ sub connstr
=pod
+=item $node->raw_connect()
+
+Open a raw TCP or Unix domain socket connection to the server. This is
+used by low-level protocol and connection limit tests.
+
+=cut
+
+sub raw_connect
+{
+ my ($self) = @_;
+ my $pgport = $self->port;
+ my $pghost = $self->host;
+
+ my $socket;
+ if ($PostgreSQL::Test::Utils::use_unix_sockets)
+ {
+ require IO::Socket::UNIX;
+ my $path = "$pghost/.s.PGSQL.$pgport";
+
+ $socket = IO::Socket::UNIX->new(
+ Type => SOCK_STREAM(),
+ Peer => $path,
+ ) or die "Cannot create socket - $IO::Socket::errstr\n";
+ }
+ else
+ {
+ $socket = IO::Socket::INET->new(
+ PeerHost => $pghost,
+ PeerPort => $pgport,
+ Proto => 'tcp'
+ ) or die "Cannot create socket - $IO::Socket::errstr\n";
+ }
+ return $socket;
+}
+
+=pod
+
=item $node->group_access()
Does the data dir allow group access?
diff --git a/src/test/postmaster/t/001_connection_limits.pl b/src/test/postmaster/t/001_connection_limits.pl
index f50aae4949..158464fe03 100644
--- a/src/test/postmaster/t/001_connection_limits.pl
+++ b/src/test/postmaster/t/001_connection_limits.pl
@@ -43,6 +43,7 @@ sub background_psql_as_user
}
my @sessions = ();
+my @raw_connections = ();
push(@sessions, background_psql_as_user('regress_regular'));
push(@sessions, background_psql_as_user('regress_regular'));
@@ -69,11 +70,45 @@ $node->connect_fails(
"superuser_reserved_connections limit",
expected_stderr => qr/FATAL: sorry, too many clients already/);
-# TODO: test that query cancellation is still possible
+# We can still open TCP (or Unix domain socket) connections, but
+# beyond a certain number (roughly 2x max_connections), they will be
+# "dead-end backends".
+for (my $i = 0; $i <= 20; $i++)
+{
+ my $sock = $node->raw_connect();
+
+ # On a busy system, the server might reject connections if
+ # postmaster cannot accept() them fast enough. The exact limit and
+ # behavior depends on the platform. To make this reliable, we
+ # attempt SSL negotiation on each connection before opening next
+ # one. The server will reject the SSL negotations, but when it
+ # does so, we know that the backend has been launched and we
+ # should be able to open another connection.
+
+ # SSLRequest packet consists of packet length followed by
+ # NEGOTIATE_SSL_CODE.
+ my $negotiate_ssl_code = pack("Nnn", 8, 1234, 5679);
+ my $sent = $sock->send($negotiate_ssl_code);
+ # Read reply. We expect the server to reject it with 'N'
+ my $reply = "";
+ $sock->recv($reply, 1);
+ is($reply, "N", "dead-end connection $i");
+
+ push(@raw_connections, $sock);
+}
+
+# TODO: test that query cancellation is still possible. A dead-end
+# backend can process a query cancellation packet.
+
+# Clean up
foreach my $session (@sessions)
{
$session->quit;
}
+foreach my $socket (@raw_connections)
+{
+ $socket->close();
+}
done_testing();
--
2.39.5
