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