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