Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. psql: Add variable SERVICEFILE

  2. libpq: Add "servicefile" connection option

  3. libpq: Add TAP test for nested service file

  4. libpq: Fix some issues in TAP tests for service files

  5. docs: Fix column count attribute in table

  6. libpq: Add TAP tests for service files and names

  1. PGSERVICEFILE as part of a normal connection string

    Torsten Förtsch <tfoertsch123@gmail.com> — 2024-11-18T20:21:56Z

    Hi,

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.

Regards.
--
Torsten

  2. Re: PGSERVICEFILE as part of a normal connection string

    Laurenz Albe <laurenz.albe@cybertec.at> — 2024-11-18T20:46:40Z

    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

  3. Re: PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2024-11-20T06:22:11Z

    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

  4. Re: PGSERVICEFILE as part of a normal connection string

    Corey Huinker <corey.huinker@gmail.com> — 2024-11-20T07:58:43Z

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

  5. Re: PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-01-27T05:00:54Z

    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

  6. Re: PGSERVICEFILE as part of a normal connection string

    Ryo Kanbayashi <kanbayashi.dev@gmail.com> — 2025-03-12T23:53:49Z

    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

  7. Re: PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-03-13T00:42:17Z

    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

  8. Re: PGSERVICEFILE as part of a normal connection string

    Laurenz Albe <laurenz.albe@cybertec.at> — 2025-03-13T06:07:39Z

    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

  9. Re: PGSERVICEFILE as part of a normal connection string

    Ryo Kanbayashi <kanbayashi.dev@gmail.com> — 2025-03-13T12:08:28Z

    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

  10. Re: PGSERVICEFILE as part of a normal connection string

    Ryo Kanbayashi <kanbayashi.dev@gmail.com> — 2025-03-20T08:39:59Z

    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

  11. [PATCH] PGSERVICEFILE as part of a normal connection string

    Ryo Kanbayashi <kanbayashi.dev@gmail.com> — 2025-03-20T09:16:44Z

    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

  12. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-03-22T07:46:23Z

    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

  13. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Ryo Kanbayashi <kanbayashi.dev@gmail.com> — 2025-03-23T03:32:03Z

    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

  14. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-03-27T09:31:14Z

    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

  15. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-03-27T23:57:34Z

    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

  16. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-03-28T01:44:04Z

    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

  17. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Ryo Kanbayashi <kanbayashi.dev@gmail.com> — 2025-03-29T06:35:11Z

    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

  18. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Ryo Kanbayashi <kanbayashi.dev@gmail.com> — 2025-03-29T12:19:16Z

    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

  19. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Andrew Jackson <andrewjackson947@gmail.com> — 2025-03-31T21:26:27Z

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

  20. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Ryo Kanbayashi <kanbayashi.dev@gmail.com> — 2025-04-02T15:36:59Z

    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

  21. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-04-07T04:04:12Z

    On Mon, Mar 31, 2025 at 04:26:27PM -0500, Andrew Jackson wrote:
> 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.

Yep, you are right on this one.  I didn't really like the hardcoding
of the host and port parts, and we should still be OK to rely on a
connstr from the valid node split on spaces.  At least that's a bit
simpler.

> 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

Interesting trick, I like that.  The point of not starting the node is
important, while we also make sure to load an environment related to
the node where the valid connection should happen.  Breaking the
contents of the valid service file on purpose breaks the connection
attempts, making the tests fail.

So applied as you have proposed, mostly, and I have added more
documentation explaining the idea behind the dummy node.
--
Michael

  22. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-04-07T04:10:08Z

    On Thu, Apr 03, 2025 at 12:36:59AM +0900, Ryo Kanbayashi wrote:
> I'll reflect your notice and suggestion to the patch current I'm
> working on :)

Thanks for that.

And I have forgotten to add you as a reviewer of what has been
committed as 2c7bd2ba507e.  Sorry for that :/
--
Michael

  23. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Ryo Kanbayashi <kanbayashi.dev@gmail.com> — 2025-04-13T10:06:06Z

    On Mon, Apr 7, 2025 at 1:10 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Apr 03, 2025 at 12:36:59AM +0900, Ryo Kanbayashi wrote:
