|From:||Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>|
|To:||Michael Paquier <michael(at)paquier(dot)xyz>|
|Cc:||Postgres hackers <pgsql-hackers(at)postgresql(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com>, Magnus Hagander <magnus(at)hagander(dot)net>, Robbie Harwood <rharwood(at)redhat(dot)com>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, hlinnaka <hlinnaka(at)iki(dot)fi>|
|Subject:||Re: Fix some error handling for read() and errno|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On 2018-Jul-14, Michael Paquier wrote:
> On Fri, Jul 13, 2018 at 11:31:31AM -0400, Alvaro Herrera wrote:
> >> Mr. Robot has been complaining about this patch set, so attached is a
> >> rebased version. Thinking about it, I would tend to just merge 0001 and
> >> give up on 0002 as that may not justify future backpatch pain. Thoughts
> >> are welcome.
> > I vote to push both.
> Thanks! Did you look at the code? The first patch is just some
> cleanup, while the second could have adjustments? For the second I went
> with the minimal amount of work, and actually there is no need to make
> ReadTransientFile() return a status per my study of ReadTwoPhaseFile()
> in https://postgr.es/m/20180709050309.GM1467@paquier.xyz which must fail
> when reading the file. So patch 0002 depends on the other 2PC patch.
I did read them, though not in minute detail. 0002 seems to result in
code easier to read. If there are particular places that deviate from
the obvious patterns, I didn't notice them.
In 0001 one thing I wasn't terribly in love with was random deviations
in sprintf format specifiers for things that should be identical, ie.
%lu in some places and %zu in others, for "read only %d of %d". It
seems you should pick the more general one (%zu) and use casts to Size
(or is it size_t?) in the places that have other types. That way you
*really* decrease translator effort to the bare minimum :-)
Ah, in 0001 you have one case of "could not read _from_" (in
SimpleXLogPageRead). The "from" is not present in the other places.
I'm not sure about putting the wait event stuff inside the new
functions. It looks odd, though I understand why you did it.
No opinion on the 2PC stuff -- didn't look at that.
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
|Next Message||Michael Paquier||2018-07-14 06:37:56||Re: Fix some error handling for read() and errno|
|Previous Message||Alvaro Herrera||2018-07-14 04:28:10||Re: [HACKERS] Small patch for pg_basebackup argument parsing|