Re: Fix some error handling for read() and errno

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
Date: 2018-07-14 04:50:02
Message-ID: 20180714045002.nypdxknt6cavgocc@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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 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
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

In response to


Browse pgsql-hackers by date

  From Date Subject
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