On Tue, Jan 28, 2020 at 12:36:41PM +0100, Julien Rouhaud wrote:
On Sat, Jan 18, 2020 at 3:51 AM Michael Paquier <mich...@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.
I agree a separate "leader_id" column is easier to work with, as it does
not require unnesting and so on.
As for the consistency, I agree we probably can't make this perfect, as
we're fetching and processing the PGPROC records one by one. Fixing that
would require acquiring a much stronger lock on PGPROC, and perhaps some
other locks. That's pre-existing behavior, of course, it's just not very
obvious as we don't have any dependencies between the rows, I think.
Adding the leader_id will change, that, of course. But I think it's
still mostly OK, even with the possible inconsistency.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
