archive modules

On 2021/11/02 3:54, Bossart, Nathan wrote:

This thread is a continuation of the thread with the subject
"parallelizing the archiver" [0]. That thread had morphed into an
effort to allow creating archive modules, so I've created a new one to
ensure that this topic has the proper visibility.

What is the main motivation of this patch? I was thinking that
it's for parallelizing WAL archiving. But as far as I read
the patch very briefly, WAL file name is still passed to
the archive callback function one by one.

Are you planning to extend this mechanism to other WAL
archiving-related commands like restore_command? I can imagine
that those who use archive library (rather than shell) would
like to use the same mechanism for WAL restore.

I've attached the latest patch from the previous thread. This patch
does a few things. First, it adds the archive_library GUC that
specifies a library to use in place of archive_command. If
archive_library is set to "shell" (the default), archive_command is
still used. The archive_library is preloaded, so its _PG_init() can
do anything that libraries loaded via shared_preload_libraries can do.
Like logical decoding output plugins, archive modules must define an
initialization function and some callbacks. The patch also introduces
the basic_archive module to ensure test coverage on the new
infrastructure.

I think that it's worth adding this module into core
rather than handling it as test module. It provides very basic
WAL archiving feature, but (I guess) it's enough for some users.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Tue, Nov 02, 2021 at 01:43:54PM +0900, Fujii Masao wrote:

On 2021/11/02 3:54, Bossart, Nathan wrote:

This thread is a continuation of the thread with the subject
"parallelizing the archiver" [0]. That thread had morphed into an
effort to allow creating archive modules, so I've created a new one to
ensure that this topic has the proper visibility.

What is the main motivation of this patch? I was thinking that
it's for parallelizing WAL archiving. But as far as I read
the patch very briefly, WAL file name is still passed to
the archive callback function one by one.

It seems to me that this patch is not moving into the right direction
implementation-wise (I have read the arguments about
backward-compatibility that led to the introduction of archive_library
and its shell mode), for what looks like a duplicate of
shared_preload_libraries but for an extra code path dedicated to the
archiver, where we could just have a hook instead? We have been
talking for some time now to make the archiver process more
bgworker-ish, so as we finish with something closer to what the
logical replication launcher is.
--
Michael

On 11/1/21, 9:44 PM, "Fujii Masao" <masao.fujii@oss.nttdata.com> wrote:

What is the main motivation of this patch? I was thinking that
it's for parallelizing WAL archiving. But as far as I read
the patch very briefly, WAL file name is still passed to
the archive callback function one by one.

The main motivation is provide a way to archive without shelling out.
This reduces the amount of overhead, which can improve archival rate
significantly. It should also make it easier to archive more safely.
For example, many of the common shell commands used for archiving
won't fsync the data, but it isn't too hard to do so via C. The
current proposal doesn't introduce any extra infrastructure for
batching or parallelism, but it is probably still possible. I would
like to eventually add batching, but for now I'm only focused on
introducing basic archive module support.

Are you planning to extend this mechanism to other WAL
archiving-related commands like restore_command? I can imagine
that those who use archive library (rather than shell) would
like to use the same mechanism for WAL restore.

I would like to do this eventually, but my current proposal is limited
to archive_command.

I think that it's worth adding this module into core
rather than handling it as test module. It provides very basic
WAL archiving feature, but (I guess) it's enough for some users.

Do you think it should go into contrib?

Nathan

On Tue, Nov 2, 2021 at 1:24 AM Michael Paquier <michael@paquier.xyz> wrote:

It seems to me that this patch is not moving into the right direction
implementation-wise (I have read the arguments about
backward-compatibility that led to the introduction of archive_library
and its shell mode), for what looks like a duplicate of
shared_preload_libraries but for an extra code path dedicated to the
archiver, where we could just have a hook instead? We have been
talking for some time now to make the archiver process more
bgworker-ish, so as we finish with something closer to what the
logical replication launcher is.

Why in the world would you want a plain hook rather than something
closer to the way logical replication works?

Plain hooks are annoying to use. If you load things at the wrong time,
it silently doesn't work. It's also impossible to unload anything. If
you want to change to a different module, you probably have to bounce
the whole server instead of just changing the GUC and letting it load
the new module when you run 'pg_ctl reload'.

Blech! :-)

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

I've just realized I forgot to CC the active participants on the last
thread to this one, so I've attempted to do that now. I didn't
intentionally leave anyone out, but I'm sorry if I missed someone.

On 11/1/21, 10:24 PM, "Michael Paquier" <michael@paquier.xyz> wrote:

It seems to me that this patch is not moving into the right direction
implementation-wise (I have read the arguments about
backward-compatibility that led to the introduction of archive_library
and its shell mode), for what looks like a duplicate of
shared_preload_libraries but for an extra code path dedicated to the
archiver, where we could just have a hook instead? We have been
talking for some time now to make the archiver process more
bgworker-ish, so as we finish with something closer to what the
logical replication launcher is.

Hm. It sounds like you want something more like what was discussed
earlier in the other thread [0]/messages/by-id/8B7BF404-29D4-4662-A2DF-1AC4C98463EB@amazon.com. This approach would basically just
add a hook and call it a day. My patch for this approach [1]/messages/by-id/attachment/127385/v2-0001-Replace-archive_command-with-a-hook.patch moved
the shell archive logic to a test module, but the general consensus
seems to be that we'd need to have it be present in core (and the
default).

Nathan

[0]: /messages/by-id/8B7BF404-29D4-4662-A2DF-1AC4C98463EB@amazon.com
[1]: /messages/by-id/attachment/127385/v2-0001-Replace-archive_command-with-a-hook.patch

On 11/2/21, 8:11 AM, "Robert Haas" <robertmhaas@gmail.com> wrote:

Why in the world would you want a plain hook rather than something
closer to the way logical replication works?

Plain hooks are annoying to use. If you load things at the wrong time,
it silently doesn't work. It's also impossible to unload anything. If
you want to change to a different module, you probably have to bounce
the whole server instead of just changing the GUC and letting it load
the new module when you run 'pg_ctl reload'.

Well, the current patch does require a reload since the modules are
preloaded, but maybe there is some way to avoid that. However, I
agree with the general argument that a dedicated archive module
interface will be easier to use correctly. With a hook, it could be
hard to know which library's archive function we are actually using.
With archive_library, we know the exact library's callback functions
we are using.

I think an argument for just adding a hook is that it's simpler and
easier to maintain. But continuous archiving is a pretty critical
piece of functionality, so I think it's a great candidate for some
sturdy infrastructure.

Nathan

On Tue, Nov 2, 2021 at 11:26 AM Bossart, Nathan <bossartn@amazon.com> wrote:

Well, the current patch does require a reload since the modules are
preloaded, but maybe there is some way to avoid that.

I think we could set things up so that if the value changes, you call
a shutdown hook for the old library, load the new one, and call any
startup hook for that one.

That wouldn't work with a hook-based approach.

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

On 11/2/21, 8:57 AM, "Robert Haas" <robertmhaas@gmail.com> wrote:

On Tue, Nov 2, 2021 at 11:26 AM Bossart, Nathan <bossartn@amazon.com> wrote:

Well, the current patch does require a reload since the modules are
preloaded, but maybe there is some way to avoid that.

I think we could set things up so that if the value changes, you call
a shutdown hook for the old library, load the new one, and call any
startup hook for that one.

Yes, that seems doable. My point is that I've intentionally chosen to
preload the libraries at the moment so that it's possible to define
PGC_POSTMASTER GUCs and to use RegisterBackgroundWorker(). If we
think that switching archive modules without restarting is more
important, I believe we will need to take on a few restrictions.

Nathan

On Tue, Nov 2, 2021 at 12:10 PM Bossart, Nathan <bossartn@amazon.com> wrote:

Yes, that seems doable. My point is that I've intentionally chosen to
preload the libraries at the moment so that it's possible to define
PGC_POSTMASTER GUCs and to use RegisterBackgroundWorker(). If we
think that switching archive modules without restarting is more
important, I believe we will need to take on a few restrictions.

I guess I'm failing to understand what the problem is. You can set
GUCs of the form foo.bar in postgresql.conf anyway, right?

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

On 11/2/21, 9:17 AM, "Robert Haas" <robertmhaas@gmail.com> wrote:

On Tue, Nov 2, 2021 at 12:10 PM Bossart, Nathan <bossartn@amazon.com> wrote:

Yes, that seems doable. My point is that I've intentionally chosen to
preload the libraries at the moment so that it's possible to define
PGC_POSTMASTER GUCs and to use RegisterBackgroundWorker(). If we
think that switching archive modules without restarting is more
important, I believe we will need to take on a few restrictions.

I guess I'm failing to understand what the problem is. You can set
GUCs of the form foo.bar in postgresql.conf anyway, right?

I must not be explaining it well, sorry. I'm mainly thinking about
the following code snippets.

In guc.c:
/*
* Only allow custom PGC_POSTMASTER variables to be created during shared
* library preload; any later than that, we can't ensure that the value
* doesn't change after startup. This is a fatal elog if it happens; just
* erroring out isn't safe because we don't know what the calling loadable
* module might already have hooked into.
*/
if (context == PGC_POSTMASTER &&
!process_shared_preload_libraries_in_progress)
elog(FATAL, "cannot create PGC_POSTMASTER variables after startup");

In bgworker.c:
if (!process_shared_preload_libraries_in_progress &&
strcmp(worker->bgw_library_name, "postgres") != 0)
{
if (!IsUnderPostmaster)
ereport(LOG,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("background worker \"%s\": must be registered in shared_preload_libraries",
worker->bgw_name)));
return;
}

You could still introduce GUCs in _PG_init(), but they couldn't be
defined as PGC_POSTMASTER. Also, you could still use
RegisterDynamicBackgroundWorker() to register a background worker, but
you couldn't use RegisterBackgroundWorker(). These might be
acceptable restrictions if swapping archive libraries on the fly seems
more important, but I wanted to bring that front and center to make
sure everyone understands the tradeoffs.

It's also entirely possible I'm misunderstanding something here...

Nathan

On Tue, Nov 2, 2021 at 12:39 PM Bossart, Nathan <bossartn@amazon.com> wrote:>

On 11/2/21, 9:17 AM, "Robert Haas" <robertmhaas@gmail.com> wrote:
You could still introduce GUCs in _PG_init(), but they couldn't be
defined as PGC_POSTMASTER.

It seems like PGC_POSTMASTER isn't very desirable anyway. Wouldn't you
want PGC_SIGHUP? I mean I'm not saying there couldn't be a case where
that wouldn't work, because you could need a big chunk of shared
memory allocated at startup time or something. But in that's probably
not typical, and if it does happen well then that particular archive
module has to be preloaded. If you know that you have several archive
modules that you want to use, you can still preload them all if for
this or any other reason you want to do that. But in a lot of cases
it's not going to be necessary.

In other words, if we use hooks, then you're guaranteed to need a
server restart to change anything. If we use something like what you
have now, there can be corner cases where you need that or benefits of
preloading, but it's not a hard requirement, and in many cases you can
get by without it. That seems strictly better to me ... but maybe I'm
still confused.

Also, you could still use
RegisterDynamicBackgroundWorker() to register a background worker, but
you couldn't use RegisterBackgroundWorker(). These might be
acceptable restrictions if swapping archive libraries on the fly seems
more important, but I wanted to bring that front and center to make
sure everyone understands the tradeoffs.

RegisterDynamicBackgroundWorker() seems way better, though. It's hard
for me to understand why this would be a problem for anybody. And
again, if somebody does have that need, they can always fall back to
saying that their particular module should be preloaded if you want to
use it.

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

On 11/2/21, 9:46 AM, "Robert Haas" <robertmhaas@gmail.com> wrote:

On Tue, Nov 2, 2021 at 12:39 PM Bossart, Nathan <bossartn@amazon.com> wrote:>

On 11/2/21, 9:17 AM, "Robert Haas" <robertmhaas@gmail.com> wrote:
You could still introduce GUCs in _PG_init(), but they couldn't be
defined as PGC_POSTMASTER.

It seems like PGC_POSTMASTER isn't very desirable anyway. Wouldn't you
want PGC_SIGHUP? I mean I'm not saying there couldn't be a case where
that wouldn't work, because you could need a big chunk of shared
memory allocated at startup time or something. But in that's probably
not typical, and if it does happen well then that particular archive
module has to be preloaded. If you know that you have several archive
modules that you want to use, you can still preload them all if for
this or any other reason you want to do that. But in a lot of cases
it's not going to be necessary.

In other words, if we use hooks, then you're guaranteed to need a
server restart to change anything. If we use something like what you
have now, there can be corner cases where you need that or benefits of
preloading, but it's not a hard requirement, and in many cases you can
get by without it. That seems strictly better to me ... but maybe I'm
still confused.

Also, you could still use
RegisterDynamicBackgroundWorker() to register a background worker, but
you couldn't use RegisterBackgroundWorker(). These might be
acceptable restrictions if swapping archive libraries on the fly seems
more important, but I wanted to bring that front and center to make
sure everyone understands the tradeoffs.

RegisterDynamicBackgroundWorker() seems way better, though. It's hard
for me to understand why this would be a problem for anybody. And
again, if somebody does have that need, they can always fall back to
saying that their particular module should be preloaded if you want to
use it.

