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

Hi,

On 2022-05-12 22:26:51 +0100, Simon Riggs wrote:
> On Thu, 12 May 2022 at 04:40, Andres Freund <andres@anarazel.de> wrote:
> > 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.

I don't agree. The ordering within a checkpoint is a lot more fragile than
what you do in an individual redo routine.


> Checkpoints exist for one purpose - as the starting place for recovery.
> 
> Why would we allow pluggable recovery without *also* allowing
> pluggable checkpoints?

Because one can do a lot of stuff with just pluggable WAL records, without
integrating into checkpoints?

Note that I'm *not* against making checkpoint extensible - I just think it
needs a good bit of design work around when the hook is called etc.


I definitely think it's too late in the cycle to add checkpoint extensibility
now.


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

Shrug. The documentation work definitely is needed. Perhaps we can get away
without multiple callbacks within a checkpoint, I think it'll become more
apparent when writing information about the precise point in time the
checkpoint callback is called.


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

FWIW, for that there are much bigger problems than checkpoint
extensibility. Most importantly there's currently no good way to integrate
relation creation / drop with the commit / abort infrastructure...

Greetings,

Andres Freund