CVS tip problems

Oliver Elphick <olly@lfix.co.uk> writes:

1. There are regression failures on timestamptz and horology which seem
to have come about either on input or output of timestamps with
fractional seconds.

Hmm ... I'm seeing slightly different misbehaviors in the
integer-datetimes and float-datetimes cases. I think we are about to
have to dig down to the root of the off-by-one-second cases that Tom
Lockhart noticed but never really solved. I suspect that the issue has
to do with platform-specific rounding of negative values to integer, but
I haven't got the full story quite yet.

I tried debugging this but got a segmentation fault and apparent stack
corruption in gdb, with the reported break point not anywhere I had set
one. I don't know what to do about that.

Recompiling with -O0 might help debuggability.

2. If the postmaster is not running, there is garbage in psql's error
message:

Works OK for me --- could you trace that one more fully?

3. There is a compilation warning that a constant will not fit into a
long in adt.c. There are two more files where INT64CONST() is required
but not supplied. Patch attached.

Not sure about these ... does anyone else get these warnings?

regards, tom lane

Oliver Elphick <olly@lfix.co.uk> writes:

1. There are regression failures on timestamptz and horology which seem
to have come about either on input or output of timestamps with
fractional seconds.

I believe I've fixed this.

2. If the postmaster is not running, there is garbage in psql's error
message:

I can't duplicate that here. It looks to me like the probable
explanation is a broken or incompatible version of strerror_r() on your
machine. Does the failure go away if you build without thread-safety?

3. There is a compilation warning that a constant will not fit into a
long in adt.c. There are two more files where INT64CONST() is required
but not supplied. Patch attached.

Patch applied, thanks.

regards, tom lane

On Mon, 2004-05-31 at 19:55, Tom Lane wrote:

Oliver Elphick <olly@lfix.co.uk> writes:

1. There are regression failures on timestamptz and horology which seem
to have come about either on input or output of timestamps with
fractional seconds.

I believe I've fixed this.

All regression tests pass now.

2. If the postmaster is not running, there is garbage in psql's error
message:

I can't duplicate that here. It looks to me like the probable
explanation is a broken or incompatible version of strerror_r() on your
machine. Does the failure go away if you build without thread-safety?

Yes it does.

I'll see if I can run with a debugging libc and find it.

--
Oliver Elphick olly@lfix.co.uk
Isle of Wight http://www.lfix.co.uk/oliver
GPG: 1024D/A54310EA 92C8 39E7 280E 3631 3F0E 1EC0 5664 7A2F A543 10EA
========================================
"How precious also are thy thoughts unto me, O God! how
great is the sum of them! If I should count them, they
are more in number than the sand; when I awake, I am
still with thee." Psalms 139: 17,18

Oliver Elphick <olly@lfix.co.uk> writes:

On Mon, 2004-05-31 at 19:55, Tom Lane wrote:

I can't duplicate that here. It looks to me like the probable
explanation is a broken or incompatible version of strerror_r() on your
machine. Does the failure go away if you build without thread-safety?

Yes it does.
I'll see if I can run with a debugging libc and find it.

First you might want to check which flavor of strerror_r() your platform
has --- does it return int or char* ? The Linux man page for
strerror_r() says

strerror_r() with prototype as given above is specified by SUSv3, and
was in use under Digital Unix and HP Unix. An incompatible function,
with prototype

char *strerror_r(int errnum, char *buf, size_t n);

is a GNU extension used by glibc (since 2.0), and must be regarded as
obsolete in view of SUSv3. The GNU version may, but need not, use the
user-supplied buffer. If it does, the result may be truncated in case
the supplied buffer is too small. The result is always NUL-terminated.

The code we have appears to assume that the result will always be placed
in the user-supplied buffer, which is apparently NOT what the glibc
version does.

regards, tom lane

Tom Lane wrote:

Oliver Elphick <olly@lfix.co.uk> writes:

On Mon, 2004-05-31 at 19:55, Tom Lane wrote:

I can't duplicate that here. It looks to me like the probable
explanation is a broken or incompatible version of strerror_r() on your
machine. Does the failure go away if you build without thread-safety?

Yes it does.
I'll see if I can run with a debugging libc and find it.

First you might want to check which flavor of strerror_r() your platform
has --- does it return int or char* ? The Linux man page for
strerror_r() says

strerror_r() with prototype as given above is specified by SUSv3, and
was in use under Digital Unix and HP Unix. An incompatible function,
with prototype

char *strerror_r(int errnum, char *buf, size_t n);

is a GNU extension used by glibc (since 2.0), and must be regarded as
obsolete in view of SUSv3. The GNU version may, but need not, use the
user-supplied buffer. If it does, the result may be truncated in case
the supplied buffer is too small. The result is always NUL-terminated.

The code we have appears to assume that the result will always be placed
in the user-supplied buffer, which is apparently NOT what the glibc
version does.

What does "may, but need not, use the user-supplied buffer" supposed to
mean in practical terms. How do they expect us to use it?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.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@candle.pha.pa.us> writes:

The code we have appears to assume that the result will always be placed
in the user-supplied buffer, which is apparently NOT what the glibc
version does.

What does "may, but need not, use the user-supplied buffer" supposed to
mean in practical terms. How do they expect us to use it?

