Обсуждение: Re: [PATCH] Add support for displaying database service in psql prompt

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

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

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.

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.

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.

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