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

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(dot)dev(at)gmail(dot)com>
wrote:

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