Horton

Thread

Commits

  1. Move basebackup code to new directory src/backend/backup

  1. moving basebackup code to its own directory

    Robert Haas <robertmhaas@gmail.com> — 2022-08-09T16:08:04Z

    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

  2. Re: moving basebackup code to its own directory

    Magnus Hagander <magnus@hagander.net> — 2022-08-09T16:12:16Z

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

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

  3. Re: moving basebackup code to its own directory

    Robert Haas <robertmhaas@gmail.com> — 2022-08-09T16:34:46Z

    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'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?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  4. Re: moving basebackup code to its own directory

    David Steele <david@pgmasters.net> — 2022-08-09T16:35:58Z

    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

  5. Re: moving basebackup code to its own directory

    David Steele <david@pgmasters.net> — 2022-08-09T16:41:28Z

    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.

Regards,
-David

  6. Re: moving basebackup code to its own directory

    Magnus Hagander <magnus@hagander.net> — 2022-08-09T16:43:28Z

    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.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

  7. Re: moving basebackup code to its own directory

    Robert Haas <robertmhaas@gmail.com> — 2022-08-09T17:32:49Z

    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 under replication.

OK, here's a patch.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  8. Re: moving basebackup code to its own directory

    David Steele <david@pgmasters.net> — 2022-08-09T17:49:34Z

    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 under replication.
> 
> OK, here's a patch.

This looks good to me.

Regards,
-David

  9. Re: moving basebackup code to its own directory

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-09T18:40:16Z

    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 it under 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

  10. Re: moving basebackup code to its own directory

    David Steele <david@pgmasters.net> — 2022-08-09T18:49:39Z

    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 it under 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

  11. Re: moving basebackup code to its own directory

    Robert Haas <robertmhaas@gmail.com> — 2022-08-09T19:28:39Z

    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

  12. Re: moving basebackup code to its own directory

    Robert Haas <robertmhaas@gmail.com> — 2022-08-10T14:08:02Z

    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

  13. Re: moving basebackup code to its own directory

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-10T14:25:03Z

    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

  14. Re: moving basebackup code to its own directory

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-10T16:20:47Z

    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

  15. Re: moving basebackup code to its own directory

    Magnus Hagander <magnus@hagander.net> — 2022-08-10T16:32:10Z

    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/

  16. Re: moving basebackup code to its own directory

    Jonathan S. Katz <jkatz@postgresql.org> — 2022-08-10T16:41:25Z

    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

  17. Re: moving basebackup code to its own directory

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-08-10T16:57:34Z

    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/

  18. Re: moving basebackup code to its own directory

    Robert Haas <robertmhaas@gmail.com> — 2022-08-10T18:04:00Z

    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