Re: Comments on Custom RMGRs

On Thu, 12 May 2022 at 04:40, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> On 2022-05-11 09:39:48 -0700, Jeff Davis wrote:
> > On Wed, 2022-05-11 at 15:24 +0100, Simon Riggs wrote:
> > > [PATCH: rmgr_001.v1.patch]
> > >
> > > [PATCH: rmgr_002.v1.patch]
> >
> > Thank you. Both of these look like good ideas, and I will commit them
> > in a few days assuming that nobody else sees a problem.
>
> What exactly is the use case here? Without passing in information about
> whether recovery will be performed etc, it's not at all clear how callbacks
> could something useful?

Sure, happy to do it that way.
[PATCH: rmgr_002.v2.patch]

> I don't think we should allocate a bunch of memory contexts to just free them
> immediately after?

Didn't seem a problem, but I've added code to use the flag requested above.

> > > It occurs to me that any use of WAL presumes that Checkpoints exist
> > > and do something useful. However, the custom rmgr interface doesn't
> > > allow you to specify any actions on checkpoint, so ends up being
> > > limited in scope. So I think we also need an rm_checkpoint() call -
> > > which would be a no-op for existing rmgrs.
> > > [PATCH: rmgr_003.v1.patch]
> >
> > I also like this idea, but can you describe the intended use case? I
> > looked through CheckPointGuts() and I'm not sure what else a custom AM
> > might want to do. Maybe sync special files in a way that's not handled
> > with RegisterSyncRequest()?
>
> I'm not happy with the idea of random code being executed in the middle of
> CheckPointGuts(), without any documentation of what is legal to do at that
> point.

The "I'm not happy.." ship has already sailed with pluggable rmgrs.

Checkpoints exist for one purpose - as the starting place for recovery.

Why would we allow pluggable recovery without *also* allowing
pluggable checkpoints?

>To actually be useful we'd likely need multiple calls to such an rmgr
> callback, with a parameter where in CheckPointGuts() we are. Right now the
> sequencing is explicit in CheckPointGuts(), but with the proposed callback,
> that'd not be the case anymore.

It is useful without the extra complexity you mention.

I see multiple uses for the rm_checkpoint() point proposed and I've
been asked multiple times for a checkpoint hook. Any rmgr that
services crash recovery for a non-smgr based storage system would need
this because the current checkpoint code only handles flushing to disk
for smgr-based approaches. That is orthogonal to other code during
checkpoint, so it stands alone quite well.

--
Simon Riggs http://www.EnterpriseDB.com/