> > I'll reflect your notice and suggestion to the patch current I'm
> > working on :)
>
> Thanks for that.
>
> And I have forgotten to add you as a reviewer of what has been
> committed as 2c7bd2ba507e.  Sorry for that :/

No problem :)

I rebased our patch according to  2c7bd2ba507e.
https://commitfest.postgresql.org/patch/5387/

---
Great regards,
Ryo Kanbayashi

  24. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-05-28T06:48:14Z

    On Sun, Apr 13, 2025 at 07:06:06PM +0900, Ryo Kanbayashi wrote:
> I rebased our patch according to  2c7bd2ba507e.
> https://commitfest.postgresql.org/patch/5387/

Thanks for the new version.

-# for the connection options and their environment variables.
+# for the connection options, servicefile options and their environment variables.

It seems to me that this comment does not need to be changed.

+	{"servicefile", "PGSERVICEFILE", NULL, NULL,
+	"Database-Service-File", "", 64, -1},

Could it be better to have a new field in pg_conn?  This would also
require a free() in freePGconn() and new PQserviceFile() routine.

+                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;
+                }

Perhaps we should add a test for that?  The same is true with
"service", as I am looking at these code paths now.  I'd suggest to
apply double quotes to the parameter name "servicefile" in this error
message, to make clear what this is.

+   # Additionaly encode a colon in servicefile path of Windows 

Typo: Additionally.

+# Backslashes escaped path string for getting collect result at concatenation
+# for Windows environment

Comment is unclear.  But what you mean here is that the conversion is
required to allow the test to work when giving the path to the
connection option, right?
--
Michael

  25. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Ryo Kanbayashi <kanbayashi.dev@gmail.com> — 2025-06-01T12:36:08Z

    On Wed, May 28, 2025 at 3:48 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sun, Apr 13, 2025 at 07:06:06PM +0900, Ryo Kanbayashi wrote:
> > I rebased our patch according to  2c7bd2ba507e.
> > https://commitfest.postgresql.org/patch/5387/
>
> Thanks for the new version.

Thanks for review :)

> -# for the connection options and their environment variables.
> +# for the connection options, servicefile options and their environment variables.
>
> It seems to me that this comment does not need to be changed.

OK.

> +       {"servicefile", "PGSERVICEFILE", NULL, NULL,
> +       "Database-Service-File", "", 64, -1},
>
> Could it be better to have a new field in pg_conn?  This would also
> require a free() in freePGconn() and new PQserviceFile() routine.

OK.


> +                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;
> +                }
>
> Perhaps we should add a test for that?  The same is true with
> "service", as I am looking at these code paths now.  I'd suggest to
> apply double quotes to the parameter name "servicefile" in this error
> message, to make clear what this is.

I added test cases for nested situations.and double-quoted the parameter names.

> +   # Additionaly encode a colon in servicefile path of Windows
>
> Typo: Additionally.

OK.

> +# Backslashes escaped path string for getting collect result at concatenation
> +# for Windows environment
>
> Comment is unclear.  But what you mean here is that the conversion is
> required to allow the test to work when giving the path to the
> connection option, right?

Strictly speaking, in a Windows environment, a path containing a
backslash is stored in the $td variable, so the value of the
$srvfile_valid variable, which contains that path, also needs to be
converted.
If conversion is not performed, this test will not work correctly in a
Windows environment.
And just because a path string is included does not necessarily mean
that conversion is necessary.

But it's difficult to describe this succinctly...

---
Great regards,
Ryo Kanbayashi

  26. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-06-04T04:42:06Z

    On Sun, Jun 01, 2025 at 09:36:08PM +0900, Ryo Kanbayashi wrote:
> Strictly speaking, in a Windows environment, a path containing a
> backslash is stored in the $td variable, so the value of the
> $srvfile_valid variable, which contains that path, also needs to be
> converted.
> If conversion is not performed, this test will not work correctly in a
> Windows environment.
> And just because a path string is included does not necessarily mean
> that conversion is necessary.
> 
> But it's difficult to describe this succinctly...

+# Backslashes escaped path string for getting collect result at concatenation
+# for Windows environment

I could suggest a simpler sentence here:
"Use correct escaped path for Windows."

Worth noting a few things reported by a `git diff --check`.

+char *
+PQserviceFile(const PGconn *conn)

All these APIs are public and need to be documented.

