RE: Parallel Apply

On Friday, April 17, 2026 7:48 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Fri, Apr 17, 2026 at 12:57 PM shveta malik <shveta(dot)malik(at)gmail(dot)com>
> wrote:
> >
> > On Thu, Apr 16, 2026 at 8:35 PM Zhijie Hou (Fujitsu)
> > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > >
> > > On Tuesday, April 14, 2026 9:00 PM Kuroda, Hayato/黒田 隼人
> <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> > > >
> > > > Other comments were addressed accordingly, please see attached patch
> set.
> > >
> >
> > Thanks for the patches. I have started reviewing it today, A few
> > comment son 001:

Thanks for the comments!

> >
> >
> > 1)
> > + elog(DEBUG1, "parallel apply worker worker init relmap for %s",
> > + rel->relname);
> >
> > worker mentioned twice

Removed.

> >
> > 2)
> > Calling handle_dependency_on_change() from
> > handle_streamed_transaction() is misleading, since the former is
> > intended for non-streaming transactions, while the latter handles
> > streaming ones.

It was added there because both transaction types require dependency tracking
after applying all patches, but as suggested by Amit, I've moved streaming
transaction support to later patches and added a dedicated function for
non-streaming transaction dependencies.

> >
> > I am not able to think of a better name for
> > handle_streamed_transaction() that would make calling the dependency
> > function from within it feel natural. So the only option I see is to
> > move handle_dependency_on_change() out. I think should be okay for
> > this function to be called from multiple places. In fact, this would
> > likely improve clarity for someone reading the
> > apply_handle_insert/update/delete code independently.
> >
> > 3)
> > Since caller of apply_handle_internal_message(), which is
> > apply_spooled_messages() is called from both leader and pa-worker;
> > apply_handle_internal_message() may benefit from below sanity check to
> > ensure only pa-workers intercept PARALLEL_APPLY_INTERNAL_MESSAGE:
> >
> > Assert(am_parallel_apply_worker())

Added.

> >
> > 4)
> > The name pa_wait_for_depended_transaction() suggests that it is
> > pa-specific worker. We can retain the name as is, but can we add a
> > comment atop this funciton saying used by both parallel and leader
> > worker?

Added some comments.

> >
> > 5)
> > I started reading 002's commit message, I think it is not that clear.
> > I was trying to find if we have actual value for remote-xid which is
> > key to hash tbale. But I think it is hash-table for only xid as key
> > for faster access may be? If so, can we please improve commit messagee
> > little bit?
> >
>
> Few comments on 002:
>
> 1)
> + if (am_leader_apply_worker())
> + {
> + /* Initialize dynamic shared hash table for last-start times. */
> + parallel_apply_dsa_area =
> dsa_create(LWTRANCHE_PARALLEL_APPLY_DSA);
> + dsa_pin(parallel_apply_dsa_area);
> + dsa_pin_mapping(parallel_apply_dsa_area);
> + parallelized_txns = dshash_create(parallel_apply_dsa_area,
> + &dsh_params, NULL);
>
> Comment seem unrelated.
>
> 2)
> A comment will help for below part in pa_wait_for_depended_transaction().

Added.

>
> pa_lock_transaction(xid, AccessShareLock); pa_unlock_transaction(xid,
> AccessShareLock);
>
> 3)
> Here if pa_lock/pa_unlock_transaction() is to end wait on dependent
> transaction (i.e. txn commit is guaranted with this), then for what purpose do
> we need infinite loop in pa_wait_for_depended_transaction?

It's for the race condition when the lock is released when the worker exits or
the worker has not yet acquired the lock, I added some comments in the patch
to explain.

Best Regards,
Hou zj