RE: Perform streaming logical transactions by background workers and parallel apply

On Thursday, January 12, 2023 12:24 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi, here are some review comments for patch v78-0001.

Thanks for your comments.

> ======
>
> General
>
> 1. (terminology)
>
> AFAIK everywhere until now we’ve been referring everywhere
> (docs/comments/code) to the parent apply worker as the "leader apply
> worker". Not the "main apply worker". Not the "apply leader worker".
> Not any other variations...
>
> From this POV I think the worker member "apply_leader_pid" would be better
> named "leader_apply_pid", but I see that this was already committed to
> HEAD differently.
>
> Maybe it is not possible (or you don't want) to change that internal member
> name but IMO at least all the new code and docs should try to be using
> consistent terminology (e.g. leader_apply_XXX) where possible.
>
> ======
>
> Commit message
>
> 2.
>
> main_worker_pid is Process ID of the leader apply worker, if this process is a
> apply parallel worker. NULL if this process is a leader apply worker or a
> synchronization worker.
>
> IIUC, this text is just cut/paste from the monitoring.sgml. In a review comment
> below I suggest some changes to that text, so then this commit message
> should also change to be the same.

Changed.

> ~~
>
> 3.
>
> The new column can make it easier to distinguish leader apply worker and
> apply parallel worker which is also similar to the 'leader_pid' column in
> pg_stat_activity.
>
> SUGGESTION
> The new column makes it easier to distinguish parallel apply workers from
> other kinds of workers. It is implemented this way to be similar to the
> 'leader_pid' column in pg_stat_activity.

Changed.

> ======
>
> doc/src/sgml/logical-replication.sgml
>
> 4.
>
> + being synchronized. Moreover, if the streaming transaction is applied in
> + parallel, there will be additional workers.
>
> SUGGESTION
> there will be additional workers -> there may be additional parallel apply
> workers

Changed.

> ======
>
> doc/src/sgml/monitoring.sgml
>
> 5. pg_stat_subscription
>
> @@ -3198,11 +3198,22 @@ SELECT pid, wait_event_type, wait_event FROM
> pg_stat_activity WHERE wait_event i
>
> <row>
> <entry role="catalog_table_entry"><para role="column_definition">
> + <structfield>apply_leader_pid</structfield> <type>integer</type>
> + </para>
> + <para>
> + Process ID of the leader apply worker, if this process is a apply
> + parallel worker. NULL if this process is a leader apply worker or a
> + synchronization worker.
> + </para></entry>
> + </row>
> +
> + <row>
> + <entry role="catalog_table_entry"><para role="column_definition">
> <structfield>relid</structfield> <type>oid</type>
> </para>
> <para>
> OID of the relation that the worker is synchronizing; null for the
> - main apply worker
> + main apply worker and the parallel apply worker
> </para></entry>
> </row>
>
> 5a.
>
> (Same as general comment #1 about terminology)
>
> "apply_leader_pid" --> "leader_apply_pid"

I changed this and all related stuff to "leader_pid" as I agree with Amit that
this might be useful for future features and is more consistent with the
leader_pid in pg_stat_activity.

>
> ~~
>
> 5b.
>
> The current text feels awkward. I see it was copied from the similar text of
> 'pg_stat_activity' but perhaps it can be simplified a bit.
>
> SUGGESTION
> Process ID of the leader apply worker if this process is a parallel apply worker;
> otherwise NULL.

I slightly adjusted this according Amit's suggestion which I think would provide
more information.

"Process ID of the leader apply worker, if this process is a parallel apply worker.
NULL if this process is a leader apply worker or does not participate in parallel apply, or a synchronization worker."
"

> ~~
>
> 5c.
> BEFORE
> null for the main apply worker and the parallel apply worker
>
> AFTER
> null for the leader apply worker and parallel apply workers

Changed.

> ~~
>
> 5c.
>
> <structfield>relid</structfield> <type>oid</type>
> </para>
> <para>
> OID of the relation that the worker is synchronizing; null for the
> - main apply worker
> + main apply worker and the parallel apply worker
> </para></entry>
>
>
> main apply worker -> leader apply worker
>

Changed.

> ~~~
>
> 6.
>
> @@ -3212,7 +3223,7 @@ SELECT pid, wait_event_type, wait_event FROM
> pg_stat_activity WHERE wait_event i
> </para>
> <para>
> Last write-ahead log location received, the initial value of
> - this field being 0
> + this field being 0; null for the parallel apply worker
> </para></entry>
> </row>
>
> BEFORE
> null for the parallel apply worker
>
> AFTER
> null for parallel apply workers
>

Changed.

> ~~~
>
> 7.
>
> @@ -3221,7 +3232,8 @@ SELECT pid, wait_event_type, wait_event FROM
> pg_stat_activity WHERE wait_event i
> <structfield>last_msg_send_time</structfield> <type>timestamp
> with time zone</type>
> </para>
> <para>
> - Send time of last message received from origin WAL sender
> + Send time of last message received from origin WAL sender; null for
> the
> + parallel apply worker
> </para></entry>
> </row>
>
> (same as #6)
>
> BEFORE
> null for the parallel apply worker
>
> AFTER
> null for parallel apply workers
>

Changed.

> ~~~
>
> 8.
>
> @@ -3230,7 +3242,8 @@ SELECT pid, wait_event_type, wait_event FROM
> pg_stat_activity WHERE wait_event i
> <structfield>last_msg_receipt_time</structfield>
> <type>timestamp with time zone</type>
> </para>
> <para>
> - Receipt time of last message received from origin WAL sender
> + Receipt time of last message received from origin WAL sender; null for
> + the parallel apply worker
> </para></entry>
> </row>
>
> (same as #6)
>
> BEFORE
> null for the parallel apply worker
>
> AFTER
> null for parallel apply workers
>

Changed.

> ~~~
>
> 9.
>
> @@ -3239,7 +3252,8 @@ SELECT pid, wait_event_type, wait_event FROM
> pg_stat_activity WHERE wait_event i
> <structfield>latest_end_lsn</structfield> <type>pg_lsn</type>
> </para>
> <para>
> - Last write-ahead log location reported to origin WAL sender
> + Last write-ahead log location reported to origin WAL sender; null for
> + the parallel apply worker
> </para></entry>
> </row>
>
> (same as #6)
>
> BEFORE
> null for the parallel apply worker
>
> AFTER
> null for parallel apply workers
>

Changed.

> ~~~
>
> 10.
>
> @@ -3249,7 +3263,7 @@ SELECT pid, wait_event_type, wait_event FROM
> pg_stat_activity WHERE wait_event i
> </para>
> <para>
> Time of last write-ahead log location reported to origin WAL
> - sender
> + sender; null for the parallel apply worker
> </para></entry>
> </row>
> </tbody>
>
> (same as #6)
>
> BEFORE
> null for the parallel apply worker
>
> AFTER
> null for parallel apply workers
>

Changed.

> 12b.
>
> I wondered if here the code should be using the
> isParallelApplyWorker(worker) macro here for readability.
>
> e.g.
>
> if (isParallelApplyWorker(worker))
> values[3] = Int32GetDatum(worker.apply_leader_pid);
> else
> nulls[3] = true;

Changed.

Best Regards,
Hou Zhijie