On 2022-Oct-13, Bharath Rupireddy wrote: > Hm. Agree. But, that requires us to include xlogbackup.h in > xlog_internal.h for SessionBackupState enum in > ResetXLogBackupActivity(). Is that okay?
It's not great, but it's not *that* bad, ISTM, mainly because xlog_internal.h will affect less stuff than xlog.h. > SessionBackupState and it needs to be set before we release WAL insert > locks, see the comment [1]. I see. Maybe we could keep that enum in xlog.h, instead. While looking at how that works: I think calling a local variable "session_backup_state" is super confusing, seeing that we have a file-global variable called sessionBackupState. I recommend naming the local "newstate" or something along those lines instead. I wonder why does pg_backup_start_callback() not change the backup state before your patch. This seems a gratuitous difference, or is it? If you change that code so that it also sets the status to BACKUP_NONE, then you can pass a bare SessionBackupState to ResetXLogBackupActivity rather than a pointer to one, which is a very strange arrangement that exists only so that you can have a third state (NULL) meaning "don't change state" -- that looks quite weird. Alternatively, if you don't want or can't change pg_backup_start_callback to pass a valid state value, another solution might be to pass a separate boolean "change state". But I would look at having another patch before your series that changes pg_backup_start_callback to make the code identical for the three callers, then you can simplify the patched code. > Should we just remove the > SessionBackupState enum and convert SESSION_BACKUP_NONE and > SESSION_BACKUP_RUNNING to just macros in xlogbackup.h and use integer > type to pass the state across? I don't know what's better here. Do you > have any thoughts on this? No, please, no passing of unadorned magic numbers. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
