Thread: Make message strings in fe-connect.c consistent

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.

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] does the former, and patch [3] does the
latter.

Pick your favorite one.

[1]: Support connection load balancing in libpq
7f5b19817eaf38e70ad1153db4e644ee9456853e

[2]: Replace placeholders with known strings
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

Best regards,
Gurjeet http://Gurje.et
Postgres Contributors Team, http://aws.amazon.com

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