Re: PGSERVICEFILE as part of a normal connection string

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