Re: Move backup-related code to xlogbackup.c/.h

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>
Cc: Andres Freund <andres(at)anarazel(dot)de>, Michael Paquier <michael(at)paquier(dot)xyz>, Nathan Bossart <nathandbossart(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Move backup-related code to xlogbackup.c/.h
Date: 2022-10-19 13:00:15
Message-ID: 20221019130015.ilmzn4fonudfsurm@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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()

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?

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().

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.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"I suspect most samba developers are already technically insane...
Of course, since many of them are Australians, you can't tell." (L. Torvalds)

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2022-10-19 13:57:03 Re: allowing for control over SET ROLE
Previous Message kuroda.hayato@fujitsu.com 2022-10-19 12:49:52 RE: Perform streaming logical transactions by background workers and parallel apply