Dear Shveta,
> 3)
> + else if (am_parallel_apply_worker())
> + {
> + /* Attach to existing dynamic shared hash table. */
> + parallel_apply_dsa_area =
> dsa_attach(MyParallelShared->parallel_apply_dsa_handle);
> + dsa_pin_mapping(parallel_apply_dsa_area);
> + parallelized_txns = dshash_attach(parallel_apply_dsa_area, &dsh_params,
> + MyParallelShared->parallelized_txns_handle,
> + NULL);
> + }
>
> Shall we have a sanity check to ensure
> 'MyParallelShared->parallel_apply_dsa_handle != DSA_HANDLE_INVALID' in
> pa worker before invoking dsa_attach?
I think it's not necessary, you can refer to all other similar functions (e.g.,
logicalrep_launcher_attach_dshmem ..) and they also doesn't add the assert.
>
> 4)
> pa_attach_parallelized_txn_hash() is done irrespective of txn type
> (streaming/non-streaming), while handle_dependency_on_change() in 003
> has this:
>
> + /* Compute dependency only for non-streaming transaction */
> + if (in_streamed_transaction || (winfo && winfo->stream_txn))
> + return;
>
> I think both should be in sync in these initial patches. If we are
> trying to setup parallel worker for non-streaming txn for the first
> time, then we can initialize the shared-hash-table for dependency
> tracking, else skip it. pa_launch_parallel_worker() can be changed to
> accept 'stream_txn' argument which can then be used for this purpose.
This check does not exist anymore because streamed transactions won't
call the function prior to 0006.
I'm unclaer it's good to add a switch which would be not needed in the later patch;
Streaming transactions must be tracked and it's done in 0006. It adds maintaince
burden for intermediate state. Also, I think it may be possible that P.A. launched
for the streamed transactions would be re-used for the non-streaming transactions.
Other comments were addressed in [1].
[1]:
https://www.postgresql.org/message-id/OS9PR01MB121490458FFF58A543ABC32F4F5362%40OS9PR01MB12149.jpnprd01.prod.outlook.com
Best regards,
Hayato Kuroda
FUJITSU LIMITED
