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

From: Michael Banck <[email-masked]>
Date: Mon, 16 Dec 2024 22:57:49 +0100

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
>From 666b80297f1cb918230b5104e2e8dce08b711394 Mon Sep 17 00:00:00 2001
From: Michael Banck <[email-masked]>
Date: Thu, 31 Oct 2024 18:27:52 +0100
Subject: [PATCH v2 1/2] Add PQservice to PGConn.

This adds the content of the database service (if any) to PGConn. One
use for this would be for psql to display the service as part of the
prompt. It also adds PGservice() as a new connection status unction.
---
 doc/src/sgml/libpq.sgml           | 20 ++++++++++++++++++++
 src/interfaces/libpq/exports.txt  |  1 +
 src/interfaces/libpq/fe-connect.c | 11 ++++++++++-
 src/interfaces/libpq/libpq-fe.h   |  1 +
 src/interfaces/libpq/libpq-int.h  |  1 +
 5 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 01f259fd0d..105b22b317 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2530,6 +2530,26 @@ char *PQport(const PGconn *conn);
      </listitem>
     </varlistentry>
 
+    <varlistentry id="libpq-PQservice">
+     <term><function>PQservice</function><indexterm><primary>PQservice</primary></indexterm></term>
+
+     <listitem>
+      <para>
+       Returns the service of the active connection.
+
+<synopsis>
+char *PQservice(const PGconn *conn);
+</synopsis>
+      </para>
+
+      <para>
+       <xref linkend="libpq-PQservice"/> returns <symbol>NULL</symbol> if the
+       <parameter>conn</parameter> argument is <symbol>NULL</symbol>.
+       Otherwise, if there was no service provided, it returns an empty string.
+      </para>
+     </listitem>
+    </varlistentry>
+
     <varlistentry id="libpq-PQtty">
      <term><function>PQtty</function><indexterm><primary>PQtty</primary></indexterm></term>
 
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 5d8213e0b5..2ad2cbf5ca 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -205,3 +205,4 @@ PQcancelFinish            202
 PQsocketPoll              203
 PQsetChunkedRowsMode      204
 PQgetCurrentTimeUSec      205
+PQservice                 206
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index aaf87e8e88..ddcc7b60ab 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -190,7 +190,8 @@ typedef struct _internalPQconninfoOption
 
 static const internalPQconninfoOption PQconninfoOptions[] = {
 	{"service", "PGSERVICE", NULL, NULL,
-	"Database-Service", "", 20, -1},
+		"Database-Service", "", 20,
+	offsetof(struct pg_conn, pgservice)},
 
 	{"user", "PGUSER", NULL, NULL,
 		"Database-User", "", 20,
@@ -7040,6 +7041,14 @@ PQdb(const PGconn *conn)
 	return conn->dbName;
 }
 
+char *
+PQservice(const PGconn *conn)
+{
+	if (!conn)
+		return NULL;
+	return conn->pgservice;
+}
+
 char *
 PQuser(const PGconn *conn)
 {
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 15012c770c..5947e7c766 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -385,6 +385,7 @@ extern int	PQrequestCancel(PGconn *conn);
 
 /* Accessor functions for PGconn objects */
 extern char *PQdb(const PGconn *conn);
+extern char *PQservice(const PGconn *conn);
 extern char *PQuser(const PGconn *conn);
 extern char *PQpass(const PGconn *conn);
 extern char *PQhost(const PGconn *conn);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 08cc391cbd..dcebca9898 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -394,6 +394,7 @@ struct pg_conn
 	char	   *fbappname;		/* fallback application name */
 	char	   *dbName;			/* database name */
 	char	   *replication;	/* connect as the replication standby? */
+	char	   *pgservice;		/* Postgres service, if any */
 	char	   *pguser;			/* Postgres username and password, if any */
 	char	   *pgpass;
 	char	   *pgpassfile;		/* path to a file containing password(s) */
-- 
2.39.5

>From 5faa624871e3267a780d794e9ad2e9f42fd42311 Mon Sep 17 00:00:00 2001
From: Michael Banck <[email-masked]>
Date: Thu, 31 Oct 2024 18:49:14 +0100
Subject: [PATCH v2 2/2] Add support for database service to psql prompt.

This adds a new psql variable SERVICE as well as the string '%s' for the
psql PROMPT, displaying the service (from PGSERVICEFILE) if so desired.
---
 doc/src/sgml/ref/psql-ref.sgml | 14 ++++++++++++++
 src/bin/psql/command.c         |  2 ++
 src/bin/psql/prompt.c          |  6 ++++++
 3 files changed, 22 insertions(+)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index e42073ed74..af78e0e87b 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4380,6 +4380,15 @@ bar
         </listitem>
       </varlistentry>
 
+      <varlistentry id="app-psql-variables-service">
+        <term><varname>SERVICE</varname></term>
+        <listitem>
+        <para>
+        The service from <filename>pg_service.conf</filename>, if applicable.
+        </para>
+        </listitem>
+      </varlistentry>
+
       <varlistentry id="app-psql-variables-shell-error">
        <term><varname>SHELL_ERROR</varname></term>
        <listitem>
@@ -4685,6 +4694,11 @@ testdb=&gt; <userinput>INSERT INTO my_table VALUES (:'content');</userinput>
          (tilde) if the database is your default database.</para></listitem>
       </varlistentry>
 
+      <varlistentry id="app-psql-prompting-s">
+        <term><literal>%s</literal></term>
+        <listitem><para>The name of the service entry, if any.</para></listitem>
+      </varlistentry>
+
       <varlistentry id="app-psql-prompting-numbersign">
         <term><literal>%#</literal></term>
         <listitem>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 1f3cbb11f7..cd16f27947 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -4082,6 +4082,7 @@ SyncVariables(void)
 	pset.sversion = PQserverVersion(pset.db);
 
 	SetVariable(pset.vars, "DBNAME", PQdb(pset.db));
+	SetVariable(pset.vars, "SERVICE", PQservice(pset.db));
 	SetVariable(pset.vars, "USER", PQuser(pset.db));
 	SetVariable(pset.vars, "HOST", PQhost(pset.db));
 	SetVariable(pset.vars, "PORT", PQport(pset.db));
@@ -4115,6 +4116,7 @@ void
 UnsyncVariables(void)
 {
 	SetVariable(pset.vars, "DBNAME", NULL);
+	SetVariable(pset.vars, "SERVICE", NULL);
 	SetVariable(pset.vars, "USER", NULL);
 	SetVariable(pset.vars, "HOST", NULL);
 	SetVariable(pset.vars, "PORT", NULL);
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 0d99d00ac9..de3d976103 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -33,6 +33,7 @@
  * %p - backend pid
  * %> - database server port number
  * %n - database user name
+ * %s - service
  * %/ - current database
  * %~ - like %/ but "~" when database name equals user name
  * %w - whitespace of the same width as the most recent output of PROMPT1
@@ -246,6 +247,11 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
 						}
 					break;
 
+				case 's':
+					if (PQservice(pset.db))
+						strlcpy(buf, PQservice(pset.db), sizeof(buf));
+					break;
+
 				case 'l':
 					snprintf(buf, sizeof(buf), UINT64_FORMAT, pset.stmt_lineno);
 					break;
-- 
2.39.5

---

From: Michael Paquier <[email-masked]>
Date: Tue, 17 Dec 2024 16:43:21 +0900

> 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':

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>

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.

[...]
+PQservice                 206

You didn't miss that, nice.

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

---

From: Michael Banck <[email-masked]>
Date: Tue, 17 Dec 2024 09:42:36 +0100

Hi,

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

Thanks for the further review.

> that does not exist.  For consistency with the surroundings this

Ok, I've done that.

> applicable".  No need to be fancy.

Done.
 
> Other entries don't use "if any", would just cut it.

Done.

V3 attached.

Michael
>From 1e71982edce8744f48906d2857d22727691b7267 Mon Sep 17 00:00:00 2001
From: Michael Banck <[email-masked]>
Date: Thu, 31 Oct 2024 18:27:52 +0100
Subject: [PATCH v3 1/2] Add PQservice to PGConn.

 

 
 
+char *

>From f70fc781adf0833ecd148d672f22d6a1d860c365 Mon Sep 17 00:00:00 2001
From: Michael Banck <[email-masked]>
Date: Thu, 31 Oct 2024 18:49:14 +0100
Subject: [PATCH v3 2/2] Add support for database service to psql prompt.

index e42073ed74..6ba85d9acf 100644
 
+        The service name, if applicable.
 
+        <listitem><para>The name of the service entry.</para></listitem>
 
index 0d99d00ac9..3b3f079229 100644
 
+				case 's':
+					if (pset.db && PQservice(pset.db))
+						strlcpy(buf, PQservice(pset.db), sizeof(buf));
+					break;
+
 				case 'l':
 					snprintf(buf, sizeof(buf), UINT64_FORMAT, pset.stmt_lineno);
 					break;
-- 
2.39.5

---

From: Michael Paquier <[email-masked]>
Date: Wed, 18 Dec 2024 15:17:36 +0900

Done.
--
Michael

---

From: Michael Banck <[email-masked]>
Date: Thu, 19 Dec 2024 13:36:44 +0100

Hi,

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

Thanks!

Michael

---

From: Noah Misch <[email-masked]>
Date: Sun, 6 Jul 2025 09:13:19 -0700

> 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.

---

From: Michael Paquier <[email-masked]>
Date: Mon, 7 Jul 2025 11:06:06 +0900

> the commit (adding the struct field, necessary for PQconninfo() to include the

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
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 9fcd2db83265..c839634d4233 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -4480,6 +4480,7 @@ SyncVariables(void)
 {
 	char		vbuf[32];
 	const char *server_version;
+	char	   *service_name;
 
 	/* get stuff from connection */
 	pset.encoding = PQclientEncoding(pset.db);
@@ -4489,12 +4490,16 @@ SyncVariables(void)
 	setFmtEncoding(pset.encoding);
 
 	SetVariable(pset.vars, "DBNAME", PQdb(pset.db));
-	SetVariable(pset.vars, "SERVICE", PQservice(pset.db));
 	SetVariable(pset.vars, "USER", PQuser(pset.db));
 	SetVariable(pset.vars, "HOST", PQhost(pset.db));
 	SetVariable(pset.vars, "PORT", PQport(pset.db));
 	SetVariable(pset.vars, "ENCODING", pg_encoding_to_char(pset.encoding));
 
+	service_name = get_service_name();
+	SetVariable(pset.vars, "SERVICE", service_name);
+	if (service_name)
+		pg_free(service_name);
+
 	/* this bit should match connection_warnings(): */
 	/* Try to get full text form of version, might include "devel" etc */
 	server_version = PQparameterStatus(pset.db, "server_version");
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index d2c0a49c46c0..3593c0468310 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -2531,6 +2531,41 @@ session_username(void)
 		return PQuser(pset.db);
 }
 
+/*
+ * Return the service name of the current connection.
+ *
+ * The caller is responsible for freeing the result value allocated.
+ */
+char *
+get_service_name(void)
+{
+	PQconninfoOption   *opts;
+	PQconninfoOption   *serviceopt = NULL;
+	char	   *res = NULL;
+
+	if (pset.db == NULL)
+		return NULL;
+
+	opts = PQconninfo(pset.db);
+	if (opts == NULL)
+		return NULL;
+
+	for (PQconninfoOption *opt = opts; opt->keyword != NULL; ++opt)
+	{
+		if (strcmp(opt->keyword, "service") == 0)
+		{
+			serviceopt = opt;
+			continue;
+		}
+	}
+
+	/* Take a copy of the value, as it is freed by PQconninfoFree(). */
+	if (serviceopt && serviceopt->val != NULL)
+		res = pg_strdup(serviceopt->val);
+	PQconninfoFree(opts);
+
+	return res;
+}
 
 /* expand_tilde
  *
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index 7f1a23de1e82..261199df3187 100644
--- a/src/bin/psql/common.h
+++ b/src/bin/psql/common.h
@@ -39,6 +39,7 @@ extern bool SendQuery(const char *query);
 extern bool is_superuser(void);
 extern bool standard_strings(void);
 extern const char *session_username(void);
+extern char *get_service_name(void);
 
 extern void expand_tilde(char **filename);
 extern void clean_extended_state(void);
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 3aa7d2d06c80..cdae9b681500 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -169,8 +169,15 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
 					break;
 					/* service name */
 				case 's':
-					if (pset.db && PQservice(pset.db))
-						strlcpy(buf, PQservice(pset.db), sizeof(buf));
+					{
+						char	   *service_name = get_service_name();
+
+						if (service_name)
+						{
+							strlcpy(buf, service_name, sizeof(buf));
+							pg_free(service_name);
+						}
+					}
 					break;
 					/* backend pid */
 				case 'p':
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 0625cf39e9af..dbbae642d769 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -205,9 +205,8 @@ PQcancelFinish            202
 PQsocketPoll              203
 PQsetChunkedRowsMode      204
 PQgetCurrentTimeUSec      205
-PQservice                 206
-PQsetAuthDataHook         207
-PQgetAuthDataHook         208
-PQdefaultAuthDataHook     209
-PQfullProtocolVersion     210
-appendPQExpBufferVA       211
+PQsetAuthDataHook         206
+PQgetAuthDataHook         207
+PQdefaultAuthDataHook     208
+PQfullProtocolVersion     209
+appendPQExpBufferVA       210
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 51a9c4165845..09eb79812ac6 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -7461,14 +7461,6 @@ PQdb(const PGconn *conn)
 	return conn->dbName;
 }
 
-char *
-PQservice(const PGconn *conn)
-{
-	if (!conn)
-		return NULL;
-	return conn->pgservice;
-}
-
 char *
 PQuser(const PGconn *conn)
 {
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 7d3a9df6fd55..af8004f952a5 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -400,7 +400,6 @@ extern int	PQrequestCancel(PGconn *conn);
 
 /* Accessor functions for PGconn objects */
 extern char *PQdb(const PGconn *conn);
-extern char *PQservice(const PGconn *conn);
 extern char *PQuser(const PGconn *conn);
 extern char *PQpass(const PGconn *conn);
 extern char *PQhost(const PGconn *conn);
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 298c4b38ef90..b2c2cf9eac83 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2740,26 +2740,6 @@ char *PQport(const PGconn *conn);
      </listitem>
     </varlistentry>
 
-    <varlistentry id="libpq-PQservice">
-     <term><function>PQservice</function><indexterm><primary>PQservice</primary></indexterm></term>
-
-     <listitem>
-      <para>
-       Returns the service of the active connection.
-
-<synopsis>
-char *PQservice(const PGconn *conn);
-</synopsis>
-      </para>
-
-      <para>
-       <xref linkend="libpq-PQservice"/> returns <symbol>NULL</symbol> if the
-       <parameter>conn</parameter> argument is <symbol>NULL</symbol>.
-       Otherwise, if there was no service provided, it returns an empty string.
-      </para>
-     </listitem>
-    </varlistentry>
-
     <varlistentry id="libpq-PQtty">
      <term><function>PQtty</function><indexterm><primary>PQtty</primary></indexterm></term>

---

From: Noah Misch <[email-masked]>
Date: Sun, 6 Jul 2025 20:00:09 -0700

> 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.

---

From: Michael Paquier <[email-masked]>
Date: Tue, 8 Jul 2025 08:52:08 +0900

> 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

---

From: Noah Misch <[email-masked]>
Date: Tue, 8 Jul 2025 17:41:32 -0700

> 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.

---

From: Michael Paquier <[email-masked]>
Date: Wed, 9 Jul 2025 13:29:17 +0900

> 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