I agree. I'll make sure the archive library can be changed via SIGHUP
in the next revision.

Nathan

On 11/2/21, 10:29 AM, "Bossart, Nathan" <bossartn@amazon.com> wrote:

I agree. I'll make sure the archive library can be changed via SIGHUP
in the next revision.

And here it is.

Nathan

On 2021/11/03 0:03, Bossart, Nathan wrote:

On 11/1/21, 9:44 PM, "Fujii Masao" <masao.fujii@oss.nttdata.com> wrote:

What is the main motivation of this patch? I was thinking that
it's for parallelizing WAL archiving. But as far as I read
the patch very briefly, WAL file name is still passed to
the archive callback function one by one.

The main motivation is provide a way to archive without shelling out.
This reduces the amount of overhead, which can improve archival rate
significantly.

It's helpful if you share how much this approach reduces
the amount of overhead.

It should also make it easier to archive more safely.
For example, many of the common shell commands used for archiving
won't fsync the data, but it isn't too hard to do so via C.

But probably we can do the same thing even by using the existing
shell interface? For example, we can implement and provide
the C program of the archive command that fsync's the file?
Users can just use it in archive_command.

The
current proposal doesn't introduce any extra infrastructure for
batching or parallelism, but it is probably still possible. I would
like to eventually add batching, but for now I'm only focused on
introducing basic archive module support.

Understood. I agree that it's reasonable to implement them gradually.

Are you planning to extend this mechanism to other WAL
archiving-related commands like restore_command? I can imagine
that those who use archive library (rather than shell) would
like to use the same mechanism for WAL restore.

I would like to do this eventually, but my current proposal is limited
to archive_command.

Understood.

I think that it's worth adding this module into core
rather than handling it as test module. It provides very basic
WAL archiving feature, but (I guess) it's enough for some users.

Do you think it should go into contrib?

Yes, at least for me..

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 11/7/21 1:04 AM, Fujii Masao wrote:

On 2021/11/03 0:03, Bossart, Nathan wrote:

On 11/1/21, 9:44 PM, "Fujii Masao" <masao.fujii@oss.nttdata.com> wrote:

What is the main motivation of this patch? I was thinking that
it's for parallelizing WAL archiving. But as far as I read
the patch very briefly, WAL file name is still passed to
the archive callback function one by one.

The main motivation is provide a way to archive without shelling out.
This reduces the amount of overhead, which can improve archival rate
significantly.

It's helpful if you share how much this approach reduces
the amount of overhead.

FWIW we have a test for this in pgBackRest. Running
`archive_command=pgbackrest archive-push ...` 1000 times via system()
yields an average of 3ms per execution. pgBackRest reports ~1ms of time
here so the system() overhead is ~2ms. These times are on my very fast
workstation and in my experience servers are quite a bit slower.

This doesn't tell the entire story, though, because in this test
pgBackRest is just checking notifications being returned by an async
process that was spawned earlier. This complexity exists to save the
startup costs of, e.g. establishing an SSH connection, which is often >
1 second.

This module would make it far easier to pay those startup costs a single
time, or at least only occasionally, making it possible to write
performant archivers with less complexity than is currently possible.

It should also make it easier to archive more safely.
For example, many of the common shell commands used for archiving
won't fsync the data, but it isn't too hard to do so via C.

But probably we can do the same thing even by using the existing
shell interface? For example, we can implement and provide
the C program of the archive command that fsync's the file?
Users can just use it in archive_command.

It is far more common to be writing WAL segments to another host or
object storage. In either case I believe a local fsync file command is
not very useful.

I think that it's worth adding this module into core
rather than handling it as test module. It provides very basic
WAL archiving feature, but (I guess) it's enough for some users.

Do you think it should go into contrib?

I would prefer this module to be in core as our standard implementation
and load by default in a vanilla install.

Regards,
--
-David
david@pgmasters.net

On 11/10/21, 8:10 AM, "David Steele" <david@pgmasters.net> wrote:

On 11/7/21 1:04 AM, Fujii Masao wrote:

It's helpful if you share how much this approach reduces
the amount of overhead.

FWIW we have a test for this in pgBackRest. Running
`archive_command=pgbackrest archive-push ...` 1000 times via system()
yields an average of 3ms per execution. pgBackRest reports ~1ms of time
here so the system() overhead is ~2ms. These times are on my very fast
workstation and in my experience servers are quite a bit slower.

In the previous thread [0]/messages/by-id/E9035E94-EC76-436E-B6C9-1C03FBD8EF54@amazon.com, I noted a 50% speedup for a basic
archiving strategy that involved copying the file to a different
directory.

