On Tue, Oct 18, 2022 at 8:06 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi, here are my review comments for patch v38-0001.
>
> 3.
>
> + /* Ensure we are reading the data into our memory context. */
> + oldctx = MemoryContextSwitchTo(ApplyMessageContext);
>
> Doesn't something need to switch back to this 'oldctx' prior to
> breaking out of the for(;;) loop?
>
> ~~~
>
> 4.
>
> + apply_dispatch(&s);
> +
> + MemoryContextReset(ApplyMessageContext);
>
> Isn't this broken now? Since you've removed the
> MemoryContextSwitchTo(oldctx), so next iteration will switch to
> ApplyMessageContext again which will overwrite and lose knowledge of
> the original 'oldctx' (??)
>
> ~~
>
> 5.
>
> Maybe this is a silly idea, I'm not sure. Because this is an infinite
> loop, then instead of the multiple calls to
> MemoryContextReset(ApplyMessageContext) maybe there can be just a
> single call to it immediately before you switch to that context in the
> first place. The effect will be the same, won't it?
>
I think so but I think it will look a bit odd, especially for the
first time. If the purpose is to just do it once, won't it be better
to do it at the end of for loop?
>
> 9. apply_handle_stream_start
>
> + *
> + * XXX We can avoid sending pairs of the START/STOP messages to the parallel
> + * worker because unlike apply worker it will process only one transaction at a
> + * time. However, it is not clear whether that is worth the effort because it
> + * is sent after logical_decoding_work_mem changes.
> */
> static void
> apply_handle_stream_start(StringInfo s)
>
> As previously mentioned ([1] #13b) it's not obvious to me what that
> last sentence means. e.g. "because it is sent" - what is "it"?
>
Here, it refers to START/STOP messages, so I think we should say "...
because these messages are sent .." instead of "... because it is sent
...". Does that makes sense to you?
--
With Regards,
Amit Kapila.
