Redundant errdetail prefix "The error was:" in some logical replication messages
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
Attachments:
v1-0001-Remove-redundant-errdetail-The-error-was.patchapplication/octet-stream; name=v1-0001-Remove-redundant-errdetail-The-error-was.patchDownload+5-6
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
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
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