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

On Fri, Mar 28, 2025 at 10:44 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Thu, Mar 27, 2025 at 06:31:14PM +0900, Michael Paquier wrote:
> > With all that in mind and more documentation added to the test, I've
> > applied 0001, so let's see what the buildfarm has to say. The CI was
> > stable, so it's a start.
>
> The buildfarm (particularly the Windows members that worried me), have
> reported back and I am not seeing any failures, so we should be good
> with 72c2f36d5727.

Thank you for review and additional modification to the patch.
I'm glad the patch was made in time for this release, even if it was
just a partial one.

On Fri, Mar 28, 2025 at 8:57 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> > I am not sure that I'll have the time to look at 0002 for this release
> > cycle, could it be possible to get a rebase for it?
> Here is a simple rebase that I have been able to assemble this
> morning. I won't have the space to review it for this release cycle
> unfortunately, but at least it works in the CI.

I'm sorry I couldn't respond to your request :(

> I am moving this patch entry to the next CF for v19, as a result of
> that.

OK
Thanks :)

On Thu, Mar 27, 2025 at 6:31 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> I have spent quite a bit of time on the review 0001 with the new
> tests to get something in for this release, and there was quite a bit
> going on there:
> - The script should set PGSYSCONFDIR, or it could grab data that
> depend on the host. This can use the temporary folder created in the
> test.
> - On the same ground, we need a similar tweak for PGSERVICEFILE or we
> would go into pqGetHomeDirectory() and look at a HOME folder (WIN32
> and non-WIN32).
>
> With that addressed, there could be much more tests, like for cases
> where PGSERVICEFILE is set but points to a file that does not exist,
> more combinations between URIs, connection parameters and PGSERVICE,
> for success and failure cases, empty service file, etc.
>
> Another thing that I've noticed to be useful to cover is the case
> based on the hardcoded service file name pg_service.conf in
> PGSYSCONFDIR, which is used as a fallback in the code if the service
> name cannot be found in the initial PGSERVICEFILE, acting as a
> fallback option. As long as PGSYSCONFDIR is set, we could test one in
> isolation using the temporary folder created by the test.

I check and modify 0002 patch (adding servicefile option and its
regression tests)
in light of the above and committed 0001 patch (regression test of
existing features)
toward next release :)

---
Great Regards,
Ryo Kanbayashi