Date: Wed, 11 May 2022 20:40:10 -0700
From: Andres Freund <andres@anarazel.de>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Simon Riggs <simon.riggs@enterprisedb.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Comments on Custom RMGRs
Message-ID: <20220512034010.4oqa76pasrulkw32@alap3.anarazel.de>
References: 
 <CANbhV-E4pTWeF-DsdaGsOrjJNFWPaR+Dstjrnkqvf9JFFgOKKQ@mail.gmail.com>
 <f781f8f198567cee9e3776487bab3e41a7b91b82.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f781f8f198567cee9e3776487bab3e41a7b91b82.camel@j-davis.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220512034010.4oqa76pasrulkw32%40alap3.anarazel.de>
Precedence: bulk

Hi,

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?

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


> > 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. 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.

Greetings,

Andres Freund