On Mon, Jul 3, 2023 at 6:52 PM Ashutosh Bapat
<ashutosh.bapat....@gmail.com> wrote:
>
> Thanks Euler for the patch.
>
> On Mon, Jul 3, 2023 at 6:32 PM Euler Taveira <eu...@eulerto.com> wrote:
> >
> > Masahiko, since abc0910e2e0 is your patch maybe you want to take a look at
> > it.
> >
>
> A couple of comments.
>
> -char *
> +const char *
>
> Nice improvement.
>
> logicalrep_message_type(LogicalRepMsgType action)
> {
> switch (action)
> @@ -1256,9 +1256,7 @@ logicalrep_message_type(LogicalRepMsgType action)
> return "STREAM ABORT";
> case LOGICAL_REP_MSG_STREAM_PREPARE:
> return "STREAM PREPARE";
> + default:
> + return "???";
> }
> -
> - elog(ERROR, "invalid logical replication message type \"%c\"", action);
> -
> - return NULL; /* keep compiler quiet */
>
> The switch is on action which is an enum. So without default it will
> provide a compilation warning for missing enums. Adding "default" case
> defeats that purpose. I think we should just return "???" from outside
> switch block.
>
PFA patch.
--
Best Wishes,
Ashutosh Bapat
From e117a1ea78327ebcb85d55fedb3b5e7e3b5c7b27 Mon Sep 17 00:00:00 2001
From: Euler Taveira <euler.tave...@enterprisedb.com>
Date: Mon, 3 Jul 2023 08:54:10 -0300
Subject: [PATCH] uncover logical change details
The commit abc0910e2e0 adds logical change details to error context.
However, the function logicalrep_message_type() introduces an
elog(ERROR) that can hide these details. Instead, avoid elog() and use
??? (that is a synonym for unknown). Spotted by Ashutosh Bapat.
Discussion: https://postgr.es/m/CAExHW5suAEDW-mBZt_qu4RVxWZ1vL54-L%2Bci2zreYWebpzxYsA%40mail.gmail.com
---
src/backend/replication/logical/proto.c | 11 +++++++----
src/include/replication/logicalproto.h | 2 +-
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/backend/replication/logical/proto.c b/src/backend/replication/logical/proto.c
index f308713275..fe6ed9f94b 100644
--- a/src/backend/replication/logical/proto.c
+++ b/src/backend/replication/logical/proto.c
@@ -1213,7 +1213,7 @@ logicalrep_read_stream_abort(StringInfo in,
/*
* Get string representing LogicalRepMsgType.
*/
-char *
+const char *
logicalrep_message_type(LogicalRepMsgType action)
{
switch (action)
@@ -1258,7 +1258,10 @@ logicalrep_message_type(LogicalRepMsgType action)
return "STREAM PREPARE";
}
- elog(ERROR, "invalid logical replication message type \"%c\"", action);
-
- return NULL; /* keep compiler quiet */
+ /*
+ * This function is called to provide context in the error raised when
+ * applying a logical message. So we can't throw an error here. Return an
+ * unknown indicator value so that the original error is still reported.
+ */
+ return "???";
}
diff --git a/src/include/replication/logicalproto.h b/src/include/replication/logicalproto.h
index 0ea2df5088..c5be981eae 100644
--- a/src/include/replication/logicalproto.h
+++ b/src/include/replication/logicalproto.h
@@ -269,6 +269,6 @@ extern void logicalrep_write_stream_abort(StringInfo out, TransactionId xid,
extern void logicalrep_read_stream_abort(StringInfo in,
LogicalRepStreamAbortData *abort_data,
bool read_abort_info);
-extern char *logicalrep_message_type(LogicalRepMsgType action);
+extern const char *logicalrep_message_type(LogicalRepMsgType action);
#endif /* LOGICAL_PROTO_H */
--
2.25.1
