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

On Sat, Mar 29, 2025 at 3:35 PM Ryo Kanbayashi <kanbayashi(dot)dev(at)gmail(dot)com> wrote:
> 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 :)

Although it probably won't be ready in time for this release, I've
created new 0001 patch (former 0002) which is reflected your review
comments.

I checked That the patch passes CI of my GitHub repository.

Best of luck :)

---
Great regards,
Ryo Kanbayashi