RE: Skipping logical replication transactions on subscriber side

On Thu, Aug 12, 2021 1:53 PM Masahiko Sawada <sawada.mshk@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