On Sat, Dec 9, 2017 at 2:24 AM, Robert Haas <robertmh...@gmail.com> wrote: > 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 also added a sentence "If we allow interrupts before cleanup session-level lock, we could call do_pg_abort_backup with an inconsistent state" at two places: setting and cleanup session-level lock. >> 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. Agreed. Updated the comment. > > 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. > Attached the updated version patch. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
fix_do_pg_abort_backup_v11.patch
Description: Binary data
