On Sat, Mar 22, 2025 at 4:46 PM Michael Paquier <mich...@paquier.xyz> wrote:
>
> On Thu, Mar 20, 2025 at 06:16:44PM +0900, Ryo Kanbayashi wrote:
> > Sorry, I found a miss on 006_service.pl.
> > Fixed patch is attached...
>
> Please note that the commit fest app needs all the patches of a a set
> to be posted in the same message.  In this case, v2-0001 is not going
> to get automatic test coverage.
>
> Your patch naming policy is also a bit confusing.  I would suggest to
> use `git format-patch -vN -2`, where N is your version number.  0001
> would be the new tests for service files, and 0002 the new feature,
> with its own tests.

All right.
I attached patches generated with your suggested command :)

> +if ($windows_os) {
> +
> +    # Windows: use CRLF
> +    print $fh "[my_srv]",                                   "\r\n";
> +    print $fh join( "\r\n", split( ' ', $node->connstr ) ), "\r\n";
> +}
> +else {
> +    # Non-Windows: use LF
> +    print $fh "[my_srv]",                                 "\n";
> +    print $fh join( "\n", split( ' ', $node->connstr ) ), "\n";
> +}
> +close $fh;
>
> That's duplicated.  Let's perhaps use a $newline variable and print
> into the file using the $newline?

OK.
I reflected above comment.

> Question: you are doing things this way in the test because fgets() is
> what is used by libpq to retrieve the lines of the service file, is
> that right?

No. I'm doing above way simply because line ending code of service file
wrote by users may become CRLF in Windows platform.

> Please note that the CI is failing.  It seems to me that you are
> missing a done_testing() at the end of the script.  If you have a
> github account, I'd suggest to set up a CI in your own fork of
> Postgres, this is really helpful to double-check the correctness of a
> patch before posting it to the lists, and saves in round trips between
> author and reviewer.  Please see src/tools/ci/README in the code tree
> for details.

Sorry.
I'm using Cirrus CI with GitHub and I checked passing the CI.
But there were misses when I created patch files...

> +# Copyright (c) 2023-2024, PostgreSQL Global Development Group
>
> These dates are incorrect.  Should be 2025, as it's a new file.

OK.

> +++ b/src/interfaces/libpq/t/007_servicefile_opt.pl
> @@ -0,0 +1,100 @@
> +# Copyright (c) 2023-2024, PostgreSQL Global Development Group
>
> Incorrect date again in the second path with the new feature.  I'd
> suggest to merge all the tests in a single script, with only one node
> initialized and started.

OK.
Additional test scripts have been merged to a single script ^^ b

---
Great regards,
Ryo Kanbayashi
From 69c4f4eb8e0ed40c434867fbb740a68383623da9 Mon Sep 17 00:00:00 2001
From: Ryo Kanbayashi <ryo.contact@gmail.com>
Date: Sun, 23 Mar 2025 11:41:06 +0900
Subject: [PATCH v3 1/2] add regression test of service connection option.

---
 src/interfaces/libpq/meson.build      |  1 +
 src/interfaces/libpq/t/006_service.pl | 79 +++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)
 create mode 100644 src/interfaces/libpq/t/006_service.pl

