On Wed, Oct 19, 2022 at 6:30 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> 0001 seems mostly OK, but I don't like some of these new function names.
> I see you've named them so that they are case-consistent with the name
> of the struct member that they affect, but I don't think that's a good
> criterion. I propose
>
> SetrunningBackups -> XLogBackupSetRunning()
> ResetXLogBackupActivity -> XLogBackupNotRunning()
> // or maybe SetNotRunning, or ResetRunning? I prefer the one above
> SetlastBackupStart -> XLogBackupSetLastStart()
>
> GetlastFpwDisableRecPtr -> XLogGetLastFPWDisableRecptr()
> GetminRecoveryPoint -> XLogGetMinRecoveryPoint()
XLogBackupResetRunning() seemed better. +1 for above function names.
> I wouldn't say in the xlog_internal.h comment that these new functions
> are for xlogbackup.c to use. The API definition doesn't have to concern
> itself with that. Maybe one day xlogrecovery.c or some other xlog*.c
> would like to call those functions, and then the comment becomes a lie;
> and what for?
Removed.
> 0002 is where the interesting stuff happens. I have not reviewed that
> part with any care, but it appears that set_backup_state is pretty much
> useless. Let's get rid of it instead of moving it. Which also means
> that we shouldn't introduce reset_backup_status in 0001, I suppose.
> I think xlogfuncs.c is content with having just get_backup_status().
There's no set_backup_state() at all. We need get_backup_status() for
xlogfuncs.c and basebackup.c and we need reset_backup_status() for
XLogBackupResetRunning() sitting in xlog.c.
> Speaking of which -- I'm not sure we really want to do 0003.
> xlogfuncs.c is not a big file, the functions are not complex, and there
> are no interesting interactions in those functions with the internals
> (other than get_backup_status). I see that Michael advised the same.
> I propose we keep those functions where they are.
I'm okay either way.
Please see the attached v8 patch set.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
