PGSERVICEFILE as part of a normal connection string

On Mon, 2024-11-18 at 21:21 +0100, Torsten Förtsch wrote:

I like to bundle all my database connections in a .pg_service.conf. Over time I
collected a bunch of such service files. A while back I discovered that the
service file can only be specified as an environment variable. It cannot be
given as part of the connection string like

psql "service=$MY_SERVICE servicefile=MY_SERVICE_FILE"

The attached patch allows that.

+1 for the idea (I didn't test the patch).

Yours,
Laurenz Albe

On Mon, Nov 18, 2024 at 09:21:56PM +0100, Torsten Förtsch wrote:

I like to bundle all my database connections in a .pg_service.conf. Over
time I collected a bunch of such service files. A while back I discovered
that the service file can only be specified as an environment variable. It
cannot be given as part of the connection string like

-    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));

That should be right, the connection parameter takes priority over the
environment variable. The comment at the top of this code block
becomes incorrect.

else
{
@@ -5678,6 +5684,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;
+                }

Interesting. We've never had tests for that even for "service".
Perhaps it would be the time to add some tests for the existing case
and the one you are adding? Your test suite should make that easy to
add.

+# This tests "service" and "servicefile"

You are introducing tests for the existing "service", as well as tests
for the new "servicefile". Could it be possible to split that into
two patches for clarity? You'd want one to provide coverage for the
existing features (PGSERVICEFILE, PGSERVICE and connection parameter
"service"), then add tests for the new feature "servicename" with its
libpq implementation. That would make your main patch simpler, as
well.

+open my $fh, '>', $srvfile or die $!;
+print $fh "[my_srv]\n";
+print $fh +($node->connstr =~ s/ /\n/gr), "\n";
+close $fh;

Sure that's OK on Windows where we have CRLFs, not just LFs?
--
Michael

Interesting. We've never had tests for that even for "service".
Perhaps it would be the time to add some tests for the existing case
and the one you are adding? Your test suite should make that easy to
add.

Currently, a lot of our utility scripts (anything that uses
connectDatabase) don't support service=name params or PGSERVICE=name env
vars, which is really too bad. I previously thought that this was because
of a lack of interest, but perhaps people do want it?

On Wed, Nov 20, 2024 at 02:58:43AM -0500, Corey Huinker wrote:

Currently, a lot of our utility scripts (anything that uses
connectDatabase) don't support service=name params or PGSERVICE=name env
vars, which is really too bad. I previously thought that this was because
of a lack of interest, but perhaps people do want it?

I'm all for more test coverage, FWIW.

Torsten, the patch has been waiting on input from you based on my
latest review for some time, so I have marked it as returned with
feedback in the CP app. Feel free to resubmit a new version if you
are planning to work on that.

Thanks.
--
Michael

On Mon, Jan 27, 2025 at 2:01 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Nov 20, 2024 at 02:58:43AM -0500, Corey Huinker wrote:

Currently, a lot of our utility scripts (anything that uses
connectDatabase) don't support service=name params or PGSERVICE=name env
vars, which is really too bad. I previously thought that this was because
of a lack of interest, but perhaps people do want it?

I'm all for more test coverage, FWIW.

Torsten, the patch has been waiting on input from you based on my
latest review for some time, so I have marked it as returned with
feedback in the CP app. Feel free to resubmit a new version if you
are planning to work on that.

TO: Torsten,
CC: Micael and other hackers

If you can't work for ther patch for a while because you are busy or
other some reason,
I can become additinal reviewer and apply review comments from Micael
to the patch instead of you.

If you don't want my action, please reply and notice me that. If
possible, within a week :)

Just to let you know, my action is not intended to steal your
contribution but to prevent your good idea from being lost.

TO: Mecael and other hackers,

There are any problem in light of community customs?

---
Great regards,
Ryo Kanbayashi

On Thu, Mar 13, 2025 at 08:53:49AM +0900, Ryo Kanbayashi wrote:

