On Thu, Apr 23, 2026 at 12:00 PM Peter Smith <[email protected]> wrote: > > On Wed, Apr 22, 2026 at 7:23 PM shveta malik <[email protected]> wrote: > > > ... > > Regarding 0001, I did not understand the need of having 2 separate messages: > > > > +#define PARALLEL_APPLY_INTERNAL_MESSAGE 'i' > > + LOGICAL_REP_MSG_INTERNAL_MESSAGE = 'i', > > > > And the need of sending both together in 0003: > > > > +send_internal_dependencies(ParallelApplyWorkerInfo *winfo, List > > *depends_on_xids) > > +{ > > + pq_sendbyte(&dependencies, PARALLEL_APPLY_INTERNAL_MESSAGE); > > + pq_sendbyte(&dependencies, LOGICAL_REP_MSG_INTERNAL_MESSAGE); > > > > > > Also, it is confusing that above 2 are 'i' and > > WORKER_INTERNAL_MSG_RELATION is also 'i'. Code has become very tricky > > to understand now. > > > > Reviewing everything, I feel having 'i' outside of LogicalRepMsgType > > was better. I think it will eb better to retain > > PARALLEL_APPLY_INTERNAL_MESSAGE and getting rid of > > LOGICAL_REP_MSG_INTERNAL_MESSAGE. And when any worker intercepts > > PARALLEL_APPLY_INTERNAL_MESSAGE, it need not dispatch > > (apply_dispatch), instead it can handle it using > > apply_handle_internal_message() > > > > Goign above way: > > --Messaged received from pub can be handled using apply_dispatch. > > --Messages generated from leader to be handled separately/internally > > using apply_handle_internal_message(). > > > > That way we have clear-cut boundary between the two types and less > > confusion. > > Hi Shveta, > > IIUC these need to be separate because they are used in 2 completly > different ways: > > 1. In LogicalParallelApplyLoop the code need to identify as different > from PqReplMsg_WALData > 2. In apply_dispach() the message is delegated elsewhere according to > the type LogicalRepMsgType > > PSA a pictue I made for my understanding of the current v15-0001 > design. It might help to visualize the message format more easily. > > While your suggestion looks good for LogicalParallelApplyLoop, I think > the real problem is going to be in the apply_spooled_mesages() which > wants call the apply_dispatch() directly. That won't be possible if > LOGICAL_REP_MSG_INTERNAL_MESSAGE is removed. And, you cannot call > directly to apply_handle_internal_message() withint knowing it is a > PARALLEL_APPLY_INTERNAL_MESSAGE message, but that means first read it > pq_getmsgbyte(s). Then, you also need some hacky way to "unread" that > byte in case it was not the PARALLEL_APPLY_INTERNAL_MESSAGE byte, but > something different. AFAIK that was exactly what the previous > v14-0001 code was doing with the is_worker_internal_message() > function. I also think v15-0001 is a bit confusing, but v14-0001 was > even more so. > > If there was some new function like `pq_peekmsgbyte(s)` which could > simply "peek" the message byte value without advancing the cursor. > Then, I apply_spooled_mesages() can just peek to find > PARALLEL_APPLY_INTERNAL_MESSAGE and your suggested simplification > could work. But it would *still* be complicated by the fact that you > would have to ensure that PARALLEL_APPLY_INTERNAL_MESSAGE could not > clash with any of the LogicalRepMsgType! In the end, just keeping the > LOGICAL_REP_MSG_INTERNAL_MESSAGE like v14 does may be the best way to > ensure that uniqueness...
I meant to write. "In the end, just keeping the LOGICAL_REP_MSG_INTERNAL_MESSAGE like v15 does..." (sorry for version typo). > > > > > Also, can we use 'R' for WORKER_INTERNAL_MSG_RELATION similar to > > LOGICAL_REP_MSG_RELATION i.e. if 'i' is followed by 'R', then it means > > it is internal relation-msg instead of pub's one? 'R' seems a better > > choice over 'i' here. > > +1 > > ====== > Kind Reagrds, > Peter Smith. > Fujitsu Australia
