On Fri, Nov 27, 2020 at 7:29 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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.
>
Yeah, I had intentionally left it because, in the same function, few
lines before we have that condition and comment, so adding it again
didn't appear to be a good idea to me.
> ===
>
> 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.
>
Cool, I'm planning to push this in a few minutes time.
--
With Regards,
Amit Kapila.