Thread: moving basebackup code to its own directory
Hi, I was thinking that it might make sense, to reduce clutter, to move *backup*.c from src/backend/replication to a new directory, perhaps src/backend/replication/backup or src/backend/backup. There's no particular reason we *have* to do this, but there are 21 C files in that directory and 11 of them are basebackup-related, so maybe it's time, especially because I think we might end up adding more basebackup-related stuff. Thoughts? -- Robert Haas EDB: http://www.enterprisedb.com
On Tue, Aug 9, 2022 at 6:08 PM Robert Haas <robertmhaas@gmail.com> wrote:
Hi,
I was thinking that it might make sense, to reduce clutter, to move
*backup*.c from src/backend/replication to a new directory, perhaps
src/backend/replication/backup or src/backend/backup.
There's no particular reason we *have* to do this, but there are 21 C
files in that directory and 11 of them are basebackup-related, so
maybe it's time, especially because I think we might end up adding
more basebackup-related stuff.
Thoughts?
Those 11 files are mostly your fault, of course ;)
Anyway, I have no objection. If there'd been that many files, or plans to have it, in the beginning we probably would've put them in replication/basebackup or something like that from the beginning. I'm not sure how much it's worth doing wrt effects on backpatching etc, but if we're planning to add even more files in the future, the pain will just become bigger once we eventually do it...
On Tue, Aug 9, 2022 at 12:12 PM Magnus Hagander <magnus@hagander.net> wrote: > Those 11 files are mostly your fault, of course ;) They are. I tend to prefer smaller source files than many developers, because I find them easier to understand and maintain. If you only include <zlib.h> in basebackup_gzip.c, then you can be pretty sure nothing else involved with basebackup is accidentally depending on it. Similarly with static variables. If you just have one giant file, it's harder to be sure about that sort of thing. > Anyway, I have no objection. If there'd been that many files, or plans to have it, in the beginning we probably would'veput them in replication/basebackup or something like that from the beginning. I'm not sure how much it's worth doingwrt effects on backpatching etc, but if we're planning to add even more files in the future, the pain will just becomebigger once we eventually do it... Right. It's not exactly clear to me what the optimal source code layout is here. I think the placement here is under src/backend/replication because the functionality is accessed via the replication protocol, but I'm not sure if all backup-related code we ever add will be related to the replication protocol. As a thought experiment, imagine a background worker that triggers a backup periodically, or a monitoring view that tells you about the status of your last 10 backup attempts, or an in-memory hash table that tracks which files have been modified since the last backup. I'm not planning on implementing any of those things specifically, but I guess I'm a little concerned that if we just do the obvious thing of src/backend/replication/backup it's going to be end up being a little awkward if I or anyone else want to add backup-related code that isn't specifically about the replication protocol. So maybe src/backend/backup? Or is that too grandiose for the amount of stuff we have here? -- Robert Haas EDB: http://www.enterprisedb.com
On 8/9/22 12:12, Magnus Hagander wrote: > On Tue, Aug 9, 2022 at 6:08 PM Robert Haas <robertmhaas@gmail.com > <mailto:robertmhaas@gmail.com>> wrote: > > Hi, > > I was thinking that it might make sense, to reduce clutter, to move > *backup*.c from src/backend/replication to a new directory, perhaps > src/backend/replication/backup or src/backend/backup. > > There's no particular reason we *have* to do this, but there are 21 C > files in that directory and 11 of them are basebackup-related, so > maybe it's time, especially because I think we might end up adding > more basebackup-related stuff. > > Thoughts? > > > Those 11 files are mostly your fault, of course ;) > > Anyway, I have no objection. If there'd been that many files, or plans > to have it, in the beginning we probably would've put them in > replication/basebackup or something like that from the beginning. I'm > not sure how much it's worth doing wrt effects on backpatching etc, but > if we're planning to add even more files in the future, the pain will > just become bigger once we eventually do it... There are big changes all around for PG15 so back-patching will be complicated no matter what. +1 from me and it would be great if we can get this into the PG15 branch as well. Regards, -David
On 8/9/22 12:34, Robert Haas wrote: > On Tue, Aug 9, 2022 at 12:12 PM Magnus Hagander <magnus@hagander.net> wrote: > >> Anyway, I have no objection. If there'd been that many files, or plans to have it, in the beginning we probably would'veput them in replication/basebackup or something like that from the beginning. I'm not sure how much it's worth doingwrt effects on backpatching etc, but if we're planning to add even more files in the future, the pain will just becomebigger once we eventually do it... > > Right. > > It's not exactly clear to me what the optimal source code layout is > here. I think the placement here is under src/backend/replication > because the functionality is accessed via the replication protocol, > but I'm not sure if all backup-related code we ever add will be > related to the replication protocol. As a thought experiment, imagine > a background worker that triggers a backup periodically, or a > monitoring view that tells you about the status of your last 10 backup > attempts, or an in-memory hash table that tracks which files have been > modified since the last backup. I'm not planning on implementing any > of those things specifically, but I guess I'm a little concerned that > if we just do the obvious thing of src/backend/replication/backup it's > going to be end up being a little awkward if I or anyone else want to > add backup-related code that isn't specifically about the replication > protocol. > > So maybe src/backend/backup? Or is that too grandiose for the amount > of stuff we have here? +1 for src/backend/backup. I'd also be happy to see the start/stop code move here at some point. Regards, -David
On Tue, Aug 9, 2022 at 6:41 PM David Steele <david@pgmasters.net> wrote:
On 8/9/22 12:34, Robert Haas wrote:
> On Tue, Aug 9, 2022 at 12:12 PM Magnus Hagander <magnus@hagander.net> wrote:
>
>> Anyway, I have no objection. If there'd been that many files, or plans to have it, in the beginning we probably would've put them in replication/basebackup or something like that from the beginning. I'm not sure how much it's worth doing wrt effects on backpatching etc, but if we're planning to add even more files in the future, the pain will just become bigger once we eventually do it...
>
> Right.
>
> It's not exactly clear to me what the optimal source code layout is
> here. I think the placement here is under src/backend/replication
> because the functionality is accessed via the replication protocol,
> but I'm not sure if all backup-related code we ever add will be
> related to the replication protocol. As a thought experiment, imagine
> a background worker that triggers a backup periodically, or a
> monitoring view that tells you about the status of your last 10 backup
> attempts, or an in-memory hash table that tracks which files have been
> modified since the last backup. I'm not planning on implementing any
> of those things specifically, but I guess I'm a little concerned that
> if we just do the obvious thing of src/backend/replication/backup it's
> going to be end up being a little awkward if I or anyone else want to
> add backup-related code that isn't specifically about the replication
> protocol.
>
> So maybe src/backend/backup? Or is that too grandiose for the amount
> of stuff we have here?
+1 for src/backend/backup. I'd also be happy to see the start/stop code
move here at some point.
Yeah, sounds reasonable. There's never an optimal source code layout, but I agree this one is better than putting it under replication.
On Tue, Aug 9, 2022 at 12:43 PM Magnus Hagander <magnus@hagander.net> wrote: >> > So maybe src/backend/backup? Or is that too grandiose for the amount >> > of stuff we have here? >> >> +1 for src/backend/backup. I'd also be happy to see the start/stop code >> move here at some point. > > Yeah, sounds reasonable. There's never an optimal source code layout, but I agree this one is better than putting it underreplication. OK, here's a patch. -- Robert Haas EDB: http://www.enterprisedb.com
Attachment
On 8/9/22 13:32, Robert Haas wrote: > On Tue, Aug 9, 2022 at 12:43 PM Magnus Hagander <magnus@hagander.net> wrote: >>>> So maybe src/backend/backup? Or is that too grandiose for the amount >>>> of stuff we have here? >>> >>> +1 for src/backend/backup. I'd also be happy to see the start/stop code >>> move here at some point. >> >> Yeah, sounds reasonable. There's never an optimal source code layout, but I agree this one is better than putting it underreplication. > > OK, here's a patch. This looks good to me. Regards, -David
On Tue, Aug 09, 2022 at 01:32:49PM -0400, Robert Haas wrote: > On Tue, Aug 9, 2022 at 12:43 PM Magnus Hagander <magnus@hagander.net> wrote: > >> > So maybe src/backend/backup? Or is that too grandiose for the amount > >> > of stuff we have here? > >> > >> +1 for src/backend/backup. I'd also be happy to see the start/stop code > >> move here at some point. > > > > Yeah, sounds reasonable. There's never an optimal source code layout, but I agree this one is better than putting itunder replication. > > OK, here's a patch. It looks like this updates the header comments in the .h files but not the .c files. Personally, I find these to be silly boilerplate .. -- Justin
On 8/9/22 14:40, Justin Pryzby wrote: > On Tue, Aug 09, 2022 at 01:32:49PM -0400, Robert Haas wrote: >> On Tue, Aug 9, 2022 at 12:43 PM Magnus Hagander <magnus@hagander.net> wrote: >>>>> So maybe src/backend/backup? Or is that too grandiose for the amount >>>>> of stuff we have here? >>>> >>>> +1 for src/backend/backup. I'd also be happy to see the start/stop code >>>> move here at some point. >>> >>> Yeah, sounds reasonable. There's never an optimal source code layout, but I agree this one is better than putting itunder replication. >> >> OK, here's a patch. > > It looks like this updates the header comments in the .h files but not the .c > files. > > Personally, I find these to be silly boilerplate .. Good catch. I did not notice that just looking at the diff. Definitely agree that repeating the filename in the top comment is mostly useless, but that seems like a separate conversation. -David
On Tue, Aug 9, 2022 at 2:40 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > It looks like this updates the header comments in the .h files but not the .c > files. > > Personally, I find these to be silly boilerplate .. Here is a version with some updates to the silly boilerplate. -- Robert Haas EDB: http://www.enterprisedb.com
Attachment
On Tue, Aug 9, 2022 at 3:28 PM Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Aug 9, 2022 at 2:40 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > It looks like this updates the header comments in the .h files but not the .c > > files. > > > > Personally, I find these to be silly boilerplate .. > > Here is a version with some updates to the silly boilerplate. If there are no further comments on this I will go ahead and commit it. David Steele voted for back-patching this on the grounds that it would make future back-patching easier, which is an argument that seems to me to have some merit, although on the other hand, we are already into August so it's quite late in the day. Anyone else want to vote? -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Aug 10, 2022 at 10:08:02AM -0400, Robert Haas wrote: > On Tue, Aug 9, 2022 at 3:28 PM Robert Haas <robertmhaas@gmail.com> wrote: > > On Tue, Aug 9, 2022 at 2:40 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > It looks like this updates the header comments in the .h files but not the .c > > > files. > > > > > > Personally, I find these to be silly boilerplate .. > > > > Here is a version with some updates to the silly boilerplate. > > If there are no further comments on this I will go ahead and commit it. > > David Steele voted for back-patching this on the grounds that it would > make future back-patching easier, which is an argument that seems to > me to have some merit, although on the other hand, we are already into > August so it's quite late in the day. Anyone else want to vote? No objection to backpatching to v15, but if you don't, git ought to handle renamed files just fine. These look like similar precedent for "late" renaming+backpatching: 41dae3553, 47ca48364 -- Justin
Robert Haas <robertmhaas@gmail.com> writes:
> David Steele voted for back-patching this on the grounds that it would
> make future back-patching easier, which is an argument that seems to
> me to have some merit, although on the other hand, we are already into
> August so it's quite late in the day. Anyone else want to vote?
Seems like low-risk refactoring, so +1 for keeping v15 close to HEAD.
regards, tom lane
On Wed, Aug 10, 2022 at 6:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > > David Steele voted for back-patching this on the grounds that it would > > make future back-patching easier, which is an argument that seems to > > me to have some merit, although on the other hand, we are already into > > August so it's quite late in the day. Anyone else want to vote? > > Seems like low-risk refactoring, so +1 for keeping v15 close to HEAD. +1, but I suggest also getting a hat-tip from the RMT on it. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
On 8/10/22 12:32 PM, Magnus Hagander wrote: > On Wed, Aug 10, 2022 at 6:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >> Robert Haas <robertmhaas@gmail.com> writes: >>> David Steele voted for back-patching this on the grounds that it would >>> make future back-patching easier, which is an argument that seems to >>> me to have some merit, although on the other hand, we are already into >>> August so it's quite late in the day. Anyone else want to vote? >> >> Seems like low-risk refactoring, so +1 for keeping v15 close to HEAD. > > +1, but I suggest also getting a hat-tip from the RMT on it. With RMT hat on, given a few folks who maintain backup utilities seem to be in favor of backpatching to v15 and they are the ones to be most affected by this, it seems to me that this is an acceptable, noncontroversial course of action. Jonathan
Attachment
On 2022-Aug-10, Robert Haas wrote: > David Steele voted for back-patching this on the grounds that it would > make future back-patching easier, which is an argument that seems to > me to have some merit, although on the other hand, we are already into > August so it's quite late in the day. Anyone else want to vote? Given that 10 of these 11 files are new in 15, I definitely agree with backpatching the move. Moving the include/ files is going to cause some pain for any third-party code #including those files. I don't think this is a problem. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
On Wed, Aug 10, 2022 at 12:57 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Given that 10 of these 11 files are new in 15, I definitely agree with > backpatching the move. OK, done. -- Robert Haas EDB: http://www.enterprisedb.com