Re: Parallel Apply - Mailing list pgsql-hackers

On Fri, Apr 17, 2026 at 12:57 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Apr 16, 2026 at 8:35 PM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Tuesday, April 14, 2026 9:00 PM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.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:
>
>
> 1)
> + elog(DEBUG1, "parallel apply worker worker init relmap for %s",
> + rel->relname);
>
> worker mentioned twice
>
> 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.
>
> 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())
>
> 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?
>
> 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().

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?

thanks
Shveta