On Wed, May 28, 2025 at 3:48 PM Michael Paquier <mich...@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
v8-0001-add-servicefile-option-usage-on-connection-string.patch
Description: Binary data
