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.
Looks odd.
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
