On Tuesday, June 9, 2026 12:14 PM Peter Smith <smithpb2250@gmail.com> wrote:
> Hi Hou-San.
>
> Some review comments for v19-0001 and v19-0002
>
Thanks for the comments.
> //////////
> v19-0001
>
> 1a.
> Maybe that ParallelizedTxnEntry should be moved to just immediately above
> 'dshash_parameters' because it seems to belong with that, and currently it is
> splitting the ParallelApplyWorkerEntry from the ParallelApplyTxnHash.
Moved as suggested.
>
> ~
>
> 1b.
> /parameters for/Parameters for/
>
> ~~~
Fixed.
> pa_attach_parallelized_txn_hash:
> 2a.
> This might be easier to read if rearranged to use if/else instead of having the
> early return.
I'm not in favor of this style, as deeply nested condition blocks are harder for
me to understand. Please see existing examples like init_dsm_registry,
initGlobalChannelTable, and logicalrep_launcher_attach_dshmem for the preferred
coding style.
>
> ~~~
>
> 2b.
> Can it be anything other than
> am_leader_apply_worker/am_parallel_apply_worker here? Should there be
> an Assert?
Only the leader and parallel apply workers need to coordinate in this context,
so I didn't add an Assert. Even if other processes use it, it will be a no-op,
which is fine.
>
> ~~~
>
> 2c.
> Since the `dsh_params` are already set up, shouldn't this code be using them?
>
> BEFORE
> dsa_create(LWTRANCHE_PARALLEL_APPLY_DSA);
>
> SUGGESTION
> dsa_create(dsh_params.tranche_id);
>
I think dsa_create and dsh_params are two different objects. The former is for
pure shared memory allocation, while the latter is part of a hash table.
Therefore, it's not necessary to pass dsh_params to dsa_create. We could even
use different LWTRANCHE for them if we wanted, but we don't have a special need
to create another one at the moment.
>
> //////////
> v19-0002
>
> ======
> .../replication/logical/applyparallelworker.
>
> 1.
> +void
> +pa_wait_for_depended_transaction(TransactionId xid) {
> +ParallelizedTxnEntry *txn_entry;
>
> The declaration of `txn_entry` can be done later within the loop where it is
> used.
Moved.
Attaching the new version patch set.
Apart from addressing above comments, I also did the following changes:
0003:
- Fix detection of dependencies on toasted columns by properly copying column
status.
- Ensure leader correctly handles table-wide dependencies when applying a
transaction directly.
- Prevent infinite loop by skipping self-dependencies when building the
dependency list.
- Expand test coverage for dependency tracking.
- Improve code readability through refactoring.
- Enhance comments for better documentation.
0008:
0009:
- Complete most TODOs and finalize the design to cover more cases. Add extensive
comments to explain the design.
- Add more tests to cover the new code paths.
Best Regards,
Hou zj
