recovery modules

Hi,

On 2022-12-27 11:24:49 -0800, Nathan Bossart wrote:

I've attached a patch set that adds the restore_library,
archive_cleanup_library, and recovery_end_library parameters to allow
archive recovery via loadable modules. This is a follow-up to the
archive_library parameter added in v15 [0] [1].

Why do we need N parameters for this? To me it seems more sensible to have one
parameter that then allows a library to implement all these (potentially
optionally).

* Unlike archive modules, recovery libraries cannot be changed at runtime.
There isn't a safe way to unload a library, and archive libraries work
around this restriction by restarting the archiver process. Since recovery
libraries are loaded via the startup and checkpointer processes (which
cannot be trivially restarted like the archiver), the same workaround is
not feasible.

I don't think that's a convincing reason to not support configuration
changes. Sure, libraries cannot be unloaded, but an unnecessarily loaded
library is cheap. All that's needed is to redirect the relevant function
calls.

* pg_rewind uses restore_command, but there isn't a straightforward path to
support restore_library. I haven't addressed this in the attached patches,
but perhaps this is a reason to allow specifying both restore_command and
restore_library at the same time. pg_rewind would use restore_command, and
the server would use restore_library.

That seems problematic, leading to situations where one might not be able to
use restore_command anymore, because it's not feasible to do
segment-by-segment restoration.

Greetings,

Andres Freund

On Tue, Dec 27, 2022 at 02:11:11PM -0800, Andres Freund wrote:

On 2022-12-27 11:24:49 -0800, Nathan Bossart wrote:

I've attached a patch set that adds the restore_library,
archive_cleanup_library, and recovery_end_library parameters to allow
archive recovery via loadable modules. This is a follow-up to the
archive_library parameter added in v15 [0] [1].

Why do we need N parameters for this? To me it seems more sensible to have one
parameter that then allows a library to implement all these (potentially
optionally).

The main reason is flexibility. Separate parameters allow using a library
for one thing and a command for another, or different libraries for
different things. If that isn't a use-case we wish to support, I don't
mind combining all three into a single recovery_library parameter.

* Unlike archive modules, recovery libraries cannot be changed at runtime.
There isn't a safe way to unload a library, and archive libraries work
around this restriction by restarting the archiver process. Since recovery
libraries are loaded via the startup and checkpointer processes (which
cannot be trivially restarted like the archiver), the same workaround is
not feasible.

I don't think that's a convincing reason to not support configuration
changes. Sure, libraries cannot be unloaded, but an unnecessarily loaded
library is cheap. All that's needed is to redirect the relevant function
calls.

This might leave some stuff around (e.g., GUCs, background workers), but if
that isn't a concern, I can adjust it to work as you describe.

* pg_rewind uses restore_command, but there isn't a straightforward path to
support restore_library. I haven't addressed this in the attached patches,
but perhaps this is a reason to allow specifying both restore_command and
restore_library at the same time. pg_rewind would use restore_command, and
the server would use restore_library.

That seems problematic, leading to situations where one might not be able to
use restore_command anymore, because it's not feasible to do
segment-by-segment restoration.

I'm not following why this would make segment-by-segment restoration
infeasible. Would you mind elaborating?

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Hi,

On 2022-12-27 14:37:11 -0800, Nathan Bossart wrote:

On Tue, Dec 27, 2022 at 02:11:11PM -0800, Andres Freund wrote:

On 2022-12-27 11:24:49 -0800, Nathan Bossart wrote:

I've attached a patch set that adds the restore_library,
archive_cleanup_library, and recovery_end_library parameters to allow
archive recovery via loadable modules. This is a follow-up to the
archive_library parameter added in v15 [0] [1].

Why do we need N parameters for this? To me it seems more sensible to have one
parameter that then allows a library to implement all these (potentially
optionally).

The main reason is flexibility. Separate parameters allow using a library
for one thing and a command for another, or different libraries for
different things. If that isn't a use-case we wish to support, I don't
mind combining all three into a single recovery_library parameter.

I think the configuration complexity is a sufficient concern to not go that
direction...

* Unlike archive modules, recovery libraries cannot be changed at runtime.
There isn't a safe way to unload a library, and archive libraries work
around this restriction by restarting the archiver process. Since recovery
libraries are loaded via the startup and checkpointer processes (which
cannot be trivially restarted like the archiver), the same workaround is
not feasible.

I don't think that's a convincing reason to not support configuration
changes. Sure, libraries cannot be unloaded, but an unnecessarily loaded
library is cheap. All that's needed is to redirect the relevant function
calls.

This might leave some stuff around (e.g., GUCs, background workers), but if
that isn't a concern, I can adjust it to work as you describe.

You can still have a shutdown hook re background workers. I don't think the
GUCs matter, given that it's the startup/checkpointer processes.

* pg_rewind uses restore_command, but there isn't a straightforward path to
support restore_library. I haven't addressed this in the attached patches,
but perhaps this is a reason to allow specifying both restore_command and
restore_library at the same time. pg_rewind would use restore_command, and
the server would use restore_library.

That seems problematic, leading to situations where one might not be able to
use restore_command anymore, because it's not feasible to do
segment-by-segment restoration.

I'm not following why this would make segment-by-segment restoration
infeasible. Would you mind elaborating?

Latency effects for example can make it infeasible to do segment-by-segment
restoration infeasible performance wise. On the most extreme end, imagine WAL
archived to tape or such.

Greetings,

Andres Freund

On Tue, Dec 27, 2022 at 02:45:30PM -0800, Andres Freund wrote:

On 2022-12-27 14:37:11 -0800, Nathan Bossart wrote:

On Tue, Dec 27, 2022 at 02:11:11PM -0800, Andres Freund wrote:

On 2022-12-27 11:24:49 -0800, Nathan Bossart wrote:

* pg_rewind uses restore_command, but there isn't a straightforward path to
support restore_library. I haven't addressed this in the attached patches,
but perhaps this is a reason to allow specifying both restore_command and
restore_library at the same time. pg_rewind would use restore_command, and
the server would use restore_library.

That seems problematic, leading to situations where one might not be able to
use restore_command anymore, because it's not feasible to do
segment-by-segment restoration.

I'm not following why this would make segment-by-segment restoration
infeasible. Would you mind elaborating?

Latency effects for example can make it infeasible to do segment-by-segment
restoration infeasible performance wise. On the most extreme end, imagine WAL
archived to tape or such.

I'm sorry, I'm still lost here. Wouldn't restoration via library tend to
improve latency? Is your point that clusters may end up depending on this
improvement so much that a shell command would no longer be able to keep
up? I might be creating a straw man, but this seems like less of a concern
for pg_rewind since it isn't meant for continuous, ongoing restoration.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Dec 27, 2022 at 02:11:11PM -0800, Andres Freund wrote:

On 2022-12-27 11:24:49 -0800, Nathan Bossart wrote:

* Unlike archive modules, recovery libraries cannot be changed at runtime.
There isn't a safe way to unload a library, and archive libraries work
around this restriction by restarting the archiver process. Since recovery
libraries are loaded via the startup and checkpointer processes (which
cannot be trivially restarted like the archiver), the same workaround is
not feasible.

I don't think that's a convincing reason to not support configuration
changes. Sure, libraries cannot be unloaded, but an unnecessarily loaded
library is cheap. All that's needed is to redirect the relevant function
calls.

Agreed. That seems worth the cost to switching this stuff to be
SIGHUP-able.

* pg_rewind uses restore_command, but there isn't a straightforward path to
support restore_library. I haven't addressed this in the attached patches,
but perhaps this is a reason to allow specifying both restore_command and
restore_library at the same time. pg_rewind would use restore_command, and
the server would use restore_library.

That seems problematic, leading to situations where one might not be able to
use restore_command anymore, because it's not feasible to do
segment-by-segment restoration.

Do you mean that supporting restore_library in pg_rewind is a hard
requirement? I fail to see why this should be the case. Note that
having the possibility to do dlopen() calls in the frontend (aka
libpq) for loading some callbacks is something I've been looking for
in the past. Having consistency in terms of restrictions between
library and command like for archives would make sense I guess (FWIW,
I mentioned on the thread of d627ce3 that we'd better not put any
restrictions for the archives).
--
Michael

Hi,

On 2022-12-27 15:04:28 -0800, Nathan Bossart wrote:

I'm sorry, I'm still lost here. Wouldn't restoration via library tend to
improve latency? Is your point that clusters may end up depending on this
improvement so much that a shell command would no longer be able to keep
up?

Yes.

I might be creating a straw man, but this seems like less of a concern
for pg_rewind since it isn't meant for continuous, ongoing restoration.

pg_rewind is in the critical path of a bunch of HA scenarios, so I wouldn't
say that restore performance isn't important...

Greetings,

Andres Freund

Here is a new patch set with the following changes:

* The restore_library, archive_cleanup_library, and recovery_end_library
parameters are merged into one parameter.

* restore_library can now be changed via SIGHUP. To provide a way for
modules to clean up when their callbacks are unloaded, I've introduced an
optional shutdown callback.

* Parameter misconfigurations are now always ERRORs. I'm less confident
that we can get by with just a WARNING now that restore_library can be
changed via SIGHUP, and this makes things more consistent with
archive_library/command.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Here is a rebased patch set for cfbot.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Jan 03, 2023 at 09:59:17AM -0800, Nathan Bossart wrote:

Here is a rebased patch set for cfbot.

I noticed that cfbot's Windows tests are failing because the backslashes in
the archive directory path are causing escaping problems. Here is an
attempt to fix that by converting all backslashes to forward slashes, which
is what other tests (e.g., 025_stuck_on_old_timeline.pl) do.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Jan 03, 2023 at 11:05:38AM -0800, Nathan Bossart wrote:

I noticed that cfbot's Windows tests are failing because the backslashes in
the archive directory path are causing escaping problems. Here is an
attempt to fix that by converting all backslashes to forward slashes, which
is what other tests (e.g., 025_stuck_on_old_timeline.pl) do.

+       GetOldestRestartPoint(&restartRedoPtr, &restartTli);
+       XLByteToSeg(restartRedoPtr, restartSegNo, wal_segment_size);
+       XLogFileName(lastRestartPointFname, restartTli, restartSegNo,
+                    wal_segment_size);
+
+       shell_archive_cleanup(lastRestartPointFname);

Hmm. Is passing down the file name used as a cutoff point the best
interface for the modules? Perhaps passing down the redo LSN and its
TLI would be a cleaner approach in terms of flexibility? I agree with
letting the startup enforce these numbers as that can be easy to mess
up for plugin authors, leading to critical problems. The same code
pattern is repeated twice for the end-of-recovery callback and the
cleanup commands when it comes to building the file name. Not
critical, still not really nice.

 MODULES = basic_archive
-PGFILEDESC = "basic_archive - basic archive module"
+PGFILEDESC = "basic_archive - basic archive and recovery module"
"basic_archive" does not reflect what this module does.  Using one
library simplifies the whole configuration picture and the tests, so
perhaps something like basic_wal_module, or something like that, would
be better long-term?
--
Michael

Thanks for taking a look.

On Wed, Jan 11, 2023 at 04:53:39PM +0900, Michael Paquier wrote:

Hmm. Is passing down the file name used as a cutoff point the best
interface for the modules? Perhaps passing down the redo LSN and its
TLI would be a cleaner approach in terms of flexibility? I agree with
letting the startup enforce these numbers as that can be easy to mess
up for plugin authors, leading to critical problems.