AFAICS they expect you to use the function's return value.

The current PG code is really erroneous for *both* strerror_r specs,
since the SUS-spec version doesn't promise to put anything into the
buffer if it returns a failure code. I think you will have to write
some autoconf code to detect which return type is provided, and do
the right things with the return value in both cases.

regards, tom lane

On Tue, 2004-06-01 at 01:33, Tom Lane wrote:

First you might want to check which flavor of strerror_r() your platform
has --- does it return int or char* ? The Linux man page for
strerror_r() says

From the definition in /usr/include/string.h, glibc 2.3.2 still has the

version that returns char*

--
Oliver Elphick olly@lfix.co.uk
Isle of Wight http://www.lfix.co.uk/oliver
GPG: 1024D/A54310EA 92C8 39E7 280E 3631 3F0E 1EC0 5664 7A2F A543 10EA
========================================
"Thou will show me the path of life; in thy presence
is fullness of joy; at thy right hand there are
pleasures for evermore." Psalms 16:11

On Tue, 2004-06-01 at 01:33, Tom Lane wrote:

First you might want to check which flavor of strerror_r() your platform
has --- does it return int or char* ?

I made the following change to the strerror_r call, which makes it work
correctly with threading enabled:

--- src/port/thread.c   23 Apr 2004 18:15:55 -0000      1.20
+++ src/port/thread.c   1 Jun 2004 07:18:26 -0000
@@ -71,7 +71,8 @@
 #if defined(FRONTEND) && defined(ENABLE_THREAD_SAFETY) && defined(HAVE_STRERROR_R)
        /* reentrant strerror_r is available */
        /* some early standards had strerror_r returning char * */
-       strerror_r(errnum, strerrbuf, buflen);
+       char buf[256];
+       StrNCpy(strerrbuf, strerror_r(errnum, buf, 256), buflen);
        return strerrbuf;

#else

(I realise this is not sufficient for a patch to correct the problem.)
--
Oliver Elphick olly@lfix.co.uk
Isle of Wight http://www.lfix.co.uk/oliver
GPG: 1024D/A54310EA 92C8 39E7 280E 3631 3F0E 1EC0 5664 7A2F A543 10EA
========================================
"Thou will show me the path of life; in thy presence
is fullness of joy; at thy right hand there are
pleasures for evermore." Psalms 16:11

Oliver Elphick <olly@lfix.co.uk> writes:

-       strerror_r(errnum, strerrbuf, buflen);
+       char buf[256];
+       StrNCpy(strerrbuf, strerror_r(errnum, buf, 256), buflen);
return strerrbuf;

Easier and safer would be

-       strerror_r(errnum, strerrbuf, buflen);
-       return strerrbuf;
+       return strerror_r(errnum, strerrbuf, buflen);

The real point here is that we need to code differently depending on
which flavor of strerror_r we have.

regards, tom lane

This patch fixes strerror_r() by checking the return type from
configure.

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

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

The code we have appears to assume that the result will always be placed
in the user-supplied buffer, which is apparently NOT what the glibc
version does.

What does "may, but need not, use the user-supplied buffer" supposed to
mean in practical terms. How do they expect us to use it?

AFAICS they expect you to use the function's return value.

The current PG code is really erroneous for *both* strerror_r specs,
since the SUS-spec version doesn't promise to put anything into the
buffer if it returns a failure code. I think you will have to write
some autoconf code to detect which return type is provided, and do
the right things with the return value in both cases.

regards, tom lane

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.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@candle.pha.pa.us> writes:

! #ifdef STRERROR_R_INT
! /* SUSv3 version */
! if (strerror_r(errnum, strerrbuf, buflen) == 0)
! return strerrbuf;
! else
! return NULL;
! #else

This code will dump core if strerror_r ever fails, which seems like
a bad idea. I suggest that it be like

! if (strerror_r(errnum, strerrbuf, buflen) == 0)
! return strerrbuf;
! else
! return "strerror_r failed";

which at least gives a hint of the problem ...

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

! #ifdef STRERROR_R_INT
! /* SUSv3 version */
! if (strerror_r(errnum, strerrbuf, buflen) == 0)
! return strerrbuf;
! else
! return NULL;
! #else

This code will dump core if strerror_r ever fails, which seems like
a bad idea. I suggest that it be like

! if (strerror_r(errnum, strerrbuf, buflen) == 0)
! return strerrbuf;
! else
! return "strerror_r failed";

which at least gives a hint of the problem ...

I assume all the callers have to check for NULL, rather than supply a
dummy string. However, are you saying that this is more of a debug tool
and should never fail to return a usable value?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.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@candle.pha.pa.us> writes:

I assume all the callers have to check for NULL,

Eh? *None* of the callers of pqStrerror check for NULL, and I don't
think they should have to. We surely do not check for null return from
plain strerror() anyplace.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I assume all the callers have to check for NULL,

Eh? *None* of the callers of pqStrerror check for NULL, and I don't
think they should have to. We surely do not check for null return from
plain strerror() anyplace.

Oh, I see. It should be like strerror(). Makes sense. I replaced NULL
with "Unknown error" which is what strerror does for an unknown error
number.

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