Another thing that could be added on top of the rest is a psql
variable called SERVICEFILE, and we would be rather feature complete,
with:
- Support in SyncVariables(), as in psql/command.c.
- Some documentation as well for the new psql variable, mapping with
the existing SERVICE.
- Perhaps a shortcut for PROMPT?
- Use of your new libpq API PQserviceFile().
This could be a patch built on top of the introduction of the core API
for the service file.
--
Michael

  27. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Ryo Kanbayashi <kanbayashi.dev@gmail.com> — 2025-06-09T13:25:26Z

    On Wed, Jun 4, 2025 at 1:42 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sun, Jun 01, 2025 at 09:36:08PM +0900, Ryo Kanbayashi wrote:
> > Strictly speaking, in a Windows environment, a path containing a
> > backslash is stored in the $td variable, so the value of the
> > $srvfile_valid variable, which contains that path, also needs to be
> > converted.
> > If conversion is not performed, this test will not work correctly in a
> > Windows environment.
> > And just because a path string is included does not necessarily mean
> > that conversion is necessary.
> >
> > But it's difficult to describe this succinctly...
>
> +# Backslashes escaped path string for getting collect result at concatenation
> +# for Windows environment
>
> I could suggest a simpler sentence here:
> "Use correct escaped path for Windows."

OK.
Thanks for suggestion!

> Worth noting a few things reported by a `git diff --check`.

OK.

> +char *
> +PQserviceFile(const PGconn *conn)
>
> All these APIs are public and need to be documented.

OK.

> Another thing that could be added on top of the rest is a psql
> variable called SERVICEFILE, and we would be rather feature complete,
> with:
> - Support in SyncVariables(), as in psql/command.c.

OK.

> - Some documentation as well for the new psql variable, mapping with
> the existing SERVICE.

OK.

> - Perhaps a shortcut for PROMPT?

I will kindly take a rain check on this one :)

> - Use of your new libpq API PQserviceFile().

OK.

> This could be a patch built on top of the introduction of the core API
> for the service file.

:)

---
Great regards,
Ryo Kanbayashi

  28. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-06-10T07:30:18Z

    On Mon, Jun 09, 2025 at 10:25:26PM +0900, Ryo Kanbayashi wrote:
>> This could be a patch built on top of the introduction of the core API
>> for the service file.
> 
> :)

>> - Perhaps a shortcut for PROMPT?
>
> I will kindly take a rain check on this one :) 

I am not sure to understand what you mean here, but let's discard this
idea as it is also possible to use %:name: in a psql's prompt with the
new variable you are introducing.

+        Defaults to <filename>~/.pg_service.conf</filename>, or
+        <filename>%APPDATA%\postgresql\.pg_service.conf</filename> on
+        Microsoft Windows.

I don't think that this sentence is true.  The parameter does not
default to these values.  The connection logic would fall back to
these files if the parameter is not defined, and the parameter knows
nothing about them.

--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -206,8 +206,9 @@ PQsocketPoll              203
 PQsetChunkedRowsMode      204
 PQgetCurrentTimeUSec      205
 PQservice                 206
-PQsetAuthDataHook         207
-PQgetAuthDataHook         208
-PQdefaultAuthDataHook     209
-PQfullProtocolVersion     210
-appendPQExpBufferVA       211
+PQserviceFile             207
+PQsetAuthDataHook         208
+PQgetAuthDataHook         209
+PQdefaultAuthDataHook     210
+PQfullProtocolVersion     211
+appendPQExpBufferVA       212

The new one goes to the bottom AFAIK.

The patch can be split into multiple pieces:
- Core libpq changes with API and tests.
- psql changes.

+# "service" param included service file (invalid)
+# including contents of pg_service_valid.conf and a nested service option

"Service file with service defined (invalid)."

+# "servicefile" param included service file (invalid)
+# including contents of pg_service_valid.conf and a nested servicefile option

"Service file with servicefile defined (invalid)."
--
Michael

  29. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Ryo Kanbayashi <kanbayashi.dev@gmail.com> — 2025-06-15T12:02:31Z

    On Tue, Jun 10, 2025 at 4:30 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Jun 09, 2025 at 10:25:26PM +0900, Ryo Kanbayashi wrote:
