[PATCH] Add support for displaying database service in psql prompt

Hi,

I think it might be useful to sometimes display the database service
(i.e. what is defined in ~/.pg_service.conf and used via psql
service=foo) in the psql prompt, e.g. if you have 'test' and 'prod'
services, but both have the same database name. This was also suggested
to me during a recent customer training.

I chose the '%s' tag for it. I had to add the service to PGConn as
PQservice (first patch) to libpq and then use it in psql in the second
patch.

Michael

Compiled and tested: works fine, so +1 from me. Honestly, I was surprised
%s was still available. :)

Cheers,
Greg

Hi,

On Tue, Nov 19, 2024 at 07:47:58PM -0500, Greg Sabino Mullane wrote:
> Compiled and tested: works fine, so +1 from me. Honestly, I was surprised
> %s was still available. :)

Thanks. Was that full review? You kept the commitfest item in "Needs
Review" state.

Michael

On Thu, Nov 28, 2024 at 09:17:23PM +0100, Michael Banck wrote:
> Hi,
>
> On Tue, Nov 19, 2024 at 07:47:58PM -0500, Greg Sabino Mullane wrote:
> > Compiled and tested: works fine, so +1 from me. Honestly, I was surprised
> > %s was still available. :)
>
> Thanks. Was that full review? You kept the commitfest item in "Needs
> Review" state.

No objections to this proposal, that can be useful. No objections
with the use of 's' for the shortcut in the psql prompt.

Back to your patch, you are missing two things at quick glance:
- Documentation for the new libpq API in 0001.
- Documentation for the new SERVICE and its new %s (see section called
"Prompting" on the psql page.

So please make sure to provide these with the next version of the
patch.
--
Michael

Hi Michael,

On Tue, Dec 10, 2024 at 04:38:24PM +0900, Michael Paquier wrote:
> On Thu, Nov 28, 2024 at 09:17:23PM +0100, Michael Banck wrote:
> > On Tue, Nov 19, 2024 at 07:47:58PM -0500, Greg Sabino Mullane wrote:
> > > Compiled and tested: works fine, so +1 from me. Honestly, I was surprised
> > > %s was still available. :)
> >
> > Thanks. Was that full review? You kept the commitfest item in "Needs
> > Review" state.
>
> No objections to this proposal, that can be useful. No objections
> with the use of 's' for the shortcut in the psql prompt.

Cool.

> Back to your patch, you are missing two things at quick glance:
> - Documentation for the new libpq API in 0001.
> - Documentation for the new SERVICE and its new %s (see section called
> "Prompting" on the psql page.
>
> So please make sure to provide these with the next version of the
> patch.

Thanks, I have added the documentation now in v2.

Michael

On Mon, Dec 16, 2024 at 10:57:49PM +0100, Michael Banck wrote:
> Thanks, I have added the documentation now in v2.

The doc additions seem fine to me. I've just grabbed three tiny nits,
nothing critical.

+ case 's':
+ if (PQservice(pset.db))
+ strlcpy(buf, PQservice(pset.db), sizeof(buf));
+ break;

Other code paths of get_prompt check for pset.db being NULL. True
that it does not matter when calling PQservice() with a connection
that does not exist. For consistency with the surroundings this
should be done at least?

+ <para>
+ The service from <filename>pg_service.conf</filename>, if applicable.
+ </para>

pg_service.conf is not especially true as it depends on the
environment used. For psql, perhaps just use "The service name, if
applicable". No need to be fancy.

--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -205,3 +205,4 @@ PQcancelFinish 202
[...]
+PQservice 206

You didn't miss that, nice.

+ <varlistentry id="app-psql-prompting-s">
+ <term><literal>%s</literal></term>
+ <listitem><para>The name of the service entry, if any.</para></listitem>
+ </varlistentry>

Other entries don't use "if any", would just cut it.
--
Michael

Hi,

On Tue, Dec 17, 2024 at 04:43:21PM +0900, Michael Paquier wrote:
> On Mon, Dec 16, 2024 at 10:57:49PM +0100, Michael Banck wrote:
> > Thanks, I have added the documentation now in v2.
>
> The doc additions seem fine to me. I've just grabbed three tiny nits,
> nothing critical.

Thanks for the further review.

> + case 's':
> + if (PQservice(pset.db))
> + strlcpy(buf, PQservice(pset.db), sizeof(buf));
> + break;
>
> Other code paths of get_prompt check for pset.db being NULL. True
> that it does not matter when calling PQservice() with a connection
> that does not exist. For consistency with the surroundings this
> should be done at least?

Ok, I've done that.

>
> + <para>
> + The service from <filename>pg_service.conf</filename>, if applicable.
> + </para>
>
> pg_service.conf is not especially true as it depends on the
> environment used. For psql, perhaps just use "The service name, if
> applicable". No need to be fancy.

Done.

> + <varlistentry id="app-psql-prompting-s">
> + <term><literal>%s</literal></term>
> + <listitem><para>The name of the service entry, if any.</para></listitem>
> + </varlistentry>
>
> Other entries don't use "if any", would just cut it.

Done.

V3 attached.

Michael

On Tue, Dec 17, 2024 at 09:42:36AM +0100, Michael Banck wrote:
> Done.
>
> V3 attached.

Done.
--
Michael

Hi,

On Wed, Dec 18, 2024 at 03:17:36PM +0900, Michael Paquier wrote:
> On Tue, Dec 17, 2024 at 09:42:36AM +0100, Michael Banck wrote:
> > Done.
> >
> > V3 attached.
>
> Done.

Thanks!

Michael