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

On Sat, Mar 22, 2025 at 4:46 PM Michael Paquier <michael(at)paquier(dot)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