> >> This could be a patch built on top of the introduction of the core API
> >> for the service file.
> >
> > :)
>
> >> - Perhaps a shortcut for PROMPT?
> >
> > I will kindly take a rain check on this one :)
>
> I am not sure to understand what you mean here, but let's discard this
> idea as it is also possible to use %:name: in a psql's prompt with the
> new variable you are introducing.

OK ^^;

> +        Defaults to <filename>~/.pg_service.conf</filename>, or
> +        <filename>%APPDATA%\postgresql\.pg_service.conf</filename> on
> +        Microsoft Windows.
>
> I don't think that this sentence is true.  The parameter does not
> default to these values.  The connection logic would fall back to
> these files if the parameter is not defined, and the parameter knows
> nothing about them.

OK.
I removed the sentence.

> --- a/src/interfaces/libpq/exports.txt
> +++ b/src/interfaces/libpq/exports.txt
> @@ -206,8 +206,9 @@ PQsocketPoll              203
>  PQsetChunkedRowsMode      204
>  PQgetCurrentTimeUSec      205
>  PQservice                 206
> -PQsetAuthDataHook         207
> -PQgetAuthDataHook         208
> -PQdefaultAuthDataHook     209
> -PQfullProtocolVersion     210
> -appendPQExpBufferVA       211
> +PQserviceFile             207
> +PQsetAuthDataHook         208
> +PQgetAuthDataHook         209
> +PQdefaultAuthDataHook     210
> +PQfullProtocolVersion     211
> +appendPQExpBufferVA       212
>
> The new one goes to the bottom AFAIK.

OK.

> The patch can be split into multiple pieces:
> - Core libpq changes with API and tests.
> - psql changes.

OK.
I splited our patch to above 2 pieces.

> +# "service" param included service file (invalid)
> +# including contents of pg_service_valid.conf and a nested service option
>
> "Service file with service defined (invalid)."
>
> +# "servicefile" param included service file (invalid)
> +# including contents of pg_service_valid.conf and a nested servicefile option
>
> "Service file with servicefile defined (invalid)."

OK

Thanks for review :)

---
Great Regards,
Ryo Kanbayashi

  30. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-06-18T03:23:00Z

    On Sun, Jun 15, 2025 at 09:02:31PM +0900, Ryo Kanbayashi wrote:
> Thanks for review :)

Thanks for the new patch.

While testing the patch, I've bumped into this scenario which feels
incomplete:
- Rely on a default location of the service file, like
$HOME/.pg_service.conf.
- Define a service, with PGSERVICE or a connection parameter.
In this case, :SERVICE shows up some information, not :SERVICEFILE
because it remains empty when building a connection file path if we
don't provide PGSERVICEFILE or servicefile as connection option.  It
seems to me that we had better force pg_conn->pgservicefile into a
value in this case, pointing to the value libpq thinks is the default
at the  time of resolving the HOME location in pqGetHomeDirectory()?
It seems to me that you should be able to do that at the end of
parseServiceFile(), at least, if we know that the status is a success
(free value if any, assign the new one, and invent an error code path
for the OOM on strdup()).

Defining PGSERVICEFILE or servicefile in a connection string reports
correctly "pgservicefile" in the libpq connection, of course.  That's
just for the default location paths.

By the way, could you split the test case for the nested "service"
value in a service file into its own file?  This is an existing error
case, and there is no need for the new feature to add this test.
--
Michael

  31. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Ryo Kanbayashi <kanbayashi.dev@gmail.com> — 2025-06-27T12:25:47Z

    On Wed, Jun 18, 2025 at 12:23 PM Michael Paquier <michael@paquier.xyz> wrote:
> While testing the patch, I've bumped into this scenario which feels
> incomplete:
> - Rely on a default location of the service file, like
> $HOME/.pg_service.conf.
> - Define a service, with PGSERVICE or a connection parameter.
> In this case, :SERVICE shows up some information, not :SERVICEFILE
> because it remains empty when building a connection file path if we
> don't provide PGSERVICEFILE or servicefile as connection option.  It
> seems to me that we had better force pg_conn->pgservicefile into a
> value in this case, pointing to the value libpq thinks is the default
> at the  time of resolving the HOME location in pqGetHomeDirectory()?
> It seems to me that you should be able to do that at the end of
> parseServiceFile(), at least, if we know that the status is a success
> (free value if any, assign the new one, and invent an error code path
> for the OOM on strdup()).
>
> Defining PGSERVICEFILE or servicefile in a connection string reports
> correctly "pgservicefile" in the libpq connection, of course.  That's
> just for the default location paths.

