On Fri, Jan 13, 2023 at 7:56 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are my review comments for v79-0001.
>
> ======
>
> General
>
> 1.
>
> When Amit suggested [1] changing the name just to "leader_pid" instead
> of "leader_apply_pid" I thought he was only referring to changing the
> view column name, not also the internal member names of the worker
> structure. Maybe it is OK anyway, but please check if that was the
> intention.
>
Yes, that was the intention.
>
> 3.
>
> <entry role="catalog_table_entry"><para role="column_definition">
> + <structfield>leader_pid</structfield> <type>integer</type>
> + </para>
> + <para>
> + 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
> + </para></entry>
>
> I felt this change is giving too many details and ended up just
> muddying the water.
>
I see that we give a similar description for other parameters as well.
For example leader_pid in pg_stat_activity, see client_dn,
client_serial in pg_stat_ssl. It is better to be consistent here and
this gives the reader a bit more information when the value is NULL
for the new column.
>
> 4.
>
> @@ -949,6 +949,7 @@ CREATE VIEW pg_stat_subscription AS
> su.oid AS subid,
> su.subname,
> st.pid,
> + st.leader_pid,
> st.relid,
> st.received_lsn,
> st.last_msg_send_time,
>
> IMO it would be very useful to have an additional "kind" attribute for
> this view. This will save the user from needing to do mental
> gymnastics every time just to recognise what kind of process they are
> looking at.
>
This could be a separate enhancement as the same should be true for
sync workers.
--
With Regards,
Amit Kapila.