I'm having trouble thinking of any practical advantage of providing the
redo LSN and TLI. If the main use-case is removing older archives as the
documentation indicates, it seems better to provide the file name so that
you can plug it straight into strcmp() to determine whether the file can be
removed (i.e., what pg_archivecleanup does). If we provided the LSN and
TLI instead, you'd either need to convert that into a WAL file name for
strcmp(), or you'd need to convert the candidate file name into an LSN and
TLI and compare against those.

"basic_archive" does not reflect what this module does. Using one
library simplifies the whole configuration picture and the tests, so
perhaps something like basic_wal_module, or something like that, would
be better long-term?

I initially created a separate basic_restore module, but decided to fold it
into basic_archive to simplify the patch and tests. I hesitated to rename
it because it already exists in v15, and since it deals with creating and
restoring archive files, the name still seemed somewhat accurate. That
being said, I don't mind renaming it if that's what folks want.

I've attached a new patch set that is rebased over c96de2c. There are no
other changes.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, Jan 11, 2023 at 11:29:01AM -0800, Nathan Bossart wrote:

I'm having trouble thinking of any practical advantage of providing the
redo LSN and TLI. If the main use-case is removing older archives as the
documentation indicates, it seems better to provide the file name so that
you can plug it straight into strcmp() to determine whether the file can be
removed (i.e., what pg_archivecleanup does). If we provided the LSN and
TLI instead, you'd either need to convert that into a WAL file name for
strcmp(), or you'd need to convert the candidate file name into an LSN and
TLI and compare against those.

Logging was one thing that came immediately in mind, to let the module
know the redo LSN and TLI the segment name was built from without
having to recalculate it back. If you don't feel strongly about that,
I am fine to discard this remark. It is not like this hook should be
set in stone across major releases, in any case.

I initially created a separate basic_restore module, but decided to fold it
into basic_archive to simplify the patch and tests. I hesitated to rename
it because it already exists in v15, and since it deals with creating and
restoring archive files, the name still seemed somewhat accurate. That
being said, I don't mind renaming it if that's what folks want.

I've done that in the past for pg_verify_checksums -> pg_checksums, so
I would not mind renaming it so as it reflects better its role.
(Being outvoted is fine for me if this suggestion sounds bad).

Saying that, 0001 seems fine on its own (minus the redo LSN/TLI with
the duplication for the segment name build), so I would be tempted to
get this one done. My gut tells me that we'd better remove the
duplication and just pass down the two fields to
shell_archive_cleanup() and shell_recovery_end(), with the segment
name given to ExecuteRecoveryCommand()..
--
Michael

On Thu, Jan 12, 2023 at 03:30:40PM +0900, Michael Paquier wrote:

On Wed, Jan 11, 2023 at 11:29:01AM -0800, Nathan Bossart wrote:

I initially created a separate basic_restore module, but decided to fold it
into basic_archive to simplify the patch and tests. I hesitated to rename
it because it already exists in v15, and since it deals with creating and
restoring archive files, the name still seemed somewhat accurate. That
being said, I don't mind renaming it if that's what folks want.

I've done that in the past for pg_verify_checksums -> pg_checksums, so
I would not mind renaming it so as it reflects better its role.
(Being outvoted is fine for me if this suggestion sounds bad).

IMHO I don't think there's an urgent need to rename it, but if there's a
better name that people like, I'm happy to do so.

Saying that, 0001 seems fine on its own (minus the redo LSN/TLI with
the duplication for the segment name build), so I would be tempted to
get this one done. My gut tells me that we'd better remove the
duplication and just pass down the two fields to
shell_archive_cleanup() and shell_recovery_end(), with the segment
name given to ExecuteRecoveryCommand()..

I moved the duplicated logic to its own function in v6.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Jan 12, 2023 at 10:17:21AM -0800, Nathan Bossart wrote:

On Thu, Jan 12, 2023 at 03:30:40PM +0900, Michael Paquier wrote:
IMHO I don't think there's an urgent need to rename it, but if there's a
better name that people like, I'm happy to do so.

Okay.

