On Wed, Dec 18, 2019 at 12:19 PM Robert Haas <robertmh...@gmail.com> wrote: > > On Tue, Dec 17, 2019 at 6:40 PM Michael Paquier <mich...@paquier.xyz> wrote: > > So that's how you prevent piling up multiple registrations of this > > callback compared to v1. FWIW, I think that it is a cleaner approach > > to remove the callback once a non-exclusive backup is done, because a > > session has no need for it once it is done with its non-exclusive > > backup, and this session may remain around for some time. > > The fact that the session may remain around for some time isn't really > relevant, because the callback isn't consuming any resources. It does > not increase memory usage by a single byte, nor CPU consumption > either. It does consume a few CPU cycles when the backend finally > exits, but the number of such cycles is very small and unrelated to > the length of the session. And removing the callback isn't entirely > free, either. > > I think the real point for me is that it's bad to have two sources of > truth. We have the sessionBackupState variable that tells us whether > we're in a backup, and then we separately have whether or not the > callback is registered. If those two things ever get out of sync, as > they do in the test cases that I've proposed, then we have problems -- > so it's better not to maintain the state in two separate ways. > > The way it's set up right now actually seems quite fragile even apart > from the problem with cancel_before_shmem_exit(). do_pg_stop_backup() > sets sessionBackupState to SESSION_BACKUP_NONE and then does things > that might fail. If they do, then the cancel_before_shmem_exit() > callback will leave the callback installed, which can lead to a > spurious warning or assertion failure later as in the original report. > The only way to avoid that problem would be to move the > cancel_before_shmem_exit() callback so that it happens right next to > setting sessionBackupState to SESSION_BACKUP_NONE -- note the comments > there saying we can't even do WALInsertLockRelease() between updating > XLogCtl and updating sessionBackupState. But that would be very ugly, > because we'd then have to pass a flag to do_pg_stop_backup() saying > whether to remove the callback, since only one caller wants that > behavior. > > And, we'd have to change cancel_before_shmem_exit() to search the > whole array, which would almost certainly cost more cycles than > leaving a do-nothing callback around.
If pg_abort_backup callback function can be called safely even when the backup is not in progress, we can just use the global variable like pg_abort_backup_registered to register the callback function only on first call. In this way, cancel_before_shmem_exit() doesn't need to search the array to get rid of the function. Regards, -- Fujii Masao
