On Tue, Dec 17, 2019 at 6:40 PM Michael Paquier <michael@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.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company