diff --git a/src/interfaces/libpq/meson.build b/src/interfaces/libpq/meson.build
index 19f4a52a97a..292fecf3320 100644
--- a/src/interfaces/libpq/meson.build
+++ b/src/interfaces/libpq/meson.build
@@ -122,6 +122,7 @@ tests += {
       't/003_load_balance_host_list.pl',
       't/004_load_balance_dns.pl',
       't/005_negotiate_encryption.pl',
+      't/006_service.pl',
     ],
     'env': {
       'with_ssl': ssl_library,
diff --git a/src/interfaces/libpq/t/006_service.pl b/src/interfaces/libpq/t/006_service.pl
new file mode 100644
index 00000000000..045e25a05d3
--- /dev/null
+++ b/src/interfaces/libpq/t/006_service.pl
@@ -0,0 +1,79 @@
+# Copyright (c) 2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use Config;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+# This tests "service" connection options.
+
+# Cluster setup which is shared for testing both load balancing methods
+my $node = PostgreSQL::Test::Cluster->new('node');
+
+# Create a data directory with initdb
+$node->init();
+
+# Start the PostgreSQL server
+$node->start();
+
+my $td      = PostgreSQL::Test::Utils::tempdir;
+my $srvfile = "$td/pgsrv.conf";
+
+# Windows: use CRLF
+# Non-Windows: use LF
+my $newline = $windows_os ? "\r\n" : "\n";
+
+# Create a service file
+open my $fh, '>', $srvfile or die $!;
+print $fh "[my_srv]",                                     $newline;
+print $fh join( $newline, split( ' ', $node->connstr ) ), $newline;
+
+close $fh;
+
+# Check that service option works as expected
+{
+    local $ENV{PGSERVICEFILE} = $srvfile;
+    $node->connect_ok(
+        'service=my_srv',
+        'service=my_srv',
+        sql             => "SELECT 'connect1'",
+        expected_stdout => qr/connect1/
+    );
+
+    $node->connect_ok(
+        'postgres://?service=my_srv',
+        'postgres://?service=my_srv',
+        sql             => "SELECT 'connect2'",
+        expected_stdout => qr/connect2/
+    );
+
+    local $ENV{PGSERVICE} = 'my_srv';
+    $node->connect_ok(
+        '',
+        'envvar: PGSERVICE=my_srv',
+        sql             => "SELECT 'connect3'",
+        expected_stdout => qr/connect3/
+    );
+}
+
+# Check that not existing service fails
+{
+    local $ENV{PGSERVICEFILE} = $srvfile;
+    local $ENV{PGSERVICE} = 'non-existent-service';
+    $node->connect_fails(
+        '',
+        'envvar: PGSERVICE=non-existent-service',
+        expected_stdout =>
+          qr/definition of service "non-existent-service" not found/
+    );
+
+    $node->connect_fails(
+        'service=non-existent-service',
+        'service=non-existent-service',
+        expected_stderr =>
+          qr/definition of service "non-existent-service" not found/
+    );
+}
+
+done_testing();
\ No newline at end of file
-- 
2.25.1

From df4ce542960afdba9fd4c619c4e40671ed4c09fd Mon Sep 17 00:00:00 2001
From: Ryo Kanbayashi <ryo.contact@gmail.com>
Date: Sun, 23 Mar 2025 11:44:15 +0900
Subject: [PATCH v3 2/2] add servicefile connection option feature

---
 doc/src/sgml/libpq.sgml               | 16 ++++++-
 src/interfaces/libpq/fe-connect.c     | 27 +++++++++--
 src/interfaces/libpq/t/006_service.pl | 65 ++++++++++++++++++++++++++-
 3 files changed, 103 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 8fa0515c6a0..1050db5c983 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2248,6 +2248,19 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
       </listitem>
      </varlistentry>
 
+     <varlistentry id="libpq-connect-servicefile" xreflabel="servicefile">
+      <term><literal>servicefile</literal></term>
+      <listitem>
+       <para>
+        This option specifies the name of the per-user connection service file
+        (see <xref linkend="libpq-pgservice"/>).
+        Defaults to <filename>~/.pg_service.conf</filename>, or
+        <filename>%APPDATA%\postgresql\.pg_service.conf</filename> on
+        Microsoft Windows.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="libpq-connect-target-session-attrs" xreflabel="target_session_attrs">
       <term><literal>target_session_attrs</literal></term>
       <listitem>
@@ -9504,7 +9517,8 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
    On Microsoft Windows, it is named
    <filename>%APPDATA%\postgresql\.pg_service.conf</filename> (where
    <filename>%APPDATA%</filename> refers to the Application Data subdirectory
-   in the user's profile).  A different file name can be specified by
+   in the user's profile).  A different file name can be specified using the
+   <literal>servicefile</literal> key word in a libpq connection string or by
    setting the environment variable <envar>PGSERVICEFILE</envar>.
    The system-wide file is named <filename>pg_service.conf</filename>.
    By default it is sought in the <filename>etc</filename> directory
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d5051f5e820..f16468be7d1 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -195,6 +195,9 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 		"Database-Service", "", 20,
 	offsetof(struct pg_conn, pgservice)},
 
+	{"servicefile", "PGSERVICEFILE", NULL, NULL,
+	"Database-Service-File", "", 64, -1},
+
 	{"user", "PGUSER", NULL, NULL,
 		"Database-User", "", 20,
 	offsetof(struct pg_conn, pguser)},
@@ -5838,6 +5841,7 @@ static int
 parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage)
 {
 	const char *service = conninfo_getval(options, "service");
+	const char *service_fname = conninfo_getval(options, "servicefile");
 	char		serviceFile[MAXPGPATH];
 	char	   *env;
 	bool		group_found = false;
@@ -5857,11 +5861,18 @@ parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage)
 		return 0;
 
 	/*
-	 * Try PGSERVICEFILE if specified, else try ~/.pg_service.conf (if that
-	 * exists).
+	 * First, check servicefile option on connection string. Second, check
+	 * PGSERVICEFILE environment variable. Finally, check ~/.pg_service.conf
+	 * (if that exists).
 	 */
