Re: Improve the connection failure error messages - Mailing list pgsql-hackers

Thanks for the patch! Here are a couple of review comments for it.

======
src/backend/commands/subscriptioncmds.c

1.
@@ -742,7 +742,7 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
  if (!wrconn)
  ereport(ERROR,
  (errcode(ERRCODE_CONNECTION_FAILURE),
- errmsg("could not connect to the publisher: %s", err)));
+ errmsg("\"%s\" could not connect to the publisher: %s", stmt->subname, err)));

In practice, these commands give errors like:

test_sub=# create subscription sub1 connection 'dbname=bogus' publication pub1;
ERROR:  could not connect to the publisher: connection to server on
socket "/tmp/.s.PGSQL.5432" failed: FATAL:  database "bogus" does not
exist

and logs like:

2024-01-12 12:45:05.177 AEDT [13108] ERROR:  could not connect to the
publisher: connection to server on socket "/tmp/.s.PGSQL.5432" failed:
FATAL:  database "bogus" does not exist
2024-01-12 12:45:05.177 AEDT [13108] STATEMENT:  create subscription
sub1 connection 'dbname=bogus' publication pub1;

Since the subscription name is already obvious from the statement that
caused the error I'm not sure it benefits much to add this to the
error message (but maybe it is useful if the error message was somehow
read in isolation from the statement).

Anyway, I felt at least it should include the word "subscription" for
better consistency with the other messages in this patch:

SUGGESTION
subscription \"%s\" could not connect to the publisher: %s

======

2.
+ appname = cluster_name[0] ? cluster_name : "walreceiver";
+
  /* Establish the connection to the primary for XLOG streaming */
- wrconn = walrcv_connect(conninfo, false, false,
- cluster_name[0] ? cluster_name : "walreceiver",
- &err);
+ wrconn = walrcv_connect(conninfo, false, false, appname, &err);
  if (!wrconn)
  ereport(ERROR,
  (errcode(ERRCODE_CONNECTION_FAILURE),
- errmsg("could not connect to the primary server: %s", err)));
+ errmsg("%s could not connect to the primary server: %s", appname, err)));

I think your new %s should be quoted according to the guidelines at [1].

======
src/test/regress/expected/subscription.out

3.
Apparently, there is no existing regression test case for the ALTER
"could not connect" message because if there was, it would have
failed. Maybe a test should be added?

======
[1] https://www.postgresql.org/docs/current/error-style-guide.html

Kind Regards,
Peter Smith.
Fujitsu Australia