Re: recovery modules

On Fri, Jan 27, 2023 at 04:23:19PM -0800, Andres Freund wrote:
>> +typedef bool (*RecoveryRestoreCB) (const char *file, const char *path,
>> + const char *lastRestartPointFileName);
>> +typedef void (*RecoveryArchiveCleanupCB) (const char *lastRestartPointFileName);
>> +typedef void (*RecoveryEndCB) (const char *lastRestartPointFileName);
>> +typedef void (*RecoveryShutdownCB) (void);
>
> I think the signature of these forces bad coding practices, because there's no
> way to have state within a recovery module (as there's no parameter for it).
>
> It's possible we would eventually support multiple modules, e.g. restoring
> from shorter term file based archiving and from longer term archiving in some
> blob store. Then we'll regret not having a varible for this.

Are you suggesting that we add a "void *arg" to each one of these? Or put
the arguments into a struct? Or something else?

>> +extern RecoveryModuleCallbacks RecoveryContext;
>
> I think that'll typically be interpreteted as a MemoryContext by readers.

How about RecoveryCallbacks?

> Also, why is this a global var? Exported too?

It's needed in xlog.c, xlogarchive.c, and xlogrecovery.c. Would you rather
it be static to xlogarchive.c and provide accessors for the others?

>> +/*
>> + * Type of the shared library symbol _PG_recovery_module_init that is looked up
>> + * when loading a recovery library.
>> + */
>> +typedef void (*RecoveryModuleInit) (RecoveryModuleCallbacks *cb);
>
> I think this is a bad way to return callbacks. This way the
> RecoveryModuleCallbacks needs to be modifiable, which makes the job for the
> compiler harder (and isn't the greatest for security).
>
> I strongly encourage you to follow the model used e.g. by tableam. The init
> function should return a pointer to a *constant* struct. Which is compile-time
> initialized with the function pointers.
>
> See the bottom of heapam_handler.c for how that ends up looking.

Hm. I used the existing strategy for archive modules and logical decoding
output plugins here. I think it would be weird for the archive module and
recovery module interfaces to look so different, but if that's okay, I can
change it.

>> +void
>> +LoadRecoveryCallbacks(void)
>> +{
>> + RecoveryModuleInit init;
>> +
>> + /*
>> + * If the shell command is enabled, use our special initialization
>> + * function. Otherwise, load the library and call its
>> + * _PG_recovery_module_init().
>> + */
>> + if (restoreLibrary[0] == '\0')
>> + init = shell_restore_init;
>> + else
>> + init = (RecoveryModuleInit)
>> + load_external_function(restoreLibrary, "_PG_recovery_module_init",
>> + false, NULL);
>
> Why a special rule for shell, instead of just defaulting the GUC to it?

I'm not following this one. The default value of the restore_library GUC
is an empty string, which means that the shell commands should be used.

>> + /*
>> + * If using shell commands, remove callbacks for any commands that are not
>> + * set.
>> + */
>> + if (restoreLibrary[0] == '\0')
>> + {
>> + if (recoveryRestoreCommand[0] == '\0')
>> + RecoveryContext.restore_cb = NULL;
>> + if (archiveCleanupCommand[0] == '\0')
>> + RecoveryContext.archive_cleanup_cb = NULL;
>> + if (recoveryEndCommand[0] == '\0')
>> + RecoveryContext.recovery_end_cb = NULL;
>
> I'd just mandate that these are implemented and that the module has to handle
> if it doesn't need to do anything.

Wouldn't this just force module authors to write empty functions for the
parts they don't need?

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