On Sat, Jul 14, 2018 at 12:50:02AM -0400, Alvaro Herrera wrote:
> 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 :-)
Yeah, that would be really the point, hence let's use "could not read
file \"%s\": read %d of %zu" everywhere with a cast to Size. That's
used in some other places as well.
> 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.
Right.
> I'm not sure about putting the wait event stuff inside the new
> functions. It looks odd, though I understand why you did it.
I don't really find that strange as any transient files should be
tracked by I/O wait events :)
> No opinion on the 2PC stuff -- didn't look at that.
For now, I think that just moving forward with 0001, and then revisit
0002 once the other 2PC patch is settled makes the most sense. On the
other thread, the current 2PC behavior can create silent data loss so
I would like to back-patch it, so that would be less work.
--
Michael