-	if ((env = getenv("PGSERVICEFILE")) != NULL)
+	if (service_fname != NULL)
+	{
+		strlcpy(serviceFile, service_fname, sizeof(serviceFile));
+	}
+	else if ((env = getenv("PGSERVICEFILE")) != NULL)
+	{
 		strlcpy(serviceFile, env, sizeof(serviceFile));
+	}
 	else
 	{
 		char		homedir[MAXPGPATH];
@@ -6023,6 +6034,16 @@ parseServiceFile(const char *serviceFile,
 					goto exit;
 				}
 
+				if (strcmp(key, "servicefile") == 0)
+				{
+					libpq_append_error(errorMessage,
+									   "nested servicefile specifications not supported in service file \"%s\", line %d",
+									   serviceFile,
+									   linenr);
+					result = 3;
+					goto exit;
+				}
+
 				/*
 				 * Set the parameter --- but don't override any previous
 				 * explicit setting.
diff --git a/src/interfaces/libpq/t/006_service.pl b/src/interfaces/libpq/t/006_service.pl
index 045e25a05d3..ff0493954dd 100644
--- a/src/interfaces/libpq/t/006_service.pl
+++ b/src/interfaces/libpq/t/006_service.pl
@@ -6,7 +6,7 @@ use PostgreSQL::Test::Utils;
 use PostgreSQL::Test::Cluster;
 use Test::More;
 
-# This tests "service" connection options.
+# This tests "service" and "servicefile" connection options.
 
 # Cluster setup which is shared for testing both load balancing methods
 my $node = PostgreSQL::Test::Cluster->new('node');
@@ -76,4 +76,67 @@ close $fh;
     );
 }
 
+# Backslashes escaped path string for getting collect result at concatenation
+# for Windows environment
+my $srvfile_win_cared = $srvfile;
+$srvfile_win_cared =~ s/\\/\\\\/g;
+
+# Check that servicefile option works as expected
+{
+    $node->connect_ok(
+        q{service=my_srv servicefile='} . $srvfile_win_cared . q{'},
+        'service=my_srv servicefile=...',
+        sql             => "SELECT 'connect4'",
+        expected_stdout => qr/connect4/
+    );
+
+    # Encode slashes and backslash
+    my $encoded_srvfile = $srvfile =~ s{([\\/])}{
+        $1 eq '/' ? '%2F' : '%5C'
+    }ger;
+
+    # Additionaly encode a colon in servicefile path of Windows
+    $encoded_srvfile =~ s/:/%3A/g;
+
+    $node->connect_ok(
+        'postgresql:///?service=my_srv&servicefile=' . $encoded_srvfile,
+        'postgresql:///?service=my_srv&servicefile=...',
+        sql             => "SELECT 'connect5'",
+        expected_stdout => qr/connect5/
+    );
+
+    local $ENV{PGSERVICE} = 'my_srv';
+    $node->connect_ok(
+        q{servicefile='} . $srvfile_win_cared . q{'},
+        'envvar: PGSERVICE=my_srv + servicefile=...',
+        sql             => "SELECT 'connect6'",
+        expected_stdout => qr/connect6/
+    );
+
+    $node->connect_ok(
+        'postgresql://?servicefile=' . $encoded_srvfile,
+        'envvar: PGSERVICE=my_srv + postgresql://?servicefile=...',
+        sql             => "SELECT 'connect7'",
+        expected_stdout => qr/connect7/
+    );
+}
+
+# Check that servicefile option takes precedence over PGSERVICEFILE environment variable
+{
+    local $ENV{PGSERVICEFILE} = 'non-existent-file.conf';
+
+    $node->connect_fails(
+        'service=my_srv',
+        'service=... fails with wrong PGSERVICEFILE',
+        expected_stderr => qr/service file "non-existent-file\.conf" not found/
+    );
+
+    $node->connect_ok(
+        q{service=my_srv servicefile='} . $srvfile_win_cared . q{'},
+        'servicefile= takes precedence over PGSERVICEFILE',
+        sql             => "SELECT 'connect8'",
+        expected_stdout => qr/connect8/
+    );
+}
+
 done_testing();
\ No newline at end of file
-- 
2.25.1

Reply via email to