Re: Parallel Apply - Mailing list pgsql-hackers

On Thu, Apr 23, 2026 at 7:31 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Wed, Apr 22, 2026 at 7:23 PM shveta malik <shveta.malik@gmail.com> 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...

Okay. I see your point. Thanks for explaning.

Another approach could be the one shown in the attached patch. In this approach:

a) We avoid pre-reading the message and then rewinding the cursor,
unlike the approach used in apply_spooled_messages() in v14.
b) We keep a single LOGICAL_REP_MSG_INTERNAL_MESSAGE for internal
messages; a separate PARALLEL_APPLY_INTERNAL_MESSAGE wrapper is not
required.
c) The caller decides whether to let apply_dispatch read the next
message or to act on an already pre-read message. This makes the
design more flexible if we need to handle additional pre-read internal
messages in the future, without introducing new wrapper message
formats.
d) The logic for dispatching actions on all message types remains
encapsulated within apply_dispatch.

Thanks
Shveta