Re: pg13: xlogreader API adjust

At Mon, 11 May 2020 16:33:36 -0400, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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.fpzdb5yyb7qyhqko@alap3.anarazel.de
> [2] https://postgr.es/m/20200508.114228.963995144765118400.horikyota.ntt@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