Bug: PGTYPEStimestamp_from_asc() in ECPG pgtypelib
Hi hackers,
I detected two problems about ECPG.
I show my opinion. Please comment.
If it's correct, I will prepare a patch.
Thank you.
1.
It is indefinite what PGTYPEStimestamp_from_asc() returns in error.
The following is written in document(36.6.8. Special Constants of pgtypeslib):
A value of type timestamp representing an invalid time stamp.
This is returned by the function PGTYPEStimestamp_from_asc on parse error.
Note that due to the internal representation of the timestamp data type,
PGTYPESInvalidTimestamp is also a valid timestamp at the same time.
It is set to 1899-12-31 23:59:59. In order to detect errors,
make sure that your application does not only test for PGTYPESInvalidTimestamp
but also for errno != 0 after each call to PGTYPEStimestamp_from_asc.
However, PGTYPESInvalidTimestamp is not defined anywhere.
It no loger exists at REL6_2 that is the oldest branch.
At current implementation, PGTYPEStimestamp_from_asc returns -1.
So we must fix the document so that users write as follows:
r = PGTYPEStimestamp_from_asc(.....);
if (r < 0 || errno != 0)
goto error;
2.
Regression test of pgtypelib is not robust (maybe incorrect).
Our test failed although there is no bug actually.
I think block2 and block3 should be swapped.
---[src/interfaces/ecpg/test/pgtypeslib/dt_test.pgc]---
// block1 (my comment)
ts1 = PGTYPEStimestamp_from_asc("96-02-29", NULL);
text = PGTYPEStimestamp_to_asc(ts1);
printf("timestamp_to_asc1: %s\n", text);
PGTYPESchar_free(text);
// block2
ts1 = PGTYPEStimestamp_from_asc("1994-02-11 26:10:35", NULL);
text = PGTYPEStimestamp_to_asc(ts1);
printf("timestamp_to_asc3: %s\n", text);
PGTYPESchar_free(text);
// The following comment is for block1 clearly.
/* abc-03:10:35-def-02/11/94-gh */
/* 12345678901234567890123456789 */
// block3
// Maybe the following is for 'ts1' returned in block1.
// In our environment, 'out' is indefinite because PGTYPEStimestamp_fmt_asc()
// didn't complete and the area is not initialized.
out = (char*) malloc(32);
i = PGTYPEStimestamp_fmt_asc(&ts1, out, 31, "abc-%X-def-%x-ghi%%");
printf("timestamp_fmt_asc: %d: %s\n", i, out);
free(out);
------------------------------------
Best Regards
Ryo Matsumura
Best Regards
Ryo Matsumura
"Ryo Matsumura (Fujitsu)" <matsumura.ryo@fujitsu.com> writes:
It is indefinite what PGTYPEStimestamp_from_asc() returns in error.
The following is written in document(36.6.8. Special Constants of pgtypeslib):
A value of type timestamp representing an invalid time stamp.
This is returned by the function PGTYPEStimestamp_from_asc on parse error.
However, PGTYPESInvalidTimestamp is not defined anywhere.
Ugh.
At current implementation, PGTYPEStimestamp_from_asc returns -1.
It looks to me like it returns 0 ("noresult"). Where are you seeing
-1?
That documentation has more problems too: it claims that "endptr"
is unimplemented, which looks like a lie to me: the code is there
to do it, and there are several callers that depend on it.
Regression test of pgtypelib is not robust (maybe incorrect).
Our test failed although there is no bug actually.
I think block2 and block3 should be swapped.
Hm, block3 seems to be completely nuts. It looks like the code is
rejecting the input (probably because "26" is out of range) and
returning zero, because what we see in the expected-stdout file is:
timestamp_to_asc3: 2000-01-01 00:00:00
We also see that the expected output from the PGTYPEStimestamp_fmt_asc
step is:
timestamp_fmt_asc: 0: abc-00:00:00-def-01/01/00-ghi%
which is consistent with that, but not very much like what the
comment is expecting. I'm a bit inclined to just drop "block 3".
If we want to keep some kind of test of the error behavior,
it doesn't belong right there, and it should be showing what errno
gets set to.
As for the "lack of robustness", I'll bet the problem you are
seeing is that the test uses the %X/%x format specifiers which
are locale-dependent. But how come we haven't noticed that
before? Have you added a setlocale() call somewhere?
regards, tom lane
Hi Tom,
Thank you for comment.
At current implementation, PGTYPEStimestamp_from_asc returns -1.
It looks to me like it returns 0 ("noresult"). Where are you seeing -1?
I took a mistake. Sorry.
PGTYPEStimestamp_from_asc returns 0(noresult).
PGTYPEStimestamp_fmt_asc given 'noresult' returns -1.
But how come we haven't noticed that
before? Have you added a setlocale() call somewhere?
I didn't notice to this point.
I added setlocale() to ECPG in my local branch.
I will test again after removing it.
It looks to me like existing ECPG code does not include setlocale().
So Please ignore about behavior of PGTYPEStimestamp_fmt_asc().
I want to continue to discuss about PGTYPEStimestamp_from_asc.
Current PGTYPEStimestamp_from_asc() returns 0, but should we return -1?
The document claims about return that "It is set to 1899-12-31 23:59:59.".
I wonder.
It is the incompatibility, but it may be allowed.
because I think usual users may check with errno.
Of course, the reason is weak.
Some users may check with 0(noresult) from their experience.
That documentation has more problems too: it claims that "endptr"
is unimplemented, which looks like a lie to me: the code is there
to do it, and there are several callers that depend on it.
I think so too. The followings have the same problem.
PGTYPESdate_from_asc (ParseDate)
PGTYPESinterval_from_asc (ParseDate)
PGTYPEStimestamp_from_asc (ParseDate)
PGTYPESnumeric_from_asc
which is consistent with that, but not very much like what the
comment is expecting. I'm a bit inclined to just drop "block 3".
If we want to keep some kind of test of the error behavior,
it doesn't belong right there, and it should be showing what errno
gets set to.
It is easy to exchange block3 to errno-checking.
However if we just fix there, it is weird because there is
no other errno-checking in dt_tests.
I'm a bit inclined to just drop "block 3".
Are you concerned at the above point?
Best Regards
Ryo Matsumura
"Ryo Matsumura (Fujitsu)" <matsumura.ryo@fujitsu.com> writes:
But how come we haven't noticed that
before? Have you added a setlocale() call somewhere?
I didn't notice to this point.
I added setlocale() to ECPG in my local branch.
I will test again after removing it.
It looks to me like existing ECPG code does not include setlocale().
So Please ignore about behavior of PGTYPEStimestamp_fmt_asc().
If that's the only ecpg test case that fails under a non-C locale,
I think it'd be good to change it to use a non-locale-dependent
format string. Surely there's no compelling reason why it has to
use %x/%X rather than something more stable.
I want to continue to discuss about PGTYPEStimestamp_from_asc.
Current PGTYPEStimestamp_from_asc() returns 0, but should we return -1?
No, I think changing its behavior now after so many years would be a
bad idea. In any case, what's better about -1? They are both legal
timestamp values. I think we'd better fix the docs to match what the
code actually does, and to tell people that they *must* check errno
to detect errors.
I'm a bit inclined to just drop "block 3".
Are you concerned at the above point?
Given your point that no other dt_test case is checking error
behavior, I'm not too concerned about dropping this one. If
we wanted to take the issue seriously, we'd need to add a bunch
of new tests not just tweak this one. (It's far from obvious that
this was even meant to be a test of an error case --- it looks like
just sloppily-verified testing to me. "block 3" must have been
dropped in after the tests before and after it were written,
without noticing that it changed their results.)
regards, tom lane
# I'm sorry for my late response.
I confirmed that the error of regression is caused by my code inserting setlocale() into ecpglib of local branch.
No other tests occur error in non-C locale.
The following is about other topics.
1. About regression test
We should test the followings:
- PGTYPEStimestamp_from_asc("1994-02-11 26:10:35", NULL) returns 0.
- PGTYPEStimestamp_fmt_asc() can accept format string including %x and %X.
ecpglib should be affected by only setlocale() called by user application and
dt_test.pgc does not call it. So the following test is the best, I think.
Please see attached patch for detail (fix_pgtypeslib_regress.patch).
ts1 = PGTYPEStimestamp_from_asc("1994-02-11 3:10:35", NULL);
text = PGTYPEStimestamp_to_asc(ts1);
printf("timestamp_to_asc2: %s\n", text);
PGTYPESchar_free(text);
/* abc-03:10:35-def-02/11/94-gh */
/* 12345678901234567890123456789 */
out = (char*) malloc(32);
i = PGTYPEStimestamp_fmt_asc(&ts1, out, 31, "abc-%X-def-%x-ghi%%");
printf("timestamp_fmt_asc: %d: %s\n", i, out);
free(out);
ts1 = PGTYPEStimestamp_from_asc("1994-02-11 26:10:35", NULL);
text = PGTYPEStimestamp_to_asc(ts1);
printf("timestamp_to_asc3: %s\n", text);
PGTYPESchar_free(text);
We should also add tests that check PGTYPEStimestamp_*() set errno for invalid input correctly,
but I want to leave the improvement to the next timing when implement for timestamp is changed.
(Maybe the timing will not come.)
2. About document of PGTYPEStimestamp_from_asc() and PGTYPESInvalidTimestamp
0 returned by PGTYPEStimestamp_from_asc () is a valid timestamp as you commented and
we should not break compatibility.
So we should remove the document for PGTYPESInvalidTimestamp and add one for checking errno
to description of PGTYPEStimestamp_from_asc().
Please see attached patch for detail (fix_PGTYPESInvalidTimestamp_doc.patch).
3. About endptr of *_from_asc()
PGTYPESdate_from_asc (ParseDate)
PGTYPEStimestamp_from_asc (ParseDate)
PGTYPESinterval_from_asc (ParseDate)
PGTYPESnumeric_from_asc
Basically, they return immediately just after detecting invalid format.
However, after passing the narrow parse, they could fails (e.g. failure of DecodeInterval(), DecodeISO8601Interval(), malloc(), and so on).
So we should write as follows:
If the function detects invalid format,
then it stores the address of the first invalid character in
endptr. However, don't assume it successed if
endptr points to end of input because other
processing(e.g. memory allocation) could fails.
Therefore, you should check return value and errno for detecting error.
You can safely endptr to NULL.
I also found pandora box that description of the followings don't show their behavior when it fails.
I fix doc including them. Please see attached patch(fix_pgtypeslib_funcs_docs.patch).
- PGTYPESdate_from_asc() # sets errno. (can not check return value)
- PGTYPESdate_defmt_asc() # returns -1 and sets errno
- PGTYPEStimestamp_to_asc() # returns NULL and sets errno
- PGTYPEStimestamp_defmt_asc() # just returns 1 and doesn't set errno!
- PGTYPESinterval_new() # returns NULL and sets errno
- PGTYPESinterval_from_asc() # returns NULL and sets errno
- PGTYPESinterval_to_asc() # returns NULL and sets errno
- PGTYPESinterval_copy # currently always return 0
- PGTYPESdecimal_new() # returns NULL and sets errno
4. Bug of PGTYPEStimestamp_defmt_asc()
PGTYPEStimestamp_defmt_asc() doesn't set errno on failure.
I didn't make a patch for it yet.
Best Regards
Ryo Matsumura
Attachments:
fix_pgtypeslib_regress.patchapplication/octet-stream; name=fix_pgtypeslib_regress.patchDownload
diff --git a/src/interfaces/ecpg/test/expected/pgtypeslib-dt_test.stdout b/src/interfaces/ecpg/test/expected/pgtypeslib-dt_test.stdout
index 4b582fd..b9dc0d9 100644
--- a/src/interfaces/ecpg/test/expected/pgtypeslib-dt_test.stdout
+++ b/src/interfaces/ecpg/test/expected/pgtypeslib-dt_test.stdout
@@ -20,8 +20,8 @@ date_defmt_asc10: 1995-12-25
date_defmt_asc12: 0095-12-25
timestamp_to_asc1: 1996-02-29 00:00:00
timestamp_to_asc2: 1994-02-11 03:10:35
+timestamp_fmt_asc: 0: abc-03:10:35-def-02/11/94-ghi%
timestamp_to_asc3: 2000-01-01 00:00:00
-timestamp_fmt_asc: 0: abc-00:00:00-def-01/01/00-ghi%
timestamp_defmt_asc(This is a 4/12/80 3-39l12test, This is a %m/%d/%y %H-%Ml%Stest) = 1980-04-12 03:39:12, error: 0
timestamp_defmt_asc(Tue Jul 22 17:28:44 +0200 2003, %a %b %d %H:%M:%S %z %Y) = 2003-07-22 15:28:44, error: 0
timestamp_defmt_asc(Tue Feb 29 17:28:44 +0200 2000, %a %b %d %H:%M:%S %z %Y) = 2000-02-29 15:28:44, error: 0
diff --git a/src/interfaces/ecpg/test/pgtypeslib/dt_test.pgc b/src/interfaces/ecpg/test/pgtypeslib/dt_test.pgc
index f81a392..acbc696 100644
--- a/src/interfaces/ecpg/test/pgtypeslib/dt_test.pgc
+++ b/src/interfaces/ecpg/test/pgtypeslib/dt_test.pgc
@@ -188,19 +188,19 @@ main(void)
printf("timestamp_to_asc2: %s\n", text);
PGTYPESchar_free(text);
- ts1 = PGTYPEStimestamp_from_asc("1994-02-11 26:10:35", NULL);
- text = PGTYPEStimestamp_to_asc(ts1);
- printf("timestamp_to_asc3: %s\n", text);
- PGTYPESchar_free(text);
-
/* abc-03:10:35-def-02/11/94-gh */
-/* 12345678901234567890123456789 */
+/* 12345678901234567890123456789 */
out = (char*) malloc(32);
i = PGTYPEStimestamp_fmt_asc(&ts1, out, 31, "abc-%X-def-%x-ghi%%");
printf("timestamp_fmt_asc: %d: %s\n", i, out);
free(out);
+ ts1 = PGTYPEStimestamp_from_asc("1994-02-11 26:10:35", NULL);
+ text = PGTYPEStimestamp_to_asc(ts1);
+ printf("timestamp_to_asc3: %s\n", text);
+ PGTYPESchar_free(text);
+
fmt = "This is a %m/%d/%y %H-%Ml%Stest";
in = "This is a 4/12/80 3-39l12test";
i = PGTYPEStimestamp_defmt_asc(in, fmt, &ts1);
fix_pgtypeslib_funcs_docs.patchapplication/octet-stream; name=fix_pgtypeslib_funcs_docs.patchDownload
diff --git a/doc/src/sgml/ecpg.sgml b/doc/src/sgml/ecpg.sgml
index 48684cf..b551636 100644
--- a/doc/src/sgml/ecpg.sgml
+++ b/doc/src/sgml/ecpg.sgml
@@ -2164,11 +2164,13 @@ numeric *PGTYPESnumeric_from_asc(char *str, char **endptr);
<literal>+3.44</literal>,
<literal>592.49E07</literal> or
<literal>-32.84e-4</literal>.
- If the value could be parsed successfully, a valid pointer is returned,
- else the NULL pointer. At the moment ECPG always parses the complete
- string and so it currently does not support to store the address of the
- first invalid character in <literal>*endptr</literal>. You can safely
- set <literal>endptr</literal> to NULL.
+ If the function detects invalid format,
+ then it stores the address of the first invalid character in
+ <literal>endptr</literal>. However, don't assume it successed if
+ <literal>endptr</literal> points to end of input because other
+ processing(e.g. memory allocation) could fails.
+ Therefore, you should check return value and errno for detecting error.
+ You can safely <literal>endptr</literal> to NULL.
</para>
</listitem>
</varlistentry>
@@ -2469,12 +2471,20 @@ date PGTYPESdate_from_timestamp(timestamp dt);
date PGTYPESdate_from_asc(char *str, char **endptr);
</synopsis>
The function receives a C char* string <literal>str</literal> and a pointer to
- a C char* string <literal>endptr</literal>. At the moment ECPG always parses
- the complete string and so it currently does not support to store the
- address of the first invalid character in <literal>*endptr</literal>.
- You can safely set <literal>endptr</literal> to NULL.
+ a C char* string <literal>endptr</literal>.
+ If the function detects invalid format,
+ then it stores the address of the first invalid character in
+ <literal>endptr</literal>. However, don't assume it successed if
+ <literal>endptr</literal> points to end of input because other
+ processing(e.g. memory allocation) could fails.
+ Therefore, you should check errno for detecting error.
+ You can safely <literal>endptr</literal> to NULL.
</para>
<para>
+ The global variable errno will be set if an error occurs.
+ In order to detect errors, check errno != 0 after calling.
+ <para>
+ <para>
Note that the function always assumes MDY-formatted dates and there is
currently no variable to change that within ECPG.
</para>
@@ -2826,6 +2836,10 @@ int PGTYPESdate_defmt_asc(date *d, char *fmt, char *str);
day.
</para>
<para>
+ Upon success, the function returns 0 and a negative value if an error occurred.
+ The global variable errno will be set if an error occurs.
+ </para>
+ <para>
<xref linkend="ecpg-rdefmtdate-example-table"/> indicates a few possible formats. This will give
you an idea of how to use this function.
</para>
@@ -2935,10 +2949,13 @@ timestamp PGTYPEStimestamp_from_asc(char *str, char **endptr);
</synopsis>
The function receives the string to parse (<literal>str</literal>) and a
pointer to a C char* (<literal>endptr</literal>).
- At the moment ECPG always parses
- the complete string and so it currently does not support to store the
- address of the first invalid character in <literal>*endptr</literal>.
- You can safely set <literal>endptr</literal> to NULL.
+ If the function detects invalid format,
+ then it stores the address of the first invalid character in
+ <literal>endptr</literal>. However, don't assume it successed if
+ <literal>endptr</literal> points to end of input because other
+ processing(e.g. memory allocation) could fails.
+ Therefore, you should check return value and errno for detecting error.
+ You can safely <literal>endptr</literal> to NULL.
</para>
<para>
The function returns the parsed timestamp on success. On error,
@@ -3007,6 +3024,9 @@ char *PGTYPEStimestamp_to_asc(timestamp tstamp);
textual representation of the timestamp.
The result must be freed with <function>PGTYPESchar_free()</function>.
</para>
+ <para>
+ The function returns NULL and sets the global variable errno if an error occurred.
+ </para>
</listitem>
</varlistentry>
@@ -3395,6 +3415,10 @@ int PGTYPEStimestamp_defmt_asc(char *str, char *fmt, timestamp *d);
that <literal>d</literal> points to.
</para>
<para>
+ Upon success, the function returns 0 and 1 if an error occurred.
+ The global variable errno will not be set if an error occurs.
+ </para>
+ <para>
If the formatting mask <literal>fmt</literal> is NULL, the function will fall
back to the default formatting mask which is <literal>%Y-%m-%d
%H:%M:%S</literal>.
@@ -3465,7 +3489,7 @@ int PGTYPEStimestamp_sub_interval(timestamp *tin, interval *span, timestamp *tou
<term><function>PGTYPESinterval_new</function></term>
<listitem>
<para>
- Return a pointer to a newly allocated interval variable.
+ Return a pointer to a newly allocated interval variable and NULL if an error occurred.
<synopsis>
interval *PGTYPESinterval_new(void);
</synopsis>
@@ -3495,10 +3519,16 @@ interval *PGTYPESinterval_from_asc(char *str, char **endptr);
</synopsis>
The function parses the input string <literal>str</literal> and returns a
pointer to an allocated interval variable.
- At the moment ECPG always parses
- the complete string and so it currently does not support to store the
- address of the first invalid character in <literal>*endptr</literal>.
- You can safely set <literal>endptr</literal> to NULL.
+ If the function detects invalid format,
+ then it stores the address of the first invalid character in
+ <literal>endptr</literal>. However, don't assume it successed if
+ <literal>endptr</literal> points to end of input because other
+ processing(e.g. memory allocation) could fails.
+ Therefore, you should check return value and errno for detecting error.
+ You can safely <literal>endptr</literal> to NULL.
+ </para>
+ <para>
+ It returns NULL and sets the global variable errno if an error occurs.
</para>
</listitem>
</varlistentry>
@@ -3516,6 +3546,9 @@ char *PGTYPESinterval_to_asc(interval *span);
<literal>@ 1 day 12 hours 59 mins 10 secs</literal>.
The result must be freed with <function>PGTYPESchar_free()</function>.
</para>
+ <para>
+ It returns NULL and sets the global variable errno if an error occurs.
+ </para>
</listitem>
</varlistentry>
@@ -3532,6 +3565,9 @@ int PGTYPESinterval_copy(interval *intvlsrc, interval *intvldest);
that you need to allocate the memory for the destination variable
before.
</para>
+ <para>
+ Upon success, the function returns 0 and non-0 value if an error occurred.
+ </para>
</listitem>
</varlistentry>
</variablelist>
@@ -3559,7 +3595,7 @@ int PGTYPESinterval_copy(interval *intvlsrc, interval *intvldest);
<term><function>PGTYPESdecimal_new</function></term>
<listitem>
<para>
- Request a pointer to a newly allocated decimal variable.
+ Request a pointer to a newly allocated decimal variable and NULL if an error occurred.
<synopsis>
decimal *PGTYPESdecimal_new(void);
</synopsis>
fix_PGTYPESInvalidTimestamp_doc.patchapplication/octet-stream; name=fix_PGTYPESInvalidTimestamp_doc.patchDownload
diff --git a/doc/src/sgml/ecpg.sgml b/doc/src/sgml/ecpg.sgml
index e7a53f3..48684cf 100644
--- a/doc/src/sgml/ecpg.sgml
+++ b/doc/src/sgml/ecpg.sgml
@@ -2942,8 +2942,14 @@ timestamp PGTYPEStimestamp_from_asc(char *str, char **endptr);
</para>
<para>
The function returns the parsed timestamp on success. On error,
- <literal>PGTYPESInvalidTimestamp</literal> is returned and <varname>errno</varname> is
- set to <literal>PGTYPES_TS_BAD_TIMESTAMP</literal>. See <xref linkend="pgtypesinvalidtimestamp"/> for important notes on this value.
+ 0 is returned and <varname>errno</varname> is set to
+ <literal>PGTYPES_TS_BAD_TIMESTAMP</literal>.
+ Note that due to the internal representation of the <type>timestamp</type> data type,
+ 0 is also a valid timestamp at the same time.
+ It is set to <literal>1900-01-01 00:00:00</literal>. In order
+ to detect errors, make sure that your application does not only test
+ for return value but also for <literal>errno != 0</literal> after each call to
+ <function>PGTYPEStimestamp_from_asc</function>.
</para>
<para>
In general, the input string can contain any combination of an allowed
@@ -3717,31 +3723,6 @@ void PGTYPESdecimal_free(decimal *var);
</variablelist>
</para>
</sect2>
-
- <sect2 id="ecpg-pgtypes-constants">
- <title>Special Constants of pgtypeslib</title>
- <para>
- <variablelist>
- <varlistentry id="pgtypesinvalidtimestamp">
- <term><literal>PGTYPESInvalidTimestamp</literal></term>
- <listitem>
- <para>
- A value of type timestamp representing an invalid time stamp. This is
- returned by the function <function>PGTYPEStimestamp_from_asc</function> on
- parse error.
- Note that due to the internal representation of the <type>timestamp</type> data type,
- <literal>PGTYPESInvalidTimestamp</literal> is also a valid timestamp at
- the same time. It is set to <literal>1899-12-31 23:59:59</literal>. In order
- to detect errors, make sure that your application does not only test
- for <literal>PGTYPESInvalidTimestamp</literal> but also for
- <literal>errno != 0</literal> after each call to
- <function>PGTYPEStimestamp_from_asc</function>.
- </para>
- </listitem>
- </varlistentry>
- </variablelist>
- </para>
- </sect2>
</sect1>
<sect1 id="ecpg-descriptors">