Re: Parallel Apply - Mailing list pgsql-hackers

Please find a few more comments.

v15-002:
---------

1)
v15-002's commit message says:

Later patches will use this hash table to support dependency waiting
and commit order preservation. When transaction A depends on
transaction B (or must commit after B), the worker will wait for
transaction A's entry to be removed before committing transaction B.
~~

Txn names wrong in last sentence? Should it be these:

 When transaction A depends on transaction B (or must commit after B),
the worker will wait for transaction B's entry to be removed before
committing transaction A.

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.

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'



v15-003:
---------------

4)
Shall we have 'Assert(ParallelApplyTxnHash)' in
pa_get_last_commit_end() before accessing ParallelApplyTxnHash.

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.

6)
pa_get_last_commit_end():

+ entry->winfo = NULL;

We set entry->winfo to NULL unconditionaly above for a finsihed txn.
And the comment little above says:

* If worker info is NULL, it indicates that the worker has been reused
* for handling other transactions.

These 2 does not align. We are setting entry->winfo to NULL
unconditionaly when the txn is FINISHED, that does not mean worker has
been reused. It is slighlty confusing. Is there a scope of comment
improvement here?

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?


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;
~~

Reviewing further.

thanks
Shveta