Horton

Thread

Commits

  1. Improve style of some replication-related error messages.

  1. Redundant errdetail prefix "The error was:" in some logical replication messages

    Peter Smith <smithpb2250@gmail.com> — 2021-03-30T04:29:35Z

    There are a couple of error messages within the logical replication
code where the errdetail text includes a prefix of "The error was:"

I could not find any other examples in all the PG src which have a
similar errdetail prefix like this.

The text seems not only redundant, but "The error was: ERROR: " also
looks a bit peculiar.

e.g.
------
2021-03-30 14:17:37.567 AEDT [22317] ERROR:  could not drop the
replication slot "tap_sub" on publisher
2021-03-30 14:17:37.567 AEDT [22317] DETAIL:  The error was: ERROR:
replication slot "tap_sub" does not exist
2021-03-30 14:17:37.567 AEDT [22317] STATEMENT:  DROP SUBSCRIPTION tap_sub;
------

PSA a small patch which simply removes this prefix "The error was:"

------
Kind Regards,
Peter Smith.
Fujitsu Australia

  2. Re: Redundant errdetail prefix "The error was:" in some logical replication messages

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-30T06:10:34Z

    Peter Smith <smithpb2250@gmail.com> writes:
> There are a couple of error messages within the logical replication
> code where the errdetail text includes a prefix of "The error was:"

Hmm, isn't project style more usually to include the error reason
in the primary message?  That is,

             ereport(LOG,
-                    (errmsg("could not drop the replication slot \"%s\" on publisher",
-                            slotname),
-                     errdetail("The error was: %s", res->err)));
+                    (errmsg("could not drop the replication slot \"%s\" on publisher: %s",
+                            slotname, res->err)));

and so on.  If we had reason to think that res->err would be extremely
long, maybe pushing it to errdetail would be sensible, but I'm not
seeing that that is likely.

(I think the "the" before "replication slot" could go away, too.)

			regards, tom lane

  3. Re: Redundant errdetail prefix "The error was:" in some logical replication messages

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-30T06:21:01Z

    On Tue, Mar 30, 2021 at 11:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Smith <smithpb2250@gmail.com> writes:
> > There are a couple of error messages within the logical replication
> > code where the errdetail text includes a prefix of "The error was:"
>
> Hmm, isn't project style more usually to include the error reason
> in the primary message?  That is,
>
>              ereport(LOG,
> -                    (errmsg("could not drop the replication slot \"%s\" on publisher",
> -                            slotname),
> -                     errdetail("The error was: %s", res->err)));
> +                    (errmsg("could not drop the replication slot \"%s\" on publisher: %s",
> +                            slotname, res->err)));
>
> and so on.  If we had reason to think that res->err would be extremely
> long, maybe pushing it to errdetail would be sensible, but I'm not
> seeing that that is likely.
>
> (I think the "the" before "replication slot" could go away, too.)

+1 to have the res->err in the primary message itself and get rid of
errdetail. Currently the error "could not fetch table info for table"
does that.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  4. Re: Redundant errdetail prefix "The error was:" in some logical replication messages

    Peter Smith <smithpb2250@gmail.com> — 2021-03-30T08:30:02Z

    On Tue, Mar 30, 2021 at 5:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Smith <smithpb2250@gmail.com> writes:
> > There are a couple of error messages within the logical replication
> > code where the errdetail text includes a prefix of "The error was:"
>
> Hmm, isn't project style more usually to include the error reason
> in the primary message?  That is,
>
>              ereport(LOG,
> -                    (errmsg("could not drop the replication slot \"%s\" on publisher",
> -                            slotname),
> -                     errdetail("The error was: %s", res->err)));
> +                    (errmsg("could not drop the replication slot \"%s\" on publisher: %s",
> +                            slotname, res->err)));
>
> and so on.  If we had reason to think that res->err would be extremely
> long, maybe pushing it to errdetail would be sensible, but I'm not
> seeing that that is likely.
>
> (I think the "the" before "replication slot" could go away, too.)

Thanks for the review and advice.

PSA version 2 of this patch which adopts your suggestions.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

  5. Re: Redundant errdetail prefix "The error was:" in some logical replication messages

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-31T19:30:43Z

    Peter Smith <smithpb2250@gmail.com> writes:
> PSA version 2 of this patch which adopts your suggestions.

LGTM, pushed.

			regards, tom lane