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

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

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.

---

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