Date: Thu, 26 Jan 2023 21:40:58 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@postgresql.org
Subject: Re: recovery modules
Message-ID: <20230127054058.GA2041427@nathanxps13>
References: <20230112181721.GA2103226@nathanxps13>
 <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y9Dbfe3C0wGJqyav@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/20230127054058.GA2041427%40nathanxps13>
Precedence: bulk

On Wed, Jan 25, 2023 at 04:34:21PM +0900, Michael Paquier wrote:
> The loop part is annoying..  I've never been a fan of adding this
> cross-value checks for the archiver modules in the first place, and it
> would make things much simpler in the checkpointer if we need to think
> about that as we want these values to be reloadable.  Perhaps this
> could just be an exception where we just give priority on one over the
> other archive_cleanup_command?  The startup process has a well-defined
> sequence after a failure, while the checkpointer is designed to remain
> robust.

Yeah, there are some problems here.  If we ERROR, we'll just bounce back to
the sigsetjmp() block once a second, and we'll never pick up configuration
reloads, shutdown signals, etc.  If we FATAL, we'll just rapidly restart
over and over.  Given the dicussion about misconfigured archiving
parameters [0], I doubt folks will be okay with giving priority to one or
the other.

I'm currently thinking that the checkpointer should set a flag and clear
the recovery callbacks when a misconfiguration is detected.  Anytime the
checkpointer tries to use the archive-cleanup callback, a WARNING would be
emitted.  This is similar to an approach I proposed for archiving
misconfigurations (that we didn't proceed with) [1].  Given the
aformentioned problems, this approach might be more suitable for the
checkpointer than it is for the archiver.

Thoughts?

[0] https://postgr.es/m/9ee5d180-2c32-a1ca-d3d7-63a723f68d9a%40enterprisedb.com
[1] https://postgr.es/m/20220914222736.GA3042279%40nathanxps13

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