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

On Thursday, January 12, 2023 12:24 PM Peter Smith <smithpb2250@gmail.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