I would prefer this module to be in core as our standard implementation
and load by default in a vanilla install.

Hm. I think I disagree with putting it in contrib and with making it
the default archive library. The first reason is backward
compatibility. There has already been quite a bit of discussion about
this, and I don't see how we can get away with anything except for
maintaining the existing behavior for now. Maybe we could move to a
better default down the road, but I'm hesitant to press that issue too
much at the moment.

The second reason is that the basic_archive module has a couple of
deficiencies. For example, it doesn't handle inconvenient server
crashes well (e.g., after archiving but before we've renamed the
.ready file). A way to fix this might be to compare the archive file
with the to-be-archived file and to succeed if they are exactly the
same. Another problem is that there is no handling for multiple
servers using basic_archive to write WAL to the same location. This
is because basic_archive first copies data to a temporary file that is
always named "archtemp." This might be fixed by appending a random
string to the temporary file or by locking it somehow, but there are
still a few things left to figure out.

I think it'd be awesome to eventually fix all these issues in
basic_archive and to recommend it as a proper archiving strategy, but
I'm worried that this will introduce a significant amount of
complexity to this patch. I really only intended for basic_archive to
be used for testing and to demonstrate that it's possible use the
archive module infrastructure to do something useful. If folks really
want it in contrib, I'd at least add a big warning about the
aforementioned problems in its docs.

Nathan

[0]: /messages/by-id/E9035E94-EC76-436E-B6C9-1C03FBD8EF54@amazon.com

On 11/10/21 1:22 PM, Bossart, Nathan wrote:

On 11/10/21, 8:10 AM, "David Steele" <david@pgmasters.net> wrote:

I would prefer this module to be in core as our standard implementation
and load by default in a vanilla install.

Hm. I think I disagree with putting it in contrib and with making it
the default archive library. The first reason is backward
compatibility. There has already been quite a bit of discussion about
this, and I don't see how we can get away with anything except for
maintaining the existing behavior for now. Maybe we could move to a
better default down the road, but I'm hesitant to press that issue too
much at the moment.

OK, I haven't had to go over the patch in detail so I didn't realize the
module was not backwards compatible. I'll have a closer look soon.

The second reason is that the basic_archive module has a couple of
deficiencies. For example, it doesn't handle inconvenient server
crashes well (e.g., after archiving but before we've renamed the
.ready file). A way to fix this might be to compare the archive file
with the to-be-archived file and to succeed if they are exactly the
same. Another problem is that there is no handling for multiple
servers using basic_archive to write WAL to the same location. This
is because basic_archive first copies data to a temporary file that is
always named "archtemp." This might be fixed by appending a random
string to the temporary file or by locking it somehow, but there are
still a few things left to figure out.

Honestly, I'm not sure to what extent it makes sense to delve into these
problems for an archiver that basically just copies to another
directory. This is a not a very realistic solution for the common
storage requirements we are seeing these days.

I think it'd be awesome to eventually fix all these issues in
basic_archive and to recommend it as a proper archiving strategy, but
I'm worried that this will introduce a significant amount of
complexity to this patch. I really only intended for basic_archive to
be used for testing and to demonstrate that it's possible use the
archive module infrastructure to do something useful. If folks really
want it in contrib, I'd at least add a big warning about the
aforementioned problems in its docs.

I'll have more to say once I've had a closer look, but in general I
agree with what you have said here. Keeping it in test for now is likely
to be the best approach.

Regards,
--
-David
david@pgmasters.net

On 11/10/21, 10:42 AM, "David Steele" <david@pgmasters.net> wrote:

OK, I haven't had to go over the patch in detail so I didn't realize the
module was not backwards compatible. I'll have a closer look soon.

It's backward-compatible in the sense that you'd be able to switch
archive_library to "shell" to continue using archive_command, but
archive_command is otherwise unused. The proposed patch sets
archive_library to "shell" by default.

Honestly, I'm not sure to what extent it makes sense to delve into these
problems for an archiver that basically just copies to another
directory. This is a not a very realistic solution for the common
storage requirements we are seeing these days.

Agreed.

I'll have more to say once I've had a closer look, but in general I
agree with what you have said here. Keeping it in test for now is likely
to be the best approach.

Looking forward to your feedback.

Nathan

On 11/10/21, 10:53 AM, "Bossart, Nathan" <bossartn@amazon.com> wrote:

Looking forward to your feedback.

Here is a rebased patch (beb4e9b broke v8).

Nathan