On Thu, Jan 16, 2020 at 04:27:27PM +0900, Michael Paquier wrote: > 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.
And actually, the way you are looking at the leader's PID is visibly incorrect and inconsistent because the patch takes no shared LWLock on the leader using LockHashPartitionLockByProc() followed by LWLockAcquire(), no? That's incorrect because it could be perfectly possible to crash with this code between the moment you check if lockGroupLeader is NULL and the moment you look at lockGroupLeader->pid if a process is being stopped in-between and removes itself from a lock group in ProcKill(). That's also inconsistent because it could be perfectly possible to finish with an incorrect view of the data while scanning for all the backend entries, like a leader set to NULL with workers pointing to the leader for example, or even workers marked incorrectly as NULL. The second one may not be a problem, but the first one could be confusing. -- Michael
signature.asc
Description: PGP signature
