RE: Parallel Apply

Dear Shveta,

> 2)
> pa_wait_for_depended_transaction()
>
> + * Acquiring the lock successfully does not guarantee we can proceed.
> + * The worker may have errored out and released the lock while leaving
> + * its shared hash entry intact, or it may not have acquired the lock
> + * yet because it hasn't processed the BEGIN message. In either case, we
> + * must continue waiting in the loop until the parallel apply worker
> + * finishes applying the transaction, or until the leader notifies us of
> + * a failure and restarts all workers.
>
> Regarding "until the parallel apply worker finishes applying the transaction"
>
> IIUC, it could be leader worker too applying the txn and not only
> parallel worker. Or let me know if that is not the case.

I did not address it. Because there are no cases that parallel apply worker waits
for the leader worker to apply the change. IIUC, the possible case is below.

1) leader calls the function to wait till the PA applies
2) PA calls the function to wait till the PA applies

> 3)
> + /*
> + * Quick exit if parallelized_txns has not been initialized yet. This can
> + * happen when this function is called by the leader worker.
> + */
> + if (!parallelized_txns)
> + return;
>
> Can we please elaborate this sentence: "when this function is called
> by the leader worker and ....?" IIUC, there has to be other condition
> along with 'called by the leader worker'

IIUC, the quick exit can happen if the leader calls the function and it has never
launched the parallel apply worker. Fixed.

> 5)
> pa_get_last_commit_end():
> I don't see any caller passing 'delete_entry = false' in this patch.
> Do we need this argument? May be later patches need this. In that
> case, shall we add 'delete_entry' in the patch where we actually have
> multiple-callers with different use-case scenario for delete-entry. In
> this patch, the only caller is
> cleanup_committed_replica_identity_entries() which can unconditionaly
> delete entries.

Now "bool delete_entry" would be added in 0004.

> 7)
>
> pa_transaction_committed has this check:
>
> + if (!ParallelApplyTxnHash)
> + return true;
>
> I see that pa_transaction_committed() call is too deep in
> dependency-tracking calls. Is it a possibility that at this stage,
> ParallelApplyTxnHash is still not initialized? Or did you mean to have
> Assert rather than above check? If above is a possibility, can we
> please add comments?

I think we cannot reach here with ParallelApplyTxnHash == NULL. It can be
initialized when the parallel apply worker is launched. For streaming=off case,
handle_dependency_on_change() can be called with new_depended_xid = InvalidTransactionId,
so any entries cannot be added in replica_identity_table. IIUC we cannot reach
there at that time. Now I used Assert().

> 8)
> Shall we cahnge this comment now to mention non-streamign txns too?
>
> /*
> * A hash table used to cache the state of streaming transactions being applied
> * by the parallel apply workers.
> */
> static HTAB *ParallelApplyTxnHash = NULL;

It was updated in 0004, because parallel apply workers are actually used from
the patch.

See the updated version [1].

[1]: https://www.postgresql.org/message-id/OS9PR01MB121490458FFF58A543ABC32F4F5362%40OS9PR01MB12149.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED