On Thu, Jan 16, 2020 at 8:28 AM Michael Paquier <mich...@paquier.xyz> wrote: > > On Fri, Dec 27, 2019 at 10:15:33AM +0100, Julien Rouhaud wrote: > > I think that not using "parallel" to name this field will help to > > avoid confusion if the lock group infrastructure is eventually used > > for something else, but that's only true if indeed we explain what a > > lock group is. > > As you already pointed out, src/backend/storage/lmgr/README includes a > full description of this stuff under the section "Group Locking". So > I agree that the patch ought to document your new field in a much > better way, without mentioning the term "group locking" that's even > better to not confuse the reader because this term not present in the > docs at all. > > > The leader_pid is NULL for processes not involved in parallel query. > > When a process wants to cooperate with parallel workers, it becomes a > > lock group leader, which means that this field will be valued to its > > own pid. When a parallel worker starts up, this field will be valued > > with the leader pid. > > The first sentence is good to have. Now instead of "lock group > leader", I think that we had better use "parallel group leader" as in > other parts of the docs (see wait events for example).
Ok, I'll change this way. > Then we just > need to say that if leader_pid has the same value as > pg_stat_activity.pid, then we have a group leader. If not, then it is > a parallel worker process initially spawned by the leader whose PID is > leader_pid (when executing Gather, Gather Merge, soon-to-be parallel > vacuum or for a parallel btree build, but this does not need a mention > in the docs). There could be an argument as well to have leader_pid > set to NULL for a leader, but that would be inconsistent with what > the PGPROC entry reports for the backend. It would also slightly complicate things to get the full set of backends involved in a parallel query, while excluding the leader is entirely trivial. > While looking at the code, I think that we could refactor things a bit > for raw_wait_event, wait_event_type and wait_event which has some > duplicated code for backend and auxiliary processes. What about > filling in the wait event data after fetching the PGPROC entry, and > also fill in leader_pid for auxiliary processes. This does not matter > now, perhaps it will never matter (or not), but that would make the > code much more consistent. Yeah, I didn't think that auxiliary would be involved any time soon but I can include this refactoring.
