From: Nathan Bossart <nathandbossart@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@postgresql.org
Subject: Re: recovery modules
Date: Fri, 27 Jan 2023 16:59:10 -0800
Message-ID: <20230128005910.GA2245287@nathanxps13> (raw)
In-Reply-To: <20230128002319.362oxrxf7ardfz2a@awork3.anarazel.de>
References: <Y8T+Yb3oB1wCdEQF@paquier.xyz>
<20230116224040.GB2714038@nathanxps13>
<Y8Yy01LBF12k9755@paquier.xyz>
<20230117182356.GA3015764@nathanxps13>
<Y8dZgIeqsLe6NP6B@paquier.xyz>
<20230118044427.GA3369836@nathanxps13>
<Y830qTRZJP6XGuOF@paquier.xyz>
<20230123214428.GA572995@nathanxps13>
<Y9Dbfe3C0wGJqyav@paquier.xyz>
<20230128002319.362oxrxf7ardfz2a@awork3.anarazel.de>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, andres@anarazel.de, michael@paquier.xyz
Subject: Re: recovery modules
In-Reply-To: <20230128005910.GA2245287@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox