Make message strings in fe-connect.c consistent
When reviewing a recently committed patch [1]Support connection load balancing in libpq 7f5b19817eaf38e70ad1153db4e644ee9456853e I noticed the odd usage
of a format specifier:
+ libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
+ "load_balance_hosts",
+ conn->load_balance_hosts);The oddity is that the first %s is unnecessary, since the value we
want there is a constant. Typically a format specifier used to get the
value stored in a variable.
Upon closer look, it looks like this is a common pattern in
fe-connect.c; there are many places where a %s format specifier is
being used in the format sting, where the name of the parameter would
have sufficed.
Upon some research, the only explanation I could come up with was that
this pattern of specifying error messages helps with message
translations. This way there's just one message to be translated. For
example:
.../libpq/po/es.po-#: fe-connect.c:1268 fe-connect.c:1294
fe-connect.c:1336 fe-connect.c:1345
.../libpq/po/es.po-#: fe-connect.c:1378 fe-connect.c:1422
.../libpq/po/es.po-#, c-format
.../libpq/po/es.po:msgid "invalid %s value: \"%s\"\n"
There's just one exception to this pattern, though.
libpq_append_conn_error(conn, "invalid require_auth method: \"%s\"",
method);
So, to make it consistent throughout the file, we should either
replace all such %s format specifiers with the respective strings, or
use the same pattern for the message string used for require_method,
as well. Attached patch [2]Replace placeholders with known strings v1-0001-Replace-placeholders-with-known-strings.patch does the former, and patch [3]Make require_auth error message similar to surrounding messages v1-0001-Make-require_auth-error-message-similar-to-surrou.patch does the
latter.
Pick your favorite one.
[1]: Support connection load balancing in libpq 7f5b19817eaf38e70ad1153db4e644ee9456853e
7f5b19817eaf38e70ad1153db4e644ee9456853e
[2]: Replace placeholders with known strings v1-0001-Replace-placeholders-with-known-strings.patch
v1-0001-Replace-placeholders-with-known-strings.patch
[3]: Make require_auth error message similar to surrounding messages v1-0001-Make-require_auth-error-message-similar-to-surrou.patch
v1-0001-Make-require_auth-error-message-similar-to-surrou.patch
Best regards,
Gurjeet http://Gurje.et
Postgres Contributors Team, http://aws.amazon.com
Attachments:
v1-0001-Replace-placeholders-with-known-strings.patchapplication/octet-stream; name=v1-0001-Replace-placeholders-with-known-strings.patchDownload
From 923c415f071b0e9e5ef0763da415a5db4d3aab35 Mon Sep 17 00:00:00 2001
From: Gurjeet Singh <gurjeet@singh.im>
Date: Thu, 20 Apr 2023 19:08:59 -0700
Subject: [PATCH v1] Replace placeholders with known strings
In pq_fe_connect.c, fix peculiar pattern of using %s for a known
string. If the string is known, we use it in the format string, instead
of using the format code %s and then providing the known string as a
parameter.
---
src/interfaces/libpq/fe-connect.c | 38 ++++++++++++++-----------------
1 file changed, 17 insertions(+), 21 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index fcd3d0d9a3..518a33c1b6 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1458,8 +1458,8 @@ connectOptions2(PGconn *conn)
&& strcmp(conn->channel_binding, "require") != 0)
{
conn->status = CONNECTION_BAD;
- libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
- "channel_binding", conn->channel_binding);
+ libpq_append_conn_error(conn, "invalid channel_binding value: \"%s\"",
+ conn->channel_binding);
return false;
}
}
@@ -1500,8 +1500,8 @@ connectOptions2(PGconn *conn)
&& strcmp(conn->sslmode, "verify-full") != 0)
{
conn->status = CONNECTION_BAD;
- libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
- "sslmode", conn->sslmode);
+ libpq_append_conn_error(conn, "invalid sslmode value: \"%s\"",
+ conn->sslmode);
return false;
}
@@ -1520,8 +1520,8 @@ connectOptions2(PGconn *conn)
case 'r': /* "require" */
case 'v': /* "verify-ca" or "verify-full" */
conn->status = CONNECTION_BAD;
- libpq_append_conn_error(conn, "%s value \"%s\" invalid when SSL support is not compiled in",
- "sslmode", conn->sslmode);
+ libpq_append_conn_error(conn, "sslmode value \"%s\" invalid when SSL support is not compiled in",
+ conn->sslmode);
return false;
}
#endif
@@ -1556,16 +1556,14 @@ connectOptions2(PGconn *conn)
if (!sslVerifyProtocolVersion(conn->ssl_min_protocol_version))
{
conn->status = CONNECTION_BAD;
- libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
- "ssl_min_protocol_version",
+ libpq_append_conn_error(conn, "invalid ssl_min_protocol_version value: \"%s\"",
conn->ssl_min_protocol_version);
return false;
}
if (!sslVerifyProtocolVersion(conn->ssl_max_protocol_version))
{
conn->status = CONNECTION_BAD;
- libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
- "ssl_max_protocol_version",
+ libpq_append_conn_error(conn, "invalid ssl_max_protocol_version value: \"%s\"",
conn->ssl_max_protocol_version);
return false;
}
@@ -1595,16 +1593,16 @@ connectOptions2(PGconn *conn)
strcmp(conn->sslcertmode, "require") != 0)
{
conn->status = CONNECTION_BAD;
- libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
- "sslcertmode", conn->sslcertmode);
+ libpq_append_conn_error(conn, "invalid sslcertmode value: \"%s\"",
+ conn->sslcertmode);
return false;
}
#ifndef USE_SSL
if (strcmp(conn->sslcertmode, "require") == 0)
{
conn->status = CONNECTION_BAD;
- libpq_append_conn_error(conn, "%s value \"%s\" invalid when SSL support is not compiled in",
- "sslcertmode", conn->sslcertmode);
+ libpq_append_conn_error(conn, "sslcertmode value \"%s\" invalid when SSL support is not compiled in",
+ conn->sslcertmode);
return false;
}
#endif
@@ -1618,8 +1616,8 @@ connectOptions2(PGconn *conn)
if (strcmp(conn->sslcertmode, "require") == 0)
{
conn->status = CONNECTION_BAD;
- libpq_append_conn_error(conn, "%s value \"%s\" is not supported (check OpenSSL version)",
- "sslcertmode", conn->sslcertmode);
+ libpq_append_conn_error(conn, "sslcertmode value \"%s\" is not supported (check OpenSSL version)",
+ conn->sslcertmode);
return false;
}
#endif
@@ -1641,7 +1639,7 @@ connectOptions2(PGconn *conn)
strcmp(conn->gssencmode, "require") != 0)
{
conn->status = CONNECTION_BAD;
- libpq_append_conn_error(conn, "invalid %s value: \"%s\"", "gssencmode", conn->gssencmode);
+ libpq_append_conn_error(conn, "invalid gssencmode value: \"%s\"", conn->gssencmode);
return false;
}
#ifndef ENABLE_GSS
@@ -1681,8 +1679,7 @@ connectOptions2(PGconn *conn)
else
{
conn->status = CONNECTION_BAD;
- libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
- "target_session_attrs",
+ libpq_append_conn_error(conn, "invalid target_session_attrs value: \"%s\"",
conn->target_session_attrs);
return false;
}
@@ -1702,8 +1699,7 @@ connectOptions2(PGconn *conn)
else
{
conn->status = CONNECTION_BAD;
- libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
- "load_balance_hosts",
+ libpq_append_conn_error(conn, "invalid load_balance_hosts value: \"%s\"",
conn->load_balance_hosts);
return false;
}
--
2.35.1
v1-0001-Make-require_auth-error-message-similar-to-surrou.patchapplication/octet-stream; name=v1-0001-Make-require_auth-error-message-similar-to-surrou.patchDownload
From 7aeddb768e3b29595569976a6dc03b1b0c71c66f Mon Sep 17 00:00:00 2001
From: Gurjeet Singh <gurjeet@singh.im>
Date: Thu, 20 Apr 2023 19:17:29 -0700
Subject: [PATCH v1] Make require_auth error message similar to surrounding
messages
This error message string was not consistent with the similar error
messages in the neighboring code. Hence changed to make the code
uniform.
---
src/interfaces/libpq/fe-connect.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index fcd3d0d9a3..75dee687b0 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1407,8 +1407,8 @@ connectOptions2(PGconn *conn)
else
{
conn->status = CONNECTION_BAD;
- libpq_append_conn_error(conn, "invalid require_auth method: \"%s\"",
- method);
+ libpq_append_conn_error(conn, "invalid %s method: \"%s\"",
+ "require_auth", method);
free(part);
return false;
--
2.35.1
Gurjeet Singh <gurjeet@singh.im> writes:
When reviewing a recently committed patch [1] I noticed the odd usage
of a format specifier:
+ libpq_append_conn_error(conn, "invalid %s value: \"%s\"", + "load_balance_hosts", + conn->load_balance_hosts);
The oddity is that the first %s is unnecessary, since the value we
want there is a constant. Typically a format specifier used to get the
value stored in a variable.
This is actually intentional, on the grounds that it reduces the
number of format strings that require translation.
There's just one exception to this pattern, though.
libpq_append_conn_error(conn, "invalid require_auth method: \"%s\"",
method);
Yup, this one did not get the memo.
regards, tom lane
On Thu, Apr 20, 2023 at 9:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Gurjeet Singh <gurjeet@singh.im> writes:
When reviewing a recently committed patch [1] I noticed the odd usage
of a format specifier:+ libpq_append_conn_error(conn, "invalid %s value: \"%s\"", + "load_balance_hosts", + conn->load_balance_hosts);The oddity is that the first %s is unnecessary, since the value we
want there is a constant. Typically a format specifier used to get the
value stored in a variable.This is actually intentional, on the grounds that it reduces the
number of format strings that require translation.
That's the only reason I too could come up with.
There's just one exception to this pattern, though.
libpq_append_conn_error(conn, "invalid require_auth method: \"%s\"",
method);Yup, this one did not get the memo.
That explains why I could not find any translation for this error message.
Best regards,
Gurjeet http://Gurje.et
Postgres Contributors Team, http://aws.amazon.com
On 21 Apr 2023, at 07:02, Gurjeet Singh <gurjeet@singh.im> wrote:
On Thu, Apr 20, 2023 at 9:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
libpq_append_conn_error(conn, "invalid require_auth method: \"%s\"",
method);Yup, this one did not get the memo.
I've pushed this, with the change to use the common "invalid %s value" format
that we use for all other libpq options. This makes this string make use of
already existing translations and makes error reporting consistent.
That explains why I could not find any translation for this error message.
The feature is new in master so any translations for it are yet to be merged
from the translation repo.
--
Daniel Gustafsson