Horton

Thread

Commits

  1. libpq: Remove PQservice()

  2. psql: Add more information about service name

  3. libpq: Add service name to PGconn and PQservice()

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

    Michael Banck <mbanck@gmx.net> — 2024-10-31T18:01:54Z

    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

  2. Re: [PATCH] Add support for displaying database service in psql prompt

    Greg Sabino Mullane <htamfids@gmail.com> — 2024-11-20T00:47:58Z

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

Cheers,
Greg

  3. Re: [PATCH] Add support for displaying database service in psql prompt

    Michael Banck <mbanck@gmx.net> — 2024-11-28T20:17:23Z

    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

  4. Re: [PATCH] Add support for displaying database service in psql prompt

    Michael Paquier <michael@paquier.xyz> — 2024-12-10T07:38:24Z

    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

  5. Re: [PATCH] Add support for displaying database service in psql prompt

    Michael Banck <mbanck@gmx.net> — 2024-12-16T21:57:49Z

    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

  6. Re: [PATCH] Add support for displaying database service in psql prompt

    Michael Paquier <michael@paquier.xyz> — 2024-12-17T07:43:21Z

    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

  7. Re: [PATCH] Add support for displaying database service in psql prompt

    Michael Banck <mbanck@gmx.net> — 2024-12-17T08:42:36Z

    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

  8. Re: [PATCH] Add support for displaying database service in psql prompt

    Michael Paquier <michael@paquier.xyz> — 2024-12-18T06:17:36Z

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

Done.
--
Michael

  9. Re: [PATCH] Add support for displaying database service in psql prompt

    Michael Banck <mbanck@gmx.net> — 2024-12-19T12:36:44Z

    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

  10. Re: [PATCH] Add support for displaying database service in psql prompt

    Noah Misch <noah@leadboat.com> — 2025-07-06T16:13:19Z

    On Wed, Dec 18, 2024 at 03:17:36PM +0900, Michael Paquier wrote:
> Done.

This new PQservice() function from commit 4b99fed75 came up in the annual
exports.txt diff.  The standard in libpq has been to not clutter the API with
new functions that simply retrieve one PQconninfoOption value.  PQconninfo()
provides access to all those values in a generic way.  What do you think of
making psql use PQconninfo() for this, then removing PQservice()?  The rest of
the commit (adding the struct field, necessary for PQconninfo() to include the
value) looks good.

  11. Re: [PATCH] Add support for displaying database service in psql prompt

    Michael Paquier <michael@paquier.xyz> — 2025-07-07T02:06:06Z

    On Sun, Jul 06, 2025 at 09:13:19AM -0700, Noah Misch wrote:
> This new PQservice() function from commit 4b99fed75 came up in the annual
> exports.txt diff.  The standard in libpq has been to not clutter the API with
> new functions that simply retrieve one PQconninfoOption value.  PQconninfo()
> provides access to all those values in a generic way.  What do you think of
> making psql use PQconninfo() for this, then removing PQservice()?  The rest of
> the commit (adding the struct field, necessary for PQconninfo() to include the
> value) looks good.

Sure, I was not aware of such a policy.  Relying on PQconninfoOption
is less efficient because we would need to look through the whole set
of options when looking for the service name, and this needs one extra
allocation as PQconninfoFree() frees the values allocated.  With two
callers perhaps this inefficiency is OK to live with anyway.

What do you think about the attached, then?
--
Michael

  12. Re: [PATCH] Add support for displaying database service in psql prompt

    Noah Misch <noah@leadboat.com> — 2025-07-07T03:00:09Z

    On Mon, Jul 07, 2025 at 11:06:06AM +0900, Michael Paquier wrote:
