On 3/15/17 2:28 AM, Michael Paquier wrote: > On Wed, Mar 15, 2017 at 12:27 AM, Anastasia Lubennikova > <lubennikov...@gmail.com> wrote: >> As far as I understand, in this thread were discussed two bugs of >> pg_stop_backup(). >> Thanks to the clear descriptions above, I easily reproduced both of them. >> >> BUG#1: >> Server crashes on assertion on call of pg_stop_backup(false) after >> interrupted call of pg_stop_backup(false). >> TRAP: FailedAssertion("!(XLogCtl->Insert.nonExclusiveBackups > 0)", File: >> "xlog.c", Line: 10747) >> >> BUG#2: >> Failure to start an exclusive backup with the same name, if previous >> exclusive backup was stopped in another session. >> >> Both problems seem to be fixed with patch "backup-session-locks-fixes.patch". >> Speaking of the patch itself, I have a question: shouldn't we also update >> sessionBackupState >> in pg_stop_backup_callback() along with XLogCtl->Insert.exclusiveBackupState? > > No, that's not necessary. sessionBackupState is not changed until the > code path where pg_stop_backup_callback() is active, and remains > unchanged until it gets deactivated. > >> And couple of minor notes: >> 1) + * Routines to starting stop, and get status of a base backup >> Probably should be: + * Routines to start, stop and get status of a base >> backup >> And also this comment should be moved below the enum. >> >> 2) This is used in parallel of the shared memory status >> s/ in parallel of/ in parallel with > > Agreed on both points.
I have tested this patch and it behaves as expected and fixes the original issue I reported. One nit, I think: + * SESSION_BACKUP_EXCLUSIVE in this one. Actual verification that an Would be better phrased as: + * SESSION_BACKUP_EXCLUSIVE in this function. Actual verification that an Thanks, -- -David da...@pgmasters.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers