Re: [PATCH] pg_hba.conf error messages for logical replication connections

On Thu, Jan 28, 2021 at 8:17 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> What exactly are you bothered about here? Is the database name not
> present in the message your concern or the message uses 'replication'
> but actually it doesn't relate to 'replication' specified in
> pg_hba.conf your concern? I think with the current scheme one might
> say that replication word in error message helps them to distinguish
> logical replication connection error from a regular connection error.
> I am not telling what you are proposing is wrong but just that the
> current scheme of things might be helpful to some users. If you can
> explain a bit how the current message misled you and the proposed one
> solves that confusion that would be better?
>

My main confusion arose from conflating the word "replication" in the
error message with the "replication" keyword in pg_hba.conf.

In my case, I was actually confused because I was creating logical
replication connections that weren't getting rejected, despite a lack
of any "replication" rules in my pg_hba.conf. I had the faulty
assumption that replication connection requires "replication" keyword,
and my change to the documentation makes it explicit that logical
replication connections do not match the "replication" keyword.

I was digging through the code trying to understand why it was working,
and also making manual connections before I stumbled upon these error
messages.

The fact that the error message doesn't include the database name
definitely contributed to my confusion. It only mentions the word
"replication", and not a database name, and that reinforces the idea
that the "replication" keyword rule should apply, because it seems
"replication" has replaced the database name.

But overall, I would agree that the current messages aren't wrong,
and my fix could still cause confusion because now logical replication
connections won't be described as "replication" connections.

How about explicitly specifying physical vs. logical replication in the
error message, and also adding hints for clarifying the use of
the "replication" keyword in pg_hba.conf?

if physical replication
Error "pg_hba.conf rejects physical replication connection ..."
Hint "Physical replication connections only match pg_hba.conf rules
using the "replication" keyword"
else if logical replication
Error "pg_hba.conf rejects logical replication connection to database %s ..."
// Maybe add this?
Hint "Logical replication connections do not match pg_hba.conf rules
using the "replication" keyword"
else
Error "pg_hba.conf rejects connection to database %s ..."

If I did go with this approach, would it be better to have three
separate branches, or to just introduce another variable for the
connection type? I'm not sure what is optimal for translation. (If both
types of replication connections get hints then probably three branches
is better.)

const char *connection_type;

connection_type =
am_db_walsender ? _("logical replication connection") :
am_walsender ? _("physical replication connection") :
_("connection")

- Paul