Thread: Re: PGSERVICEFILE as part of a normal connection string

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 https://www.postgresql.org/message-id/Zz2AE7NKKLIZTtEh%40paquier.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 https://www.postgresql.org/message-id/Zz2AE7NKKLIZTtEh%40paquier.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

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
introducedin this thread. Basically they incorrectly invoke the append perl function by passing multiple strings to
appendwhen the function only takes one string to append. This caused the generated service files to not actually
containany connection parameters. The tests were only passing because the connect_ok perl function set the connection
parametersas 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
thoughthat 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