On Fri, Jan 13, 2023 at 7:56 AM Peter Smith <smithpb2...@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.
