RE: Skipping logical replication transactions on subscriber side

On Thu, Aug 12, 2021 1:53 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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