On Fri, Sep 22, 2017 at 3:00 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Fri, Sep 22, 2017 at 11:34 AM, Masahiko Sawada <sawada.m...@gmail.com> > wrote: >> You're right. I updated the patch so that it exits do_pg_abort_backup >> if the state is NONE and setting the state to NONE in >> do_pg_stop_backup before releasing the WAL insert lock. > > - /* Clean up session-level lock */ > + /* > + * Clean up session-level lock. To avoid interrupting before changing > + * the backup state by LWLockWaitForVar we change it while holding the > + * WAL insert lock. > + */ > sessionBackupState = SESSION_BACKUP_NONE; > You could just mention directly CHECK_FOR_INTERRUPTS here.
Thank you for the reviewing. I have a question. Since WALInsertLockRelease seems not to call LWLockWaitForVar I thought you wanted to mean LWLockReleaseClearVar instead, is that right? > + /* Quick exit if we have done the backup */ > + if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_NONE) > + return; > > The patch contents look fine to me. I have also tested things in > depths but could not find any problems. I also looked again at the > backup start code, trying to see any flows between the moment the > session backup lock is changed and the moment the callback to do > backup cleanup is registered but I have not found any issues. I am > marking this as ready for committer. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
