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

On Sat, Jan 30, 2021 at 12:24 AM Paul Martinez <paulmtz(at)google(dot)com> wrote:
>
> 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 think it is good to be more explicit in the documentation but we
already mention "physical replication connection" in the sentence. So
it might be better that we add a separate sentence related to logical
replication.

> 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?
>

Yeah, hints or more details might improve the situation but I am not
sure we want to add more branching here. Can we write something
similar to HOSTNAME_LOOKUP_DETAIL for hints? Also, I think what you
are proposing to write is more of a errdetail kind of message. See
more error routines in the docs [1].

[1] - https://www.postgresql.org/docs/devel/error-message-reporting.html

--
With Regards,
Amit Kapila.