Обсуждение: Re: pgsql: Rename contrib module basic_archive to basic_wal_module
On Wed, Jan 25, 2023 at 12:37 AM Michael Paquier <michael@paquier.xyz> wrote: > Rename contrib module basic_archive to basic_wal_module FWIW, I find this new name much less clear than the old one. If we want to provide a basic_archive module and a basic_recovery module, that seems fine. Why merge them? -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Jan 25, 2023 at 12:49:45PM -0500, Robert Haas wrote: > On Wed, Jan 25, 2023 at 12:37 AM Michael Paquier <michael@paquier.xyz> wrote: >> Rename contrib module basic_archive to basic_wal_module > > FWIW, I find this new name much less clear than the old one. > > If we want to provide a basic_archive module and a basic_recovery > module, that seems fine. Why merge them? I'll admit I've been stewing on whether "WAL Modules" is the right name. My first instinct was to simply call it "Archive and Recovery Modules," which is longer but (IMHO) clearer. I wanted to merge basic_archive and basic_recovery because there's a decent chunk of duplicated code. Perhaps that is okay, but I would rather just have one test module. AFAICT the biggest reason to split it is because we can't determine a good name. Maybe we could leave the name as "basic_archive" since it deals with creating and recovering archive files. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Wed, Jan 25, 2023 at 1:17 PM Nathan Bossart <nathandbossart@gmail.com> wrote: > On Wed, Jan 25, 2023 at 12:49:45PM -0500, Robert Haas wrote: > > On Wed, Jan 25, 2023 at 12:37 AM Michael Paquier <michael@paquier.xyz> wrote: > >> Rename contrib module basic_archive to basic_wal_module > > > > FWIW, I find this new name much less clear than the old one. > > > > If we want to provide a basic_archive module and a basic_recovery > > module, that seems fine. Why merge them? > > I'll admit I've been stewing on whether "WAL Modules" is the right name. > My first instinct was to simply call it "Archive and Recovery Modules," > which is longer but (IMHO) clearer. > > I wanted to merge basic_archive and basic_recovery because there's a decent > chunk of duplicated code. Perhaps that is okay, but I would rather just > have one test module. AFAICT the biggest reason to split it is because we > can't determine a good name. Maybe we could leave the name as > "basic_archive" since it deals with creating and recovering archive files. Yeah, maybe. I'm not sure what the best thing to do is, but if I see a module called basic_archive or basic_restore, I know what it's about, whereas basic_wal_module seems a lot less specific. It sounds like it could be generating or streaming it just as easily as it could be archiving it. It would be nice to have a name that is less prone to that kind of unclarity. -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Jan 25, 2023 at 02:05:39PM -0500, Robert Haas wrote: > On Wed, Jan 25, 2023 at 1:17 PM Nathan Bossart <nathandbossart@gmail.com> wrote: >> I wanted to merge basic_archive and basic_recovery because there's a decent >> chunk of duplicated code. Perhaps that is okay, but I would rather just >> have one test module. AFAICT the biggest reason to split it is because we >> can't determine a good name. Maybe we could leave the name as >> "basic_archive" since it deals with creating and recovering archive files. > > Yeah, maybe. I'm not sure what the best thing to do is, but if I see a > module called basic_archive or basic_restore, I know what it's about, > whereas basic_wal_module seems a lot less specific. It sounds like it > could be generating or streaming it just as easily as it could be > archiving it. It would be nice to have a name that is less prone to > that kind of unclarity. Good point. It seems like the most straightforward approach is just to have separate modules. Unless Michael objects, I'll go that route. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
> I wanted to merge basic_archive and basic_recovery because there's a decent
> chunk of duplicated code.
Would said code likely be duplicated into non-test uses of this feature?
If so, maybe you ought to factor it out into a common location. I agree
with Robert's point that basic_wal_module is a pretty content-free name.
regards, tom lane
Hi, On 2023-01-25 14:05:39 -0500, Robert Haas wrote: > > I wanted to merge basic_archive and basic_recovery because there's a decent > > chunk of duplicated code. Perhaps that is okay, but I would rather just > > have one test module. AFAICT the biggest reason to split it is because we > > can't determine a good name. Maybe we could leave the name as > > "basic_archive" since it deals with creating and recovering archive files. > > Yeah, maybe. I'm not sure what the best thing to do is, but if I see a > module called basic_archive or basic_restore, I know what it's about, > whereas basic_wal_module seems a lot less specific. It sounds like it > could be generating or streaming it just as easily as it could be > archiving it. It would be nice to have a name that is less prone to > that kind of unclarity. I think it'd be just fine to keep the name as basic_archive and use it for both archiving and restoring. Restoring from an archive still deals with archiving. I agree that basic_wal_module isn't a good name. Greetings, Andres Freund
On Wed, Jan 25, 2023 at 04:50:22PM -0500, Tom Lane wrote: > Nathan Bossart <nathandbossart@gmail.com> writes: >> I wanted to merge basic_archive and basic_recovery because there's a decent >> chunk of duplicated code. > > Would said code likely be duplicated into non-test uses of this feature? > If so, maybe you ought to factor it out into a common location. I agree > with Robert's point that basic_wal_module is a pretty content-free name. I doubt it. The duplicated parts are things like _PG_init(), the check hook for the GUC, and all the rest of the usual boilerplate stuff for extensions (e.g., Makefile, meson.build). This module is small enough that this probably makes up the majority of the code. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Wed, Jan 25, 2023 at 01:58:01PM -0800, Andres Freund wrote: > I think it'd be just fine to keep the name as basic_archive and use it for > both archiving and restoring. Restoring from an archive still deals with > archiving. This is my preference. If Michael and Robert are okay with it, I think this is what we should do. Else, I'll create separate basic_archive and basic_restore modules. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Wed, Jan 25, 2023 at 02:41:18PM -0800, Nathan Bossart wrote: > This is my preference. If Michael and Robert are okay with it, I think > this is what we should do. Else, I'll create separate basic_archive and > basic_restore modules. Grouping both things into the same module has the advantage to ease the configuration, at least in the example, where the archive directory GUC can be used for both paths. Regarding the renaming, I pushed for that. There's little love for it as far as I can see, so will revert accordingly. Keeping basic_archive as name for the module while it includes recovery is fine by me. -- Michael