On 2020-Apr-24, Kyotaro Horiguchi wrote: > At Thu, 23 Apr 2020 19:16:03 -0400, Alvaro Herrera <alvhe...@2ndquadrant.com> > wrote in
> > 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). > I modestly object to such many call-back functions. FWIW I'm writing > this with [1] in my mind. > [1] > https://www.postgresql.org/message-id/20200422.101246.331162888498679491.horikyota.ntt%40gmail.com Hmm. Looking at your 0001, I think there's nothing in that patch that's not compatible with my proposed API change. 0002 is a completely different story of course; but that patch is a radical change of spirit for xlogreader, in the sense that it's no longer a "reader", but rather just an interpreter of bytes from a WAL byte sequence into WAL records; and shifts the responsibility of the actual reading to the caller. That's why xlogreader no longer receives the page_read callback (and why it doesn't need the segment_open, segment_close callbacks). I have to admit that until today I hadn't realized that that's what your patch series was doing. I'm not familiar with how you intend to implement WAL encryption on top of this, but on first blush I'm not liking this proposed design too much. > 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. Well, the complaint is that the current layering is weird, in that there are two levels at which we pass callbacks: one is XLogReaderAllocate, where you specify the page_read callback; and the other layer is inside the page_read callback, if that layer uses the WALRead auxiliary function. The thing that my patch is doing is pass all three callbacks at the XLogReaderAllocate level. So when xlogreader drills down to read_page, xlogreader already has the segment_open callback handy if it needs it. Conceptually, this seems more sensible. I think a "cleanup" callback might also be sensible in general terms, but we have a problem with the specifics -- namely that the "state" that we need to clean up (the file descriptor of the open segment) is part of xlogreader's state. And we obviously cannot remove the FD from XLogReaderState, because when we need the FD to do things with it to obtain data from the file. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
