On Fri, Dec 8, 2017 at 4:13 AM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Fri, Dec 8, 2017 at 5:38 PM, Masahiko Sawada <sawada.m...@gmail.com> wrote: >> After off-discussion with Fujii-san, I've updated the comment of why >> we should disallow interrupts before setting/cleanup the session-level >> lock. Please review it. > > + /* > + * Set session-level lock. If we allow interrupts before setting > + * session-level lock, we could call callbacks with an inconsistent > + * state. To avoid calling CHECK_FOR_INTERRUPTS by > LWLockReleaseClearVar > + * which is called by WALInsertLockRelease before changing the backup > + * state we change it while holding the WAL insert lock. > + */ > So you are just adding the reference to WALInsertLockRelease.. Instead > of writing the function names for LWLocks, I would just write "To > avoid calling CHECK_FOR_INTERRUPTS which can happen when releasing a > LWLock" and be done with it. There is no point to list a full function > dependency list, which could change in the future with static routines > of lwlock.c.
I think it's actually good to be explicit here. I looked at this patch a bit last week and had great difficulty understanding how the CHECK_FOR_INTERRUPTS() could happen. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company