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

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.

======

Commit message

2.

leader_pid is the process ID of the leader apply worker if this process is a
parallel apply worker. If this field is NULL, it indicates that the process is
a leader apply worker or does not participate in parallel apply, or a
synchronization worker.

~

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.

======

doc/src/sgml/monitoring.sgml

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.

E.g. Now this says basically "NULL if AAA or BBB, or CCC" but that
makes it sounds like there are 3 other things the process could be
instead of a parallel worker. But that is not not really true unless
you are making some distinction between the main "apply worker" which
is a leader versus a main apply worker which is not a leader. IMO we
should not be making any distinction at all - the leader apply worker
and the main (not leader) apply worker are one-and-the-same process.

So, I still prefer my previous suggestion (see [2] #5b]

======

src/backend/catalog/system_views.sql

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.

For example, I tried this:

CREATE VIEW pg_stat_subscription AS
SELECT
su.oid AS subid,
su.subname,
CASE
WHEN st.relid IS NOT NULL THEN 'tablesync'
WHEN st.leader_pid IS NOT NULL THEN 'parallel apply'
ELSE 'leader apply'
END AS kind,
st.pid,
st.leader_pid,
st.relid,
st.received_lsn,
st.last_msg_send_time,
st.last_msg_receipt_time,
st.latest_end_lsn,
st.latest_end_time
FROM pg_subscription su
LEFT JOIN pg_stat_get_subscription(NULL) st
ON (st.subid = su.oid);

and it results in much more readable output IMO:

test_sub=# select * from pg_stat_subscription;
subid | subname | kind | pid | leader_pid | relid |
received_lsn | last_msg_send_time |
last_msg_receipt_time | lat
est_end_lsn | latest_end_time
-------+---------+--------------+------+------------+-------+--------------+-------------------------------+-------------------------------+----
------------+-------------------------------
16388 | sub1 | leader apply | 5281 | | |
0/1901378 | 2023-01-13 12:39:03.984249+11 | 2023-01-13
12:39:03.986157+11 | 0/1
901378 | 2023-01-13 12:39:03.984249+11
(1 row)

Thoughts?

------
[1] Amit - https://www.postgresql.org/message-id/CAA4eK1KYUbnthSPyo4VjnhMygB0c1DZtp0XC-V2-GSETQ743ww%40mail.gmail.com
[2] My v78-0001 review -
https://www.postgresql.org/message-id/CAHut%2BPvA10Bp9Jaw9OS2%2BpuKHr7ry_xB3Tf2-bbv5gyxD5E_gw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia