At Thu, 23 Apr 2020 19:16:03 -0400, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote in > On 2020-Apr-22, Andres Freund wrote: > > > I'm in favor of doing so. Not necessarily primarily to avoid repeated > > API changes, but because I don't think the v13 changes went in the quite > > right direction. > > > > ISTM that we should: > > - have the three callbacks you mention above > > - change WALSegmentOpen to also get the XLogReaderState > > - add private state to WALOpenSegment, so it can be used even when not > > accessing data in files / when one needs more information to close the > > file. > > - disambiguate between WALOpenSegment (struct describing an open > > segment) and WALSegmentOpen (callback to open a segment) (note that > > the read page callback uses a *CB naming, why not follow?) > > Here's a first attempt at that. The segment_open/close callbacks are > now given at XLogReaderAllocate time, and are passed the XLogReaderState > pointer. I wrote a comment to explain that the page_read callback can > use WALRead() if it wishes to do so; but if it does, then segment_open > has to be provided. segment_close is mandatory (since we call it at > XLogReaderFree). > > Of the half a dozen cases that exist, three are slightly weird: > > * Physical walsender does not use a xlogreader at all. I think we could > beat that code up so that it does. But for the moment I just cons up > a fake xlogreader, which only has the segment_open pointer set up, so > that it can call WALRead. > > * main xlog.c uses an xlogreader with XLogPageRead(), which does not use > WALRead. Therefore it does not pass open_segment. It does not use > xlogreader->seg.ws_file either. Eventually we may want to beat this > one up also. > > * pg_rewind has its own page read callback, SimpleXLogPageRead, which > does all the required opening and closing. I don't think it'd be an > improvement to force this to use segment_open. Oddly enough, it calls > itself "simple" but is unique in having the ability to read files from > the wal archive. > > All tests are passing for me.
I modestly object to such many call-back functions. FWIW I'm writing this with [1] in my mind. An open-callback is bound to a read-callback. A close-callback is bound to the way the read-callback opens a segment (or the open-callback). I'm afraid that only adding "cleanup" callback might be sufficient. [1] https://www.postgresql.org/message-id/20200422.101246.331162888498679491.horikyota.ntt%40gmail.com regards. -- Kyotaro Horiguchi NTT Open Source Software Center
