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

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