Saying that, 0001 seems fine on its own (minus the redo LSN/TLI with
the duplication for the segment name build), so I would be tempted to
get this one done. My gut tells me that we'd better remove the
duplication and just pass down the two fields to
shell_archive_cleanup() and shell_recovery_end(), with the segment
name given to ExecuteRecoveryCommand()..

I moved the duplicated logic to its own function in v6.

While looking at 0001, I have noticed one issue as of the following
block in shell_restore():

+   if (wait_result_is_signal(rc, SIGTERM))
+       proc_exit(1);
+
+   ereport(wait_result_is_any_signal(rc, true) ? FATAL : DEBUG2,
+           (errmsg("could not restore file \"%s\" from archive: %s",
+                   file, wait_result_to_str(rc))));

This block of code would have been executed even if rc == 0, which is
incorrect because the command would have succeeded. HEAD would not
have done that, actually, as RestoreArchivedFile() would return
before. I guess that you have not noticed it because this basically
just generated incorrect DEBUG2 messages when rc == 0?

One part that this slightly influences is the order of the reports
when the command succeeds the follow-up stat() fails to check the
size, where we reverse these two logs:
DEBUG2, "could not restore"
LOG/FATAL, "could not stat file"

However, that does not really change the value of the information
reported: if the stat() failed, the failure mode is the same except
that we don't get the extra DEBUG2/"could not restore" message, which
does not really matter except if your elevel is high enough for
that and there is always the LOG for that..

Once this issue was fixed, nothing else stood out, so applied this
part.
--
Michael

On Mon, Jan 16, 2023 at 04:36:01PM +0900, Michael Paquier wrote:

Once this issue was fixed, nothing else stood out, so applied this
part.

Thanks! I've attached a rebased version of the rest of the patch set.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Mon, Jan 16, 2023 at 02:40:40PM -0800, Nathan Bossart wrote:

On Mon, Jan 16, 2023 at 04:36:01PM +0900, Michael Paquier wrote:

Once this issue was fixed, nothing else stood out, so applied this
part.

Thanks! I've attached a rebased version of the rest of the patch set.

When it comes to 0002, the only difference between the three code
paths of shell_recovery_end(), shell_archive_cleanup() and
shell_restore() is the presence of BuildRestoreCommand(). However
this is now just a thin wrapper of replace_percent_placeholders() that
does just one extra make_native_path() for the xlogpath.

Could it be cleaner in the long term to remove entirely
BuildRestoreCommand() and move the conversion of the xlogpath with
make_native_path() one level higher in the stack?
--
Michael

On Tue, Jan 17, 2023 at 02:32:03PM +0900, Michael Paquier wrote:

Could it be cleaner in the long term to remove entirely
BuildRestoreCommand() and move the conversion of the xlogpath with
make_native_path() one level higher in the stack?

Yeah, this seems cleaner. I removed BuildRestoreCommand() in v8.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Jan 17, 2023 at 10:23:56AM -0800, Nathan Bossart wrote:

Yeah, this seems cleaner. I removed BuildRestoreCommand() in v8.

        if (*sp == *lp)
        {
-           if (val)
-           {
-               appendStringInfoString(&result, val);
-               found = true;
-           }
-           /* If val is NULL, we will report an error. */
+           appendStringInfoString(&result, val);
+           found = true;

In 0002, this code block has been removed as an effect of the removal
of BuildRestoreCommand(), because RestoreArchivedFile() needs to
handle two flags with two values. The current design has the
advantage to warn extension developers with an unexpected
manipulation, as well, so I have kept the logic in percentrepl.c
as-is.

I was wondering also if ExecuteRecoveryCommand() should use a bits32
for its two boolean flags, but did not bother as it is static in
shell_restore.c so ABI does not matter, even if there are three
callers of it with 75% of the combinations possible (only false/true
is not used).

And 0002 is applied.
--
Michael

On Wed, Jan 18, 2023 at 11:29:20AM +0900, Michael Paquier wrote:

And 0002 is applied.

Thanks. Here's a rebased version of the last patch.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com