Re: [COMMITTERS] pgsql: Add comments about why errno is set to zero.

Log Message:
-----------
Add comments about why errno is set to zero.

Modified Files:
--------------
pgsql/src/backend/utils/adt:
datetime.c (r1.162 -> r1.163)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/datetime.c.diff?r1=1.162&r2=1.163)
float.c (r1.116 -> r1.117)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/float.c.diff?r1=1.116&r2=1.117)

momjian(at)postgresql(dot)org (Bruce Momjian) writes:
> Log Message:
> -----------
> Add comments about why errno is set to zero.

These comments seem a bit wrongheaded, since "checking
LONG_MIN/LONG_MAX" is exactly not what we could do to detect an overflow
error.

regards, tom lane

Tom Lane wrote:
> momjian(at)postgresql(dot)org (Bruce Momjian) writes:
> > Log Message:
> > -----------
> > Add comments about why errno is set to zero.
>
> These comments seem a bit wrongheaded, since "checking
> LONG_MIN/LONG_MAX" is exactly not what we could do to detect an overflow
> error.

Yea, I noticed the 0 was listed as another value that needs to be
checked. Should I just change them all to:

errno = 0; /* avoid checking result for failure */

or should I add a macro to c.h as:

/* Sometimes we need to clear errno so we can check errno
* without having to check for a failure value from the function
* call.
*/
#define CLEAR_ERRNO \\
do { \
errno = 0; \\
while (0);

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Should I just change them all to:

> errno = 0; /* avoid checking result for failure */

No, that's still a completely inaccurate description of the reason
for having the statement.

> or should I add a macro to c.h as:

> /* Sometimes we need to clear errno so we can check errno
> * without having to check for a failure value from the function
> * call.
> */
> #define CLEAR_ERRNO \\
> do { \
> errno = 0; \\
> while (0);

I vote "neither". Anyone who doesn't understand what this is for will
need to go read the C library man pages for a bit anyway. Nor do I find
"CLEAR_ERRNO" an improvement over "errno = 0".

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Should I just change them all to:
>
> > errno = 0; /* avoid checking result for failure */
>
> No, that's still a completely inaccurate description of the reason
> for having the statement.
>
> > or should I add a macro to c.h as:
>
> > /* Sometimes we need to clear errno so we can check errno
> > * without having to check for a failure value from the function
> > * call.
> > */
> > #define CLEAR_ERRNO \\
> > do { \
> > errno = 0; \\
> > while (0);
>
> I vote "neither". Anyone who doesn't understand what this is for will
> need to go read the C library man pages for a bit anyway. Nor do I find
> "CLEAR_ERRNO" an improvement over "errno = 0".

Well, there seems to be enough confusion, even in this email list, that
identifying _why_ errno is being cleared is a good idea.

I modified it to:

errno = 0; /* avoid having to check the result for failure */

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:
> Tom Lane wrote:

> > > or should I add a macro to c.h as:
> >
> > > /* Sometimes we need to clear errno so we can check errno
> > > * without having to check for a failure value from the function
> > > * call.
> > > */
> > > #define CLEAR_ERRNO \\
> > > do { \
> > > errno = 0; \\
> > > while (0);

May I vote against this kind of use of macros in general? It doesn't
add much value (actually, none in this case) and it makes the code
harder to read. For a pathological example I can point to PHP, which is
so full of strange macros that it's very very hard to read.

Of course there are places where macros are valuable tools, but this
doesn't seem to be one of them.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Thu, Dec 01, 2005 at 04:12:30PM -0500, Bruce Momjian wrote:
> Well, there seems to be enough confusion, even in this email list, that
> identifying _why_ errno is being cleared is a good idea.
>
> I modified it to:
>
> errno = 0; /* avoid having to check the result for failure */

I don't know about others but I find that wording ambiguous. Like it's
saying that once you've done that it can't fail. I think I'd prefer
something like:

errno = 0; /* Make error condition detectable */

or even

errno = 0; /* clear pending errors */

or

errno = 0; /* clear prior detected errors */

YMMV,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

Martijn van Oosterhout wrote:
-- Start of PGP signed section.
> On Thu, Dec 01, 2005 at 04:12:30PM -0500, Bruce Momjian wrote:
> > Well, there seems to be enough confusion, even in this email list, that
> > identifying _why_ errno is being cleared is a good idea.
> >
> > I modified it to:
> >
> > errno = 0; /* avoid having to check the result for failure */
>
> I don't know about others but I find that wording ambiguous. Like it's
> saying that once you've done that it can't fail. I think I'd prefer
> something like:
>
> errno = 0; /* Make error condition detectable */
>
> or even
>
> errno = 0; /* clear pending errors */
>
> or
>
> errno = 0; /* clear prior detected errors */

Maybe it should be:

errno = 0; /* Allow unconditional errno check */

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Thu, 2005-12-01 at 16:38 -0500, Bruce Momjian wrote:
> Maybe it should be:
>
> errno = 0; /* Allow unconditional errno check */

I think any solution that involves adding more duplication at each
strtol() callsite is not great ("Don't Repeat Yourself"). I'd still like
to see this refactored into a separate function, as I suggested on
-patches. If people would like to see a detailed explanation of the
interaction between strtol() and errno, a header comment to pg_strtol()
seems a good place to put it. IMO that is better than copying and
pasting a cryptic one-line comment to each and every callsite of
strtol().

-Neil

Neil Conway <neilc(at)samurai(dot)com> writes:
> If people would like to see a detailed explanation of the
> interaction between strtol() and errno, a header comment to pg_strtol()
> seems a good place to put it. IMO that is better than copying and
> pasting a cryptic one-line comment to each and every callsite of
> strtol().

Next we'll be copying-and-pasting entire C-library man pages, no doubt.
I think this whole discussion is a waste of electrons, as are the
proposed comments. No one ever asked for extra documentation in the
original coding in pg_atoi, or the other dozen or so places where we
have historically checked the result of strtol. Why do we suddenly
feel it requires extra doc now?

regards, tom lane

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> I modified it to:
> errno = 0; /* avoid having to check the result for failure */

Just for the record, that's *still* wrong. It implies that if we
tested (result == LONG_MAX && errno == ERANGE), without zeroing
errno beforehand, the code would be correct. But it would not,
because the errno value could still be leftover. The plain fact
of the matter is that if you're going to check for strtol overflow at
all, you have to zero errno beforehand. This is perfectly well
explained in the strtol spec page, and I see no need to duplicate it:

Because 0, LONG_MIN and LONG_MAX are returned on error and are
also valid returns on success, an application wishing to check
for error situations should set errno to 0, then call strtol(),
then check errno.

regards, tom lane

Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> errno = 0; /* clear prior detected errors */

That one is at least a correct explanation of what the code is doing...

regards, tom lane

OK, comments removed, and comment added to port/strtol.c.

---------------------------------------------------------------------------

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > I modified it to:
> > errno = 0; /* avoid having to check the result for failure */
>
> Just for the record, that's *still* wrong. It implies that if we
> tested (result == LONG_MAX && errno == ERANGE), without zeroing
> errno beforehand, the code would be correct. But it would not,
> because the errno value could still be leftover. The plain fact
> of the matter is that if you're going to check for strtol overflow at
> all, you have to zero errno beforehand. This is perfectly well
> explained in the strtol spec page, and I see no need to duplicate it:
>
> Because 0, LONG_MIN and LONG_MAX are returned on error and are
> also valid returns on success, an application wishing to check
> for error situations should set errno to 0, then call strtol(),
> then check errno.
>
> regards, tom lane
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073