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.gm1...@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