On Sat, Jan 18, 2020 at 3:51 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Jan 17, 2020 at 05:07:55PM +0100, Julien Rouhaud wrote:
> > Oh indeed. But unless we hold some LWLock during the whole function
> > execution, we cannot guarantee a consistent view right?
>
> Yep. That's possible.
>
> > And isn't it already possible to e.g. see a parallel worker in
> > pg_stat_activity while all other queries are shown are idle, if
> > you're unlucky enough?
>
> Yep. That's possible.
>
> > Also, LockHashPartitionLockByProc requires the leader PGPROC, and
> > there's no guarantee that we'll see the leader before any of the
> > workers, so I'm unsure how to implement what you said. Wouldn't it be
> > better to simply fetch the leader PGPROC after acquiring a shared
> > ProcArrayLock, and using that copy to display the pid, after checking
> > that we retrieved a non-null PGPROC?
>
> Another idea would be to check if the current PGPROC entry is a leader
> and print in an int[] the list of PIDs of all the workers while
> holding a shared LWLock to avoid anything to be unregistered. Less
> handy, but a bit more consistent. One problem with doing that is
> that you may have in your list of PIDs some worker processes that are
> already gone when going through all the other backend entries. An
> advantage is that an empty array could mean "I am the leader, but
> nothing has been registered yet to my group lock." (note that the
> leader adds itself to lockGroupMembers).
So, AFAICT the LockHashPartitionLockByProc is required when
iterating/modifying lockGroupMembers or lockGroupLink, but just
getting the leader pid should be safe. Since we'll never be able to
get a totally consistent view of data here, I'm in favor of avoiding
taking extra locks here. I agree that outputting an array of the pid
would be more consistent for the leader, but will have its own set of
corner cases. It seems to me that a new leader_pid column is easier
to handle at SQL level, so I kept that approach in attached v4. If
you have strong objections to it. I can still change it.