On Tue, Oct 18, 2022 at 8:06 AM Peter Smith <smithpb2...@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.
