On Thu, Nov 26, 2020 at 11:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Okay, I see your point, so changed it accordingly and fixed the code
> comments as suggested by you. Does this address all your
> concerns/suggestions?
Yes. Now the v4 patch addresses everything I previously mentioned.
However, today I did notice one more small thing you may want to change.
===
REVIEW COMMENT:
(1) missing comment?
@@ -928,7 +917,9 @@ apply_handle_stream_abort(StringInfo s)
/* write the updated subxact list */
subxact_info_write(MyLogicalRepWorker->subid, xid);
- CommitTransactionCommand();
+
+ if (!am_tablesync_worker())
+ CommitTransactionCommand();
}
}
That new condition seems to be missing a comment saying "/* The
synchronization worker runs in a single transaction */". Such a
comment accompanies all other am_tablesync_worker() conditions that
look like this one.
===
I also re-ran the tablesync test using the v4 patch, and have stepped
to see the normal stream file normal cleanup being executed by the
tablesync worker.
So apart from that trivial missing comment, I think now it is all good.
---
Kind Regards,
Peter Smith.
Fujitsu Australia