Re: Perform streaming logical transactions by background workers and parallel apply

On Thu, Dec 15, 2022 at 8:58 AM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>

Few minor comments:
=================
1.
+ for (i = list_length(subxactlist) - 1; i >= 0; i--)
+ {
+ TransactionId xid_tmp = lfirst_xid(list_nth_cell(subxactlist, i));
+
+ if (xid_tmp == subxid)
+ {
+ RollbackToSavepoint(spname);
+ CommitTransactionCommand();
+ subxactlist = list_truncate(subxactlist, i + 1);

I find that there is always one element extra in the list after
rollback to savepoint. Don't we need to truncate the list to 'i' as
shown in the diff below?

2.
* Note that If it's an empty sub-transaction then we will not find
* the subxid here.

If in above comment seems to be in wrong case. Anyway, I have slightly
modified it as you can see in the diff below.

$ git diff
diff --git a/src/backend/replication/logical/applyparallelworker.c
b/src/backend/replication/logical/applyparallelworker.c
index 11695c75fa..c809b1fd01 100644
--- a/src/backend/replication/logical/applyparallelworker.c
+++ b/src/backend/replication/logical/applyparallelworker.c
@@ -1516,8 +1516,8 @@ pa_stream_abort(LogicalRepStreamAbortData *abort_data)
* Search the subxactlist, determine the offset tracked for the
* subxact, and truncate the list.
*
- * Note that If it's an empty sub-transaction then we
will not find
- * the subxid here.
+ * Note that for an empty sub-transaction we won't
find the subxid
+ * here.
*/
for (i = list_length(subxactlist) - 1; i >= 0; i--)
{
@@ -1527,7 +1527,7 @@ pa_stream_abort(LogicalRepStreamAbortData *abort_data)
{
RollbackToSavepoint(spname);
CommitTransactionCommand();
- subxactlist = list_truncate(subxactlist, i + 1);
+ subxactlist = list_truncate(subxactlist, i);
break;
}
}

--
With Regards,
Amit Kapila.