> On Sun, Jul 06, 2025 at 09:13:19AM -0700, Noah Misch wrote:
> > This new PQservice() function from commit 4b99fed75 came up in the annual
> > exports.txt diff.  The standard in libpq has been to not clutter the API with
> > new functions that simply retrieve one PQconninfoOption value.  PQconninfo()
> > provides access to all those values in a generic way.  What do you think of
> > making psql use PQconninfo() for this, then removing PQservice()?  The rest of
> > the commit (adding the struct field, necessary for PQconninfo() to include the
> > value) looks good.
> 
> Sure, I was not aware of such a policy.  Relying on PQconninfoOption
> is less efficient because we would need to look through the whole set
> of options when looking for the service name, and this needs one extra
> allocation as PQconninfoFree() frees the values allocated.  With two
> callers perhaps this inefficiency is OK to live with anyway.

I think the choice to make there is whether to call PQconninfo() once per
prompt emission or to cache the value, invalidating that cache e.g. once per
SyncVariables().  My first thought was to cache, but the decision is not too
important.  A PQconninfo() call is likely negligible relative to all that
happens between prompts.  Even if not negligible, the overhead of not caching
will affect only prompts using the new escape sequence.

  13. Re: [PATCH] Add support for displaying database service in psql prompt

    Michael Paquier <michael@paquier.xyz> — 2025-07-07T23:52:08Z

    On Sun, Jul 06, 2025 at 08:00:09PM -0700, Noah Misch wrote:
> I think the choice to make there is whether to call PQconninfo() once per
> prompt emission or to cache the value, invalidating that cache e.g. once per
> SyncVariables().  My first thought was to cache, but the decision is not too
> important.  A PQconninfo() call is likely negligible relative to all that
> happens between prompts.  Even if not negligible, the overhead of not caching
> will affect only prompts using the new escape sequence.

SyncVariables() happens at startup and when re-syncing a connection
during a check, so that does not really worry me.

By the way, there is a second change in the CF that's suggesting the
addition of a SERVICEFILE variable, which also uses a separate libpq
API (forgot about this one):
https://commitfest.postgresql.org/patch/5387/

Changing the patch on the other thread to use a conninfo is
stright-forward.  How about extending the get_service_name()@common.c
I've proposed upthread so as it takes a string in input and it could
be reused for more connection options than only "service" so as it
could be reused there as well?
--
Michael

  14. Re: [PATCH] Add support for displaying database service in psql prompt

    Noah Misch <noah@leadboat.com> — 2025-07-09T00:41:32Z

    On Tue, Jul 08, 2025 at 08:52:08AM +0900, Michael Paquier wrote:
> On Sun, Jul 06, 2025 at 08:00:09PM -0700, Noah Misch wrote:
> > I think the choice to make there is whether to call PQconninfo() once per
> > prompt emission or to cache the value, invalidating that cache e.g. once per
> > SyncVariables().  My first thought was to cache, but the decision is not too
> > important.  A PQconninfo() call is likely negligible relative to all that
> > happens between prompts.  Even if not negligible, the overhead of not caching
> > will affect only prompts using the new escape sequence.
> 
> SyncVariables() happens at startup and when re-syncing a connection
> during a check, so that does not really worry me.
> 
> By the way, there is a second change in the CF that's suggesting the
> addition of a SERVICEFILE variable, which also uses a separate libpq
> API (forgot about this one):
> https://commitfest.postgresql.org/patch/5387/
> 
> Changing the patch on the other thread to use a conninfo is
> stright-forward.  How about extending the get_service_name()@common.c
> I've proposed upthread so as it takes a string in input and it could
> be reused for more connection options than only "service" so as it
> could be reused there as well?

I'd prefer not to get involved in decisions affecting only psql efficiency and
psql code cosmetics.  Please make that decision without my input.

  15. Re: [PATCH] Add support for displaying database service in psql prompt

    Michael Paquier <michael@paquier.xyz> — 2025-07-09T04:29:17Z

    On Tue, Jul 08, 2025 at 05:41:32PM -0700, Noah Misch wrote:
> I'd prefer not to get involved in decisions affecting only psql efficiency and
> psql code cosmetics.  Please make that decision without my input.

Okay, I have used this more extensible routine then, planning to use
it for the other patch.  The prompt shortcut retrieves the value using
a GetVariable(), rather than looking at the connection options all the
time.
--
Michael