At Mon, 11 May 2020 16:33:36 -0400, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote in > Hello > > Per discussion in thread [1], I propose the following patch to give > another adjustment to the xlogreader API. This results in a small but > not insignificat net reduction of lines of code. What this patch does > is adjust the signature of these new xlogreader callbacks, making the > API simpler. The changes are: > > * the segment_open callback installs the FD in xlogreader state itself, > instead of passing the FD back. This was suggested by Kyotaro > Horiguchi in that thread[2]. > > * We no longer pass segcxt to segment_open; it's in XLogReaderState, > which is already an argument. > > * We no longer pass seg/segcxt to WALRead; instead, that function takes > them from XLogReaderState, which is already an argument. > (This means XLogSendPhysical has to drink more of the fake_xlogreader > kool-aid.) > > I claim the reason to do it now instead of pg14 is to make it simpler > for third-party xlogreader callers to adjust. > > (Some might be thinking that I do this to avoid an API change later, but > my guts tell me that we'll adjust xlogreader again in pg14 for the > encryption stuff and other reasons, so.) > > > [1] https://postgr.es/m/20200406025651.fpzdb5yyb7qyh...@alap3.anarazel.de > [2] > https://postgr.es/m/20200508.114228.963995144765118400.horikyota....@gmail.com
The simplified interface of WALRead looks far better to me since it no longer has unreasonable duplicates of parameters. I agree to the discussion about third-party xlogreader callers but not sure about back-patching burden. I'm not sure the reason for wal_segment_open and WalSndSegmentOpen being modified different way about error handling of BasicOpenFile, I prefer the WalSndSegmentOpen way. However, that difference doesn't harm anything so I'm fine with the current patch. + fake_xlogreader.seg = *sendSeg; + fake_xlogreader.segcxt = *sendCxt; fake_xlogreader.seg is a different instance from *sendSeg. WALRead modifies fake_xlogreader.seg but does not modify *sendSeg. Thus the change doesn't persist. On the other hand WalSndSegmentOpen reads *sendSeg, which is not under control of WALRead. Maybe we had better to make fake_xlogreader be a global variable of walsender.c that covers the current sendSeg and sendCxt. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
