Re: Parallel Apply

On Wed, May 13, 2026 at 10:54 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Wed, May 6, 2026 at 9:18 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Thu, Apr 30, 2026 at 8:10 PM Zhijie Hou (Fujitsu)
> > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > >
> > > On Wednesday, April 29, 2026 7:32 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > > >
> > > >
> > > > Few more comments on v17-003:
> > > >
> > > >
> > >
> > > Thanks for the comments, I have addressed all of them.
> > >
> > > Here is the latest patch set.
> >
> > Thank you. I seem to have missed this email thread (as it was split
> > into a new thread) and was waiting for the patches. I’ve just noticed
> > it now and will resume the review.
> >
>
> Please find a few comments for patch003 mostly:
>
> 1)
> * depended on by other transactions. Entries are of type ParallelizedTxnEntry.
> *
> * dshash is used to enable dynamic shared memory allocation based on
> the number
> - * of transactions being applied in parallel.
> + * of transactions being applied in parallel. Entries are of type
> ParallelizedTxnEntry.
> */
> static dsa_area *parallel_apply_dsa_area = NULL;
> static dshash_table *parallelized_txns = NULL;
>
> 'Entries are of type ParallelizedTxnEntry' repeated twice in this comment.
>
>
> 2)
> cleanup_committed_replica_identity_entries:
>
> + if (!skipped_write && !XLogRecPtrIsValid(pos->local_end))
> + continue;
>
> Perhaps a comment will help to indicate above checks means a txn not
> yet finished.
>
> 3)
> Can you please clarify the scope, life-span of entries in
> parallelized_txns vs ParallelApplyTxnHash. Both have remote-xid field.
> So at any point of time, do both tables will have same number of
> entries or if entries in one has bigger life-span/scope as compared to
> other? It will be good to briefly mention these atop the hash-tables.
>
> 4)
> +/*
> + * Hash table storing replica identity values for changes being applied in
> + * parallel, along with the last transaction that modified each row.
> ...
> +static replica_identity_hash *replica_identity_table = NULL;
>
> Regarding 'along with the last transaction that modified each row', is
> 'remote_xid' in ReplicaIdentityEntry is the last transaction that
> modified this row or the one which is currently modifying it?
>
> 5)
> Since we have added comments for rest for the fields for below
> existing structure, do you think we can update comment for 'xid' as
> well to say it is remote-one. It does not mention it anywhere in
> comment.
>
> typedef struct ParallelApplyWorkerEntry
> {
> TransactionId xid; /* Hash key -- must be first */
>
>
> 6)
> 003' commit message says about RI table entry:
>
> Entries are deleted when transactions committed by parallel workers
> are gathered, or the number of entries exceeds the limit.
> --
> I don't understand what do we mean by "when transactions committed by
> parallel workers are gathered". Can we please make it more
> clear/elaborate.
>
> ~~
>
> Reviewing further.
>

Few more on 003:

7)
I find check_and_record_ri_dependency() difficult to understand.

a)
As an example, this part:

+ /*
+ * Return if no entry exists, or if the current transaction was the last one
+ * to modify the key.
+ */
+ if (!rientry || TransactionIdEquals(rientry->remote_xid, new_depended_xid))
+ return;

IMO, the second check makes sense if new_depended_xid is valid. If so,
why don't we make it part of previous 'if
(TransactionIdIsValid(new_depended_xid))' logic. If 'found' was true,
we can check it inside that if-block and 'return' from there instead
of proceeding further.

Once we make above change, we can even move below logic inside
previous 'if (TransactionIdIsValid(new_depended_xid))' block, as it
looks strange that in previous if-block we are assigning
InvalidTransactionId to 'rientry->remote_xid' while we have valid
new_depended_xid available there.

+ /*
+ * Update the new depended xid into the entry if valid, the new xid could
+ * be invalid if the transaction will be applied by the leader itself
+ * which means all the changes will be committed before processing next
+ * transaction, so no need to be depended on.
+ */
+ if (TransactionIdIsValid(new_depended_xid))
+ rientry->remote_xid = new_depended_xid;

b)
Also this part is not clear:

+ /*
+ * Return if RI key is NULL or is explicitly marked unchanged. The key
+ * value could be NULL in the new tuple of a update opertaion which
+ * means the RI key is not updated.
+ */
+ if (original_data->colstatus[i] == LOGICALREP_COLUMN_NULL ||
+ original_data->colstatus[i] == LOGICALREP_COLUMN_UNCHANGED)
+ return;

Why do have we 'return' here when one of the columns is NULL or
unchanged? What happens to rest of the RI columns? Which scenario may
hit this? It needs more comments to explain the scenario.

8)
check_and_record_ri_dependency() has this comment and logic around
invalid remote_xid:

+ /*
+ * Remove the entry if the transaction has been committed and no new
+ * dependency needs to be added.
+ */
+ else if (!TransactionIdIsValid(rientry->remote_xid))
+ {
+ free_replica_identity_key(rientry->keydata);
+ replica_identity_delete_item(replica_identity_table, rientry);
+ }

While find_all_dependencies_on_rel() has this assert:
+ Assert(TransactionIdIsValid(rientry->remote_xid));

The first logic says that we may have Invalid remote_xid in existing
entry in replica_identity_table while second logic has a sanity check
while iterating the same hash-table that all entries must have valid
remote_xid. Is the Assert correct? We might have Invalid remote_xid if
txn is committed (done in check_and_append_xid_dependency).

thanks
Shveta