On 2019-Nov-25, Antonin Houska wrote: > Alvaro Herrera <alvhe...@2ndquadrant.com> wrote:
> > I see no reason to leave ws_off. We can move that to XLogReaderState; I > > did that here. We also need the offset in WALReadError, though, so I > > added it there too. Conceptually it seems clearer to me this way. > > > > What do you think of the attached? > > It looks good to me. Attached is just a fix of a minor problem in error > reporting that Michael pointed out earlier. Excellent, I pushed it with this change included and some other cosmetic changes. Now there's only XLogPageRead() ... > > BTW I'm not clear what errors can pread()/pg_pread() report that do not > > set errno. I think lines 1083/1084 of WALRead are spurious now. > > All I can say is that the existing calls of pg_pread() do not clear errno, so > you may be right. Right ... in this interface, we only report an error if pg_pread() returns negative, which is documented to always set errno. > I'd appreciate more background about the "partial read" that > Michael mentions here: > > https://www.postgresql.org/message-id/20191125033048.GG37821%40paquier.xyz In the current implementation, if pg_pread() does a partial read, we just loop one more time. I considered changing the "if (readbytes <= 0)" with "if (readbytes < segbytes)", but that seemed pointless. However, writing this now makes me think that we should add a CHECK_FOR_INTERRUPTS in this loop. (I also wonder if we shouldn't limit the number of times we retry if pg_pread returns zero (i.e. no error, but no bytes read either). I don't know if this is a real-world consideration.) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
