On Thu, Aug 12, 2021 1:53 PM Masahiko Sawada <sawada.m...@gmail.com> wrote:
> I've attached the updated patches. FYI I've included the patch
> (v8-0005) that fixes the assertion failure during shared fileset cleanup to
> make
> cfbot tests happy.
Hi,
Thanks for the new patches.
I have a few comments on the v8-0001 patch.
1)
+
+ if (TransactionIdIsNormal(errarg->remote_xid))
+ appendStringInfo(&buf, _(" in transaction id %u with commit
timestamp %s"),
+ errarg->remote_xid,
+ errarg->commit_ts == 0
+ ? "(unset)"
+ :
timestamptz_to_str(errarg->commit_ts));
+
+ errcontext("%s", buf.data);
I think we can output the timestamp in a separete check which can be more
consistent with the other code style in apply_error_callback()
(ie)
+ if (errarg->commit_ts != 0)
+ appendStringInfo(&buf, _(" with commit timestamp %s"),
+
timestamptz_to_str(errarg->commit_ts));
2)
+/*
+ * Get string representing LogicalRepMsgType.
+ */
+char *
+logicalrep_message_type(LogicalRepMsgType action)
+{
...
+
+ elog(ERROR, "invalid logical replication message type \"%c\"", action);
+}
Some old compilers might complain that the function doesn't have a return value
at the end of the function, maybe we can code like the following:
+char *
+logicalrep_message_type(LogicalRepMsgType action)
+{
+ switch (action)
+ {
+ case LOGICAL_REP_MSG_BEGIN:
+ return "BEGIN";
...
+ default:
+ elog(ERROR, "invalid logical replication message type
\"%c\"", action);
+ }
+ return NULL; /* keep compiler quiet */
+}
3)
Do we need to invoke set_apply_error_context_xact() in the function
apply_handle_stream_prepare() to save the xid and timestamp ?
Best regards,
Hou zj
