On 2022-Oct-22, Bharath Rupireddy wrote:
> + /* We should be here only by one of these reasons, never both */
> + Assert(during_backup_start ^
> + (sessionBackupState == SESSION_BACKUP_RUNNING));
> +
>
> What's the problem even if we're here when both of them are true?
In what case should we be there with both conditions true?
> The runningBackups is incremented anyways, right?
In the current code, yes, but it seems to be easier to reason about if
we know precisely why we're there and whether we should be running the
cleanup or not. Otherwise we might end up with a bug where we run the
function but it doesn't do anything because we failed to understand the
preconditions. At the very least, this forces a developer changing this
code to think through it.
> Why can't we just get rid of the Assert and treat during_backup_start
> as backup_marked_active_in_shmem or something like that to keep things
> simple?
Why is that simpler?
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La verdad no siempre es bonita, pero el hambre de ella sí"