Horton

Thread

Commits

  1. Improve SQLSTATE reporting in some replication-related code.

  1. SQLSTATE for replication connection failures

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-12T15:42:02Z

    So far as I can find, just about everyplace that deals with replication
connections has slipshod error reporting.  An example from worker.c is

        LogRepWorkerWalRcvConn = walrcv_connect(MySubscription->conninfo, true,
                                                MySubscription->name, &err);
        if (LogRepWorkerWalRcvConn == NULL)
            ereport(ERROR,
                    (errmsg("could not connect to the publisher: %s", err)));

Because of the lack of any errcode() call, this failure will be reported
as XX000 ERRCODE_INTERNAL_ERROR, which is surely not appropriate.
worker.c is in good company though, because EVERY caller of walrcv_connect
is equally slipshod.

Shall we just use ERRCODE_CONNECTION_FAILURE for these failures, or
would it be better to invent another SQLSTATE code?  Arguably,
ERRCODE_CONNECTION_FAILURE is meant for failures of client connections;
but on the other hand, a replication connection is a sort of client.

			regards, tom lane

  2. Re: SQLSTATE for replication connection failures

    Amit Kapila <amit.kapila16@gmail.com> — 2021-06-14T09:17:44Z

    On Sat, Jun 12, 2021 at 9:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> So far as I can find, just about everyplace that deals with replication
> connections has slipshod error reporting.  An example from worker.c is
>
>         LogRepWorkerWalRcvConn = walrcv_connect(MySubscription->conninfo, true,
>                                                 MySubscription->name, &err);
>         if (LogRepWorkerWalRcvConn == NULL)
>             ereport(ERROR,
>                     (errmsg("could not connect to the publisher: %s", err)));
>
> Because of the lack of any errcode() call, this failure will be reported
> as XX000 ERRCODE_INTERNAL_ERROR, which is surely not appropriate.
> worker.c is in good company though, because EVERY caller of walrcv_connect
> is equally slipshod.
>
> Shall we just use ERRCODE_CONNECTION_FAILURE for these failures, or
> would it be better to invent another SQLSTATE code?  Arguably,
> ERRCODE_CONNECTION_FAILURE is meant for failures of client connections;
> but on the other hand, a replication connection is a sort of client.
>

Your reasoning sounds good to me. So, +1 for using ERRCODE_CONNECTION_FAILURE.

-- 
With Regards,
Amit Kapila.

  3. Re: SQLSTATE for replication connection failures

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-06-16T06:49:23Z

    On Mon, Jun 14, 2021 at 6:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Jun 12, 2021 at 9:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > So far as I can find, just about everyplace that deals with replication
> > connections has slipshod error reporting.  An example from worker.c is
> >
> >         LogRepWorkerWalRcvConn = walrcv_connect(MySubscription->conninfo, true,
> >                                                 MySubscription->name, &err);
> >         if (LogRepWorkerWalRcvConn == NULL)
> >             ereport(ERROR,
> >                     (errmsg("could not connect to the publisher: %s", err)));
> >
> > Because of the lack of any errcode() call, this failure will be reported
> > as XX000 ERRCODE_INTERNAL_ERROR, which is surely not appropriate.
> > worker.c is in good company though, because EVERY caller of walrcv_connect
> > is equally slipshod.
> >
> > Shall we just use ERRCODE_CONNECTION_FAILURE for these failures, or
> > would it be better to invent another SQLSTATE code?  Arguably,
> > ERRCODE_CONNECTION_FAILURE is meant for failures of client connections;
> > but on the other hand, a replication connection is a sort of client.
> >
>
> Your reasoning sounds good to me. So, +1 for using ERRCODE_CONNECTION_FAILURE.

+1

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  4. Re: SQLSTATE for replication connection failures

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-16T15:53:50Z

    Masahiko Sawada <sawada.mshk@gmail.com> writes:
> On Mon, Jun 14, 2021 at 6:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>> Shall we just use ERRCODE_CONNECTION_FAILURE for these failures, or
>>> would it be better to invent another SQLSTATE code?  Arguably,
>>> ERRCODE_CONNECTION_FAILURE is meant for failures of client connections;
>>> but on the other hand, a replication connection is a sort of client.

>> Your reasoning sounds good to me. So, +1 for using ERRCODE_CONNECTION_FAILURE.

> +1

Done that way.  I also fixed some nearby ereports that were missing
errcodes; some of them seemed more like PROTOCOL_VIOLATIONs than
CONNECTION_FAILUREs, though.

			regards, tom lane