On 2020-May-12, Kyotaro Horiguchi wrote:
> 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.
Yeah, I couldn't decide which style I liked the most. I used the one
you suggested.
> + 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.
I tried that. I was about to leave it at just modifying physical
walsender (simple enough, and it passed tests), but I noticed that
WalSndErrorCleanup() would be a problem because we don't know if it's
physical or logical walsender. So in the end I added a global
'xlogreader' pointer in walsender.c -- logical walsender sets it to the
true xlogreader it has inside the logical decoding context, and physical
walsender sets it to its fake xlogreader. That seems to work nicely.
sendSeg/sendCxt are gone entirely. Logical walsender was doing
WALOpenSegmentInit() uselessly during InitWalSender(), since it was
using the separate sendSeg/sendCxt structs instead of the ones in its
xlogreader. (Some mysteries become clearer!)
It's slightly disquieting that the segment_close call in
WalSndErrorCleanup is not covered, but in any case this should work well
AFAICS. I think this is simpler to understand than formerly.
Now the only silliness remaining is the fact that different users of the
xlogreader interface are doing different things about the TLI.
Hopefully we can unify everything to something sensible one day .. but
that's not going to happen in pg13.
I'll get this pushed tomorrow, unless there are further objections.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services