If you can't work for ther patch for a while because you are busy or
other some reason,
I can become additinal reviewer and apply review comments from Micael
to the patch instead of you.

If you don't want my action, please reply and notice me that. If
possible, within a week :)

Putting a bit of context here. Most of the Postgres hackers based in
Japan had a meeting last Friday, and Kanbayashi-san has asked me about
patches that introduce to simpler code paths in the tree that could be
worked on for this release. I've mentioned this thread to him.

Just to let you know, my action is not intended to steal your
contribution but to prevent your good idea from being lost.

Authors and reviewers get busy because of life and work matters, and
contributions are listed in the commit logs for everybody who
participates. If you can help move this patch forward, thanks a lot
for the help! IMO, that would be great. The patch set still needs
more reorganization and adjustments, but I think that we can get it
there.
--
Michael

On Thu, 2025-03-13 at 08:53 +0900, Ryo Kanbayashi wrote:

Just to let you know, my action is not intended to steal your
contribution but to prevent your good idea from being lost.

TO: Mecael and other hackers,

There are any problem in light of community customs?

Anything submitted to the mailing list is no longer private
intellectual property. You are free and welcome to start working
on any patch that you are interested in and that seems neglected
by the author. There is no problem with listing more than one
author.

Yours,
Laurenz Albe

On Thu, Mar 13, 2025 at 9:42 AM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Mar 13, 2025 at 08:53:49AM +0900, Ryo Kanbayashi wrote:

If you can't work for ther patch for a while because you are busy or
other some reason,
I can become additinal reviewer and apply review comments from Micael
to the patch instead of you.

If you don't want my action, please reply and notice me that. If
possible, within a week :)

Putting a bit of context here. Most of the Postgres hackers based in
Japan had a meeting last Friday, and Kanbayashi-san has asked me about
patches that introduce to simpler code paths in the tree that could be
worked on for this release. I've mentioned this thread to him.

Just to let you know, my action is not intended to steal your
contribution but to prevent your good idea from being lost.

Authors and reviewers get busy because of life and work matters, and
contributions are listed in the commit logs for everybody who
participates. If you can help move this patch forward, thanks a lot
for the help! IMO, that would be great. The patch set still needs
more reorganization and adjustments, but I think that we can get it
there.

On Thu, Mar 13, 2025 at 3:07 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:

On Thu, 2025-03-13 at 08:53 +0900, Ryo Kanbayashi wrote:

Just to let you know, my action is not intended to steal your
contribution but to prevent your good idea from being lost.

TO: Mecael and other hackers,

There are any problem in light of community customs?

Anything submitted to the mailing list is no longer private
intellectual property. You are free and welcome to start working
on any patch that you are interested in and that seems neglected
by the author. There is no problem with listing more than one
author.

Michael and Laurenz,

Thank you for context description and comments to my action :)

I start coding to complete the patch :)

---
Great regards,
Ryo Kanbayashi

On Mon, Jan 27, 2025 at 2:01 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Mar 13, 2025 at 08:53:49AM +0900, Ryo Kanbayashi wrote:

Putting a bit of context here. Most of the Postgres hackers based in
Japan had a meeting last Friday, and Kanbayashi-san has asked me about
patches that introduce to simpler code paths in the tree that could be
worked on for this release. I've mentioned this thread to him.

Just to let you know, my action is not intended to steal your
contribution but to prevent your good idea from being lost.

Authors and reviewers get busy because of life and work matters, and
contributions are listed in the commit logs for everybody who
participates. If you can help move this patch forward, thanks a lot
for the help! IMO, that would be great. The patch set still needs
more reorganization and adjustments, but I think that we can get it
there

Michael,
CC: Torsten

I reviewed the patch and add some modification described below.

part of /messages/by-id/Zz2AE7NKKLIZTtEh@paquier.xyz

+# This tests "service" and "servicefile"

You are introducing tests for the existing "service", as well as tests
for the new "servicefile". Could it be possible to split that into
two patches for clarity? You'd want one to provide coverage for the
existing features (PGSERVICEFILE, PGSERVICE and connection parameter
"service"), then add tests for the new feature "servicename" with its
libpq implementation. That would make your main patch simpler, as
well.

