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
From 209030c136f7fed139d626506ae2a151ea318fdc Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Tue, 30 Mar 2021 15:01:31 +1100
Subject: [PATCH v1] Remove redundant errdetail "The error was".
Removed this redundant text because:
1. The extra text does not add anything useful to the DETAIL of the message.
2. It looks strange: "The error was: ERROR ...".
3. There seems no other errdetail examples in all of PG source which are worded like this.
---
src/backend/commands/subscriptioncmds.c | 6 +++---
src/backend/replication/logical/tablesync.c | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 89abc81..adfc7fd 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1429,7 +1429,7 @@ ReplicationSlotDropAtPubNode(WalReceiverConn *wrconn, char *slotname, bool missi
ereport(LOG,
(errmsg("could not drop the replication slot \"%s\" on publisher",
slotname),
- errdetail("The error was: %s", res->err)));
+ errdetail("%s", res->err)));
}
else
{
@@ -1437,7 +1437,7 @@ ReplicationSlotDropAtPubNode(WalReceiverConn *wrconn, char *slotname, bool missi
ereport(ERROR,
(errmsg("could not drop the replication slot \"%s\" on publisher",
slotname),
- errdetail("The error was: %s", res->err)));
+ errdetail("%s", res->err)));
}
walrcv_clear_result(res);
@@ -1653,7 +1653,7 @@ ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err)
ereport(ERROR,
(errmsg("could not connect to publisher when attempting to "
"drop the replication slot \"%s\"", slotname),
- errdetail("The error was: %s", err),
+ errdetail("%s", err),
/* translator: %s is an SQL ALTER command */
errhint("Use %s to disassociate the subscription from the slot.",
"ALTER SUBSCRIPTION ... SET (slot_name = NONE)")));
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 2937f41..3ff433d 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1040,7 +1040,7 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
if (res->status != WALRCV_OK_COMMAND)
ereport(ERROR,
(errmsg("table copy could not start transaction on publisher"),
- errdetail("The error was: %s", res->err)));
+ errdetail("%s", res->err)));
walrcv_clear_result(res);
/*
@@ -1100,7 +1100,7 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
if (res->status != WALRCV_OK_COMMAND)
ereport(ERROR,
(errmsg("table copy could not finish transaction on publisher"),
- errdetail("The error was: %s", res->err)));
+ errdetail("%s", res->err)));
walrcv_clear_result(res);
table_close(rel, NoLock);
--
1.8.3.1
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
Attachments:
v2-0001-Remove-redundant-errdetail-The-error-was.patchapplication/octet-stream; name=v2-0001-Remove-redundant-errdetail-The-error-was.patchDownload
From d644901f62034d01da9f680427ef3bc3c47b5a6e Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Tue, 30 Mar 2021 19:20:24 +1100
Subject: [PATCH v2] Remove redundant errdetail "The error was".
Also the errdetail part of the message is now included as part of the
primary message to make it consistent with the usual project style for errors.
---
src/backend/commands/subscriptioncmds.c | 13 +++++--------
src/backend/replication/logical/tablesync.c | 8 ++++----
2 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index bfd3514..5282b79 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1320,17 +1320,15 @@ ReplicationSlotDropAtPubNode(WalReceiverConn *wrconn, char *slotname, bool missi
{
/* LOG. Error, but missing_ok = true. */
ereport(LOG,
- (errmsg("could not drop the replication slot \"%s\" on publisher",
- slotname),
- errdetail("The error was: %s", res->err)));
+ (errmsg("could not drop replication slot \"%s\" on publisher: %s",
+ slotname, res->err)));
}
else
{
/* ERROR. */
ereport(ERROR,
- (errmsg("could not drop the replication slot \"%s\" on publisher",
- slotname),
- errdetail("The error was: %s", res->err)));
+ (errmsg("could not drop replication slot \"%s\" on publisher: %s",
+ slotname, res->err)));
}
walrcv_clear_result(res);
@@ -1545,8 +1543,7 @@ ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err)
ereport(ERROR,
(errmsg("could not connect to publisher when attempting to "
- "drop the replication slot \"%s\"", slotname),
- errdetail("The error was: %s", err),
+ "drop replication slot \"%s\": %s", slotname, err),
/* translator: %s is an SQL ALTER command */
errhint("Use %s to disassociate the subscription from the slot.",
"ALTER SUBSCRIPTION ... SET (slot_name = NONE)")));
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 8494db8..0638f5c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1043,8 +1043,8 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
0, NULL);
if (res->status != WALRCV_OK_COMMAND)
ereport(ERROR,
- (errmsg("table copy could not start transaction on publisher"),
- errdetail("The error was: %s", res->err)));
+ (errmsg("table copy could not start transaction on publisher: %s",
+ res->err)));
walrcv_clear_result(res);
/*
@@ -1103,8 +1103,8 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
res = walrcv_exec(wrconn, "COMMIT", 0, NULL);
if (res->status != WALRCV_OK_COMMAND)
ereport(ERROR,
- (errmsg("table copy could not finish transaction on publisher"),
- errdetail("The error was: %s", res->err)));
+ (errmsg("table copy could not finish transaction on publisher: %s",
+ res->err)));
walrcv_clear_result(res);
table_close(rel, NoLock);
--
1.8.3.1