OK.
I think I was able to fix it as per your request :)

> By the way, could you split the test case for the nested "service"
> value in a service file into its own file?  This is an existing error
> case, and there is no need for the new feature to add this test.

No problem.
I've attached modified and splited patch files to this mail.

---
Great regards,
Ryo Kanbayashi

  32. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-07-09T07:31:31Z

    On Fri, Jun 27, 2025 at 09:25:47PM +0900, Ryo Kanbayashi wrote:
> I've attached modified and splited patch files to this mail.

Taken in isolation, 0001 was incorrect because it still contained the
case of "servicefile" nested to a service file, but this code path is
only introduced in 0002.  I have extracted the relevant part of the
patch that works on HEAD, and applied it.

Attached is a rebased version of the rest, with the recent stanza
related to fef6da9e9c87 taken into account.  0002 still has a change
that should be in 0001: I have not really touched the structure of the
two remaining patches yet.
--
Michael

  33. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-07-10T05:21:38Z

    On Wed, Jul 09, 2025 at 04:31:31PM +0900, Michael Paquier wrote:
> Attached is a rebased version of the rest, with the recent stanza
> related to fef6da9e9c87 taken into account.  0002 still has a change
> that should be in 0001: I have not really touched the structure of the
> two remaining patches yet.

+    /* If service was found successfully, set servicefile option if not already set */
+    if (*group_found && result == 0)
+    {
+        for (i = 0; options[i].keyword; i++)
+        {
+            if (strcmp(options[i].keyword, "servicefile") != 0)
+                continue;
+
+            if (options[i].val != NULL)
+                break;
+
+            options[i].val = strdup(serviceFile);
+            if (options[i].val == NULL)
+            {
+                libpq_append_error(errorMessage, "out of memory");
+                return 3;
+            }
+            break;

There was a bug here: if the new value cannot be strdup'd, we would
miss the fclose() of the exit path, so this cannot return directly.
It is possible to set the status to a new value instead, then break.

After that, I have applied a few cosmetic tweaks here and there, and
attached is what I have staged for commit, minus proper commit
messages.  The new TAP tests have some WIN32-specific things, and I
won't be able to look at the buildfarm if I were to apply things
today, so this will have to wait until the beginning of next week.
The CI is happy with it, so at least we are one checkbox down.
--
Michael

  34. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Michael Paquier <michael@paquier.xyz> — 2025-07-14T00:10:41Z

    On Thu, Jul 10, 2025 at 02:21:38PM +0900, Michael Paquier wrote:
> After that, I have applied a few cosmetic tweaks here and there, and
> attached is what I have staged for commit, minus proper commit
> messages.  The new TAP tests have some WIN32-specific things, and I
> won't be able to look at the buildfarm if I were to apply things
> today, so this will have to wait until the beginning of next week.
> The CI is happy with it, so at least we are one checkbox down.

Both are now applied as of 6b1c4d326b06 and 092f3c63efc6.

The scary part was the WIN32 buildfarm members, which I've confirmed
have accepted the new tests.  So we should be good here.
--
Michael

  35. Re: [PATCH] PGSERVICEFILE as part of a normal connection string

    Ryo Kanbayashi <kanbayashi.dev@gmail.com> — 2025-07-14T09:41:56Z

    On Mon, Jul 14, 2025 at 9:10 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Jul 10, 2025 at 02:21:38PM +0900, Michael Paquier wrote:
> > After that, I have applied a few cosmetic tweaks here and there, and
> > attached is what I have staged for commit, minus proper commit
> > messages.  The new TAP tests have some WIN32-specific things, and I
> > won't be able to look at the buildfarm if I were to apply things
> > today, so this will have to wait until the beginning of next week.
> > The CI is happy with it, so at least we are one checkbox down.
>
> Both are now applied as of 6b1c4d326b06 and 092f3c63efc6.
>
> The scary part was the WIN32 buildfarm members, which I've confirmed
> have accepted the new tests.  So we should be good here.

Thank you for your long-time review responses and comments :)

I will do my best for next patch as well :)

---
Great regards,
NTT Open Source Software Center
Ryo Kanbayashi