+open my $fh, '>', $srvfile or die $!;
+print $fh "[my_srv]\n";
+print $fh +($node->connstr =~ s/ /\n/gr), "\n";
+close $fh;

Sure that's OK on Windows where we have CRLFs, not just LFs?

I did...
* Split the patch to two patches
1) regression test of existing features.
2) adding servicefile option feature, its regression test and etc
* Add codes which care new line code of Windows
* Add comments and apply formatter :)

---
Great Regards,
Ryo Kanbayashi

On Thu, Mar 20, 2025 at 5:39 PM Ryo Kanbayashi <kanbayashi.dev@gmail.com> wrote:

On Mon, Jan 27, 2025 at 2:01 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Mar 13, 2025 at 08:53:49AM +0900, Ryo Kanbayashi wrote:

Putting a bit of context here. Most of the Postgres hackers based in
Japan had a meeting last Friday, and Kanbayashi-san has asked me about
patches that introduce to simpler code paths in the tree that could be
worked on for this release. I've mentioned this thread to him.

Just to let you know, my action is not intended to steal your
contribution but to prevent your good idea from being lost.

Authors and reviewers get busy because of life and work matters, and
contributions are listed in the commit logs for everybody who
participates. If you can help move this patch forward, thanks a lot
for the help! IMO, that would be great. The patch set still needs
more reorganization and adjustments, but I think that we can get it
there

Michael,
CC: Torsten

I reviewed the patch and add some modification described below.

part of /messages/by-id/Zz2AE7NKKLIZTtEh@paquier.xyz

+# This tests "service" and "servicefile"

You are introducing tests for the existing "service", as well as tests
for the new "servicefile". Could it be possible to split that into
two patches for clarity? You'd want one to provide coverage for the
existing features (PGSERVICEFILE, PGSERVICE and connection parameter
"service"), then add tests for the new feature "servicename" with its
libpq implementation. That would make your main patch simpler, as
well.

+open my $fh, '>', $srvfile or die $!;
+print $fh "[my_srv]\n";
+print $fh +($node->connstr =~ s/ /\n/gr), "\n";
+close $fh;

Sure that's OK on Windows where we have CRLFs, not just LFs?

I did...
* Split the patch to two patches
1) regression test of existing features.
2) adding servicefile option feature, its regression test and etc
* Add codes which care new line code of Windows
* Add comments and apply formatter :)

Sorry, I found a miss on 006_service.pl.
Fixed patch is attached...

---
Great Regards,
Ryo Kanbayashi

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.

+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?

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?

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.

+# Copyright (c) 2023-2024, PostgreSQL Global Development Group

These dates are incorrect. Should be 2025, as it's a new file.

+++ 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.
--
Michael

On Sat, Mar 22, 2025 at 4:46 PM Michael Paquier <michael@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

On Sun, Mar 23, 2025 at 12:32:03PM +0900, Ryo Kanbayashi wrote:

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

I have spent quite a bit of time on the review 0001 with the new
tests to get something in for this release, and there was quite a bit
going on there:
- The script should set PGSYSCONFDIR, or it could grab data that
depend on the host. This can use the temporary folder created in the
test.
- On the same ground, we need a similar tweak for PGSERVICEFILE or we
would go into pqGetHomeDirectory() and look at a HOME folder (WIN32
and non-WIN32).

With that addressed, there could be much more tests, like for cases
where PGSERVICEFILE is set but points to a file that does not exist,
more combinations between URIs, connection parameters and PGSERVICE,
for success and failure cases, empty service file, etc.

Another thing that I've noticed to be useful to cover is the case
based on the hardcoded service file name pg_service.conf in
PGSYSCONFDIR, which is used as a fallback in the code if the service
name cannot be found in the initial PGSERVICEFILE, acting as a
fallback option. As long as PGSYSCONFDIR is set, we could test one in
isolation using the temporary folder created by the test.

With all that in mind and more documentation added to the test, I've
applied 0001, so let's see what the buildfarm has to say. The CI was
stable, so it's a start.

I am not sure that I'll have the time to look at 0002 for this release
cycle, could it be possible to get a rebase for it?
--
Michael

On Thu, Mar 27, 2025 at 06:31:14PM +0900, Michael Paquier wrote:

I am not sure that I'll have the time to look at 0002 for this release
cycle, could it be possible to get a rebase for it?

Here is a simple rebase that I have been able to assemble this
morning. I won't have the space to review it for this release cycle
unfortunately, but at least it works in the CI.

I am moving this patch entry to the next CF for v19, as a result of
that.
--
Michael

On Thu, Mar 27, 2025 at 06:31:14PM +0900, Michael Paquier wrote:

With all that in mind and more documentation added to the test, I've
applied 0001, so let's see what the buildfarm has to say. The CI was
stable, so it's a start.

The buildfarm (particularly the Windows members that worried me), have
reported back and I am not seeing any failures, so we should be good
with 72c2f36d5727.
--
Michael

On Fri, Mar 28, 2025 at 10:44 AM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Mar 27, 2025 at 06:31:14PM +0900, Michael Paquier wrote:

With all that in mind and more documentation added to the test, I've
applied 0001, so let's see what the buildfarm has to say. The CI was
stable, so it's a start.

The buildfarm (particularly the Windows members that worried me), have
reported back and I am not seeing any failures, so we should be good
with 72c2f36d5727.

Thank you for review and additional modification to the patch.
I'm glad the patch was made in time for this release, even if it was
just a partial one.

On Fri, Mar 28, 2025 at 8:57 AM Michael Paquier <michael@paquier.xyz> wrote:

I am not sure that I'll have the time to look at 0002 for this release
cycle, could it be possible to get a rebase for it?

Here is a simple rebase that I have been able to assemble this
morning. I won't have the space to review it for this release cycle
unfortunately, but at least it works in the CI.

I'm sorry I couldn't respond to your request :(

I am moving this patch entry to the next CF for v19, as a result of
that.

OK
Thanks :)

On Thu, Mar 27, 2025 at 6:31 PM Michael Paquier <michael@paquier.xyz> wrote:

I have spent quite a bit of time on the review 0001 with the new
tests to get something in for this release, and there was quite a bit
going on there:
- The script should set PGSYSCONFDIR, or it could grab data that
depend on the host. This can use the temporary folder created in the
test.
- On the same ground, we need a similar tweak for PGSERVICEFILE or we
would go into pqGetHomeDirectory() and look at a HOME folder (WIN32
and non-WIN32).

With that addressed, there could be much more tests, like for cases
where PGSERVICEFILE is set but points to a file that does not exist,
more combinations between URIs, connection parameters and PGSERVICE,
for success and failure cases, empty service file, etc.

Another thing that I've noticed to be useful to cover is the case
based on the hardcoded service file name pg_service.conf in
PGSYSCONFDIR, which is used as a fallback in the code if the service
name cannot be found in the initial PGSERVICEFILE, acting as a
fallback option. As long as PGSYSCONFDIR is set, we could test one in
isolation using the temporary folder created by the test.

I check and modify 0002 patch (adding servicefile option and its
regression tests)
in light of the above and committed 0001 patch (regression test of
existing features)
toward next release :)

---
Great Regards,
Ryo Kanbayashi

On Sat, Mar 29, 2025 at 3:35 PM Ryo Kanbayashi <kanbayashi.dev@gmail.com> wrote:

On Fri, Mar 28, 2025 at 8:57 AM Michael Paquier <michael@paquier.xyz> wrote:

I am not sure that I'll have the time to look at 0002 for this release
cycle, could it be possible to get a rebase for it?

Here is a simple rebase that I have been able to assemble this
morning. I won't have the space to review it for this release cycle
unfortunately, but at least it works in the CI.

I'm sorry I couldn't respond to your request :(

I am moving this patch entry to the next CF for v19, as a result of
that.

OK
Thanks :)

On Thu, Mar 27, 2025 at 6:31 PM Michael Paquier <michael@paquier.xyz> wrote:

I have spent quite a bit of time on the review 0001 with the new
tests to get something in for this release, and there was quite a bit
going on there:
- The script should set PGSYSCONFDIR, or it could grab data that
depend on the host. This can use the temporary folder created in the
test.
- On the same ground, we need a similar tweak for PGSERVICEFILE or we
would go into pqGetHomeDirectory() and look at a HOME folder (WIN32
and non-WIN32).

With that addressed, there could be much more tests, like for cases
where PGSERVICEFILE is set but points to a file that does not exist,
more combinations between URIs, connection parameters and PGSERVICE,
for success and failure cases, empty service file, etc.

Another thing that I've noticed to be useful to cover is the case
based on the hardcoded service file name pg_service.conf in
PGSYSCONFDIR, which is used as a fallback in the code if the service
name cannot be found in the initial PGSERVICEFILE, acting as a
fallback option. As long as PGSYSCONFDIR is set, we could test one in
isolation using the temporary folder created by the test.

I check and modify 0002 patch (adding servicefile option and its
regression tests)
in light of the above and committed 0001 patch (regression test of
existing features)
toward next release :)

Although it probably won't be ready in time for this release, I've
created new 0001 patch (former 0002) which is reflected your review
comments.

I checked That the patch passes CI of my GitHub repository.

Best of luck :)

---
Great regards,
Ryo Kanbayashi

Hi,

I am working on a feature adjacent to the connection service functionality
and noticed some issues with the tests introduced in this thread. Basically
they incorrectly invoke the append perl function by passing multiple
strings to append when the function only takes one string to append. This
caused the generated service files to not actually contain any connection
parameters. The tests were only passing because the connect_ok perl
function set the connection parameters as environment variables which
covered up the misformed connection service file.

The attached patch is much more strict in that it creates a dummy database
that is not started and passes all queries though that and tests that the
connection service file correctly overrides the environment variables set
by the dummy databases' query functions

Thanks,
Andrew Jackson

On Mon, Mar 31, 2025, 4:01 PM Ryo Kanbayashi <kanbayashi.dev@gmail.com>
wrote:

On Tue, Apr 1, 2025 at 6:26 AM Andrew Jackson
<andrewjackson947@gmail.com> wrote:

Hi,

I am working on a feature adjacent to the connection service functionality and noticed some issues with the tests introduced in this thread. Basically they incorrectly invoke the append perl function by passing multiple strings to append when the function only takes one string to append. This caused the generated service files to not actually contain any connection parameters. The tests were only passing because the connect_ok perl function set the connection parameters as environment variables which covered up the misformed connection service file.
The attached patch is much more strict in that it creates a dummy database that is not started and passes all queries though that and tests that the connection service file correctly overrides the environment variables set by the dummy databases' query functions

Andrew,
CC: Michael, Torsten

Thank you to find issues the tests.

I confirmed points you noticed and validity of your proposed
modifications with local execution and internal impl of connect_ok
func.

- Current usage of append_to_file func is wrong and not appropriate
service file is generated
- connect_ok perl func set the connection parameters as environment
variables which covered up the misformed connection service file
- https://github.com/postgres/postgres/blob/ea3f9b6da34a1a4dc2c0c118789587c2a85c78d7/src/test/perl/PostgreSQL/Test/Cluster.pm#L2576
- https://github.com/postgres/postgres/blob/ea3f9b6da34a1a4dc2c0c118789587c2a85c78d7/src/test/perl/PostgreSQL/Test/Cluster.pm#L2120
- https://github.com/postgres/postgres/blob/ea3f9b6da34a1a4dc2c0c118789587c2a85c78d7/src/test/perl/PostgreSQL/Test/Cluster.pm#L1718
- Your dummy node object introduced code works without problem and the
code is more strict than current code

I'll reflect your notice and suggestion to the patch current I'm working on :)

---
Great Regards,
Ryo Kanbayashi