Re: [HACKERS] Bug in to_timestamp().
On 2/27/17 4:19 AM, Artur Zakirov wrote: > On 15.02.2017 15:26, Amul Sul wrote: >> >> The new status of this patch is: Ready for Committer >> > > Thank you for your review! This submission has been moved to CF 2017-07. -- -David da...@pgmasters.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On 15.02.2017 15:26, Amul Sul wrote: The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, passed Spec compliant: not tested Documentation:tested, passed Review of v7 patch: - Patch applies to the top of master HEAD cleanly & make check-world also fine. - Tom Lane's review comments are fixed. The new status of this patch is: Ready for Committer Thank you for your review! -- Artur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, passed Spec compliant: not tested Documentation:tested, passed Review of v7 patch: - Patch applies to the top of master HEAD cleanly & make check-world also fine. - Tom Lane's review comments are fixed. The new status of this patch is: Ready for Committer -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Mon, Dec 5, 2016 at 11:35 AM, Haribabu Kommi wrote: > Moved to next CF with "needs review" status. Same, this time to CF 2017-03. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Mon, Nov 7, 2016 at 2:26 AM, Artur Zakirov wrote: > > Hm, truly. Fixed it. > > I've attached the fixed patch. > Moved to next CF with "needs review" status. Regards, Hari Babu Fujitsu Australia
Re: [HACKERS] Bug in to_timestamp().
Thank you for your comments, 2016-11-04 20:36 GMT+02:00 Tom Lane : > > Artur Zakirov writes: > > I attached new version of the patch, which fix is_char_separator() > > declaration too. > > I did some experimenting using > http://rextester.com/l/oracle_online_compiler > > > which makes it look a lot like Oracle treats separator characters in the > pattern the same as spaces (but I haven't checked their documentation to > confirm that). > > The proposed patch doesn't seem to me to be trying to follow > these Oracle behaviors, but I think there is very little reason for > changing any of this stuff unless we move it closer to Oracle. Previous versions of the patch doesn't try to follow all Oracle behaviors. It tries to fix Amul Sul's behaviors. Because I was confused by dealing with spaces and separators by Oracle to_timestamp() and there is not information about it in the Oracle documentation: https://docs.oracle.com/cd/B19306_01/server.102/b14200/sql_elements004.htm#g195443 I've thought better about it now and fixed the patch. Now parser removes spaces after and before fields and insists that count of separators in the input string should match count of spaces or separators in the formatting string (but in formatting string we can have more separators than in input string). > > Some other nitpicking: > > * I think the is-separator function would be better coded like > > static bool > is_separator_char(const char *str) > { > /* ASCII printable character, but not letter or digit */ > return (*str > 0x20 && *str < 0x7F && > !(*str >= 'A' && *str <= 'Z') && > !(*str >= 'a' && *str <= 'z') && > !(*str >= '0' && *str <= '9')); > } > > The previous way is neither readable nor remarkably efficient, and it > knows much more about the ASCII character set than it needs to. Fixed. > > * Don't forget the cast to unsigned char when using isspace() or other > functions. Fixed. > > * I do not see the reason for throwing an error here: > > + /* Previous character was a backslash */ > + if (in_backslash) > + { > + /* After backslash should go non-space character */ > + if (isspace(*str)) > + ereport(ERROR, > + > (errcode(ERRCODE_SYNTAX_ERROR), > +errmsg("invalid escape > sequence"))); > + in_backslash = false; > > Why shouldn't backslash-space be a valid quoting sequence? > Hm, truly. Fixed it. I've attached the fixed patch. -- Sincerely, Artur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 2e64cc4..5a4e248 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -6159,7 +6159,8 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}'); to_timestamp and to_date skip multiple blank spaces in the input string unless the FX option is used. For example, - to_timestamp('2000JUN', ' MON') works, but + to_timestamp('2000JUN', ' MON') and + to_timestamp('2000 JUN', 'MON') work, but to_timestamp('2000JUN', 'FX MON') returns an error because to_timestamp expects one space only. FX must be specified as the first item in @@ -6169,6 +6170,19 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}'); + to_timestamp and to_date don't + skip multiple printable non letter and non digit characters in the input + string, but skip them in the formatting string. For example, + to_timestamp('2000-JUN', '/MON') and + to_timestamp('2000/JUN', '//MON') work, but + to_timestamp('2000//JUN', '/MON') + returns an error because count of the "/" character in the input string + doesn't match count of it in the formatting string. + + + + + Ordinary text is allowed in to_char templates and will be output literally. You can put a substring in double quotes to force it to be interpreted as literal text diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c index d4eaa50..d28ceec 100644 --- a/src/backend/utils/adt/formatting.c +++ b/src/backend/utils/adt/formatting.c @@ -169,6 +169,8 @@ struct FormatNode #define NODE_TYPE_END 1 #define NODE_TYPE_ACTION 2 #define NODE_TYPE_CHAR 3 +#define NODE_TYPE_SEPARATOR 4 +#define NODE_TYPE_SPACE 5 #define SUFFTYPE_PREFIX 1 #define SUFFTYPE_POSTFIX 2 @@ -951,6 +953,7 @@ typedef struct NUMProc static const KeyWord *index_seq_search(const char *str, const KeyWord *kw, const int *index); static const KeySuffix *suff_search(const char *str, const KeySuffix *suf, int type); +static bool is_separator_char(const char *str); static void NUMDesc_prepare
Re: [HACKERS] Bug in to_timestamp().
Artur Zakirov writes: > I attached new version of the patch, which fix is_char_separator() > declaration too. I did some experimenting using http://rextester.com/l/oracle_online_compiler It appears that Oracle will consider a single space in the pattern to match zero or more spaces in the input, as all of these produce the expected result: SELECT to_timestamp('2000JUN', ' MON') FROM dual SELECT to_timestamp('2000 JUN', ' MON') FROM dual SELECT to_timestamp('2000 JUN', ' MON') FROM dual Also, a space in the pattern will match a single separator character in the input, but not multiple separators: SELECT to_timestamp('2000-JUN', ' MON') FROM dual -- ok SELECT to_timestamp('2000--JUN', ' MON') FROM dual ORA-01843: not a valid month And you can have whitespace along with that single separator: SELECT to_timestamp('2000 + JUN', ' MON') FROM dual -- ok SELECT to_timestamp('2000 + JUN', ' MON') FROM dual -- ok SELECT to_timestamp('2000 ++ JUN', ' MON') FROM dual ORA-01843: not a valid month You can have leading whitespace, but not leading separators: SELECT to_timestamp(' 2000 JUN', ' MON') FROM dual -- ok SELECT to_timestamp('/2000 JUN', ' MON') FROM dual ORA-01841: (full) year must be between -4713 and +, and not be 0 These all work: SELECT to_timestamp('2000 JUN', '/MON') FROM dual SELECT to_timestamp('2000JUN', '/MON') FROM dual SELECT to_timestamp('2000/JUN', '/MON') FROM dual SELECT to_timestamp('2000-JUN', '/MON') FROM dual but not SELECT to_timestamp('2000//JUN', '/MON') FROM dual ORA-01843: not a valid month SELECT to_timestamp('2000--JUN', '/MON') FROM dual ORA-01843: not a valid month which makes it look a lot like Oracle treats separator characters in the pattern the same as spaces (but I haven't checked their documentation to confirm that). The proposed patch doesn't seem to me to be trying to follow these Oracle behaviors, but I think there is very little reason for changing any of this stuff unless we move it closer to Oracle. Some other nitpicking: * I think the is-separator function would be better coded like static bool is_separator_char(const char *str) { /* ASCII printable character, but not letter or digit */ return (*str > 0x20 && *str < 0x7F && !(*str >= 'A' && *str <= 'Z') && !(*str >= 'a' && *str <= 'z') && !(*str >= '0' && *str <= '9')); } The previous way is neither readable nor remarkably efficient, and it knows much more about the ASCII character set than it needs to. * Don't forget the cast to unsigned char when using isspace() or other functions. * I do not see the reason for throwing an error here: + /* Previous character was a backslash */ + if (in_backslash) + { + /* After backslash should go non-space character */ + if (isspace(*str)) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), +errmsg("invalid escape sequence"))); + in_backslash = false; Why shouldn't backslash-space be a valid quoting sequence? I'll set this back to Waiting on Author. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Fri, Sep 30, 2016 at 12:34 PM, Amul Sul wrote: > The new status of this patch is: Ready for Committer And moved to next CF with same status. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation:tested, passed Appreciate your support. I've tested v6 patch again, looks good to me, code in v6 does not differ much from v4 patch. Ready for committer review. Thanks ! Regards, Amul Sul The new status of this patch is: Ready for Committer -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
Robert Haas writes: > On Thu, Sep 29, 2016 at 4:54 AM, amul sul wrote: >> Commitfest status left untouched, I am not sure how to deal with >> "Returned With Feedback" patch. Is there any chance that, this can be >> considered again for committer review? > You may be able to set the status back to "Ready for Committer", or > else you can move the entry to the next CommitFest and do it there. I already switched it back to "Needs Review". AFAIK none of the statuses are immovable. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Thu, Sep 29, 2016 at 4:54 AM, amul sul wrote: > On Thu, Sep 29, 2016 at 2:48 AM, Tom Lane wrote: >> Artur Zakirov writes: >>> - now DCH_cache_getnew() is called after parse_format(). Because now >>> parse_format() can raise an error and in the next attempt >>> DCH_cache_search() could return broken cache entry. >> >> I started looking at your 0001-to-timestamp-format-checking-v4.patch >> and this point immediately jumped out at me. Currently the code relies >> ... without any documentation ... on no elog being thrown out of >> parse_format(). That's at the very least trouble waiting to happen. >> There's a hack to deal with errors from within the NUMDesc_prepare >> subroutine, but it's a pretty ugly and underdocumented hack. And what >> you had here was randomly different from that solution, too. >> >> After a bit of thought it seemed to me that a much cleaner fix is to add >> a "valid" flag to the cache entries, which we can leave clear until we >> have finished parsing the new format string. That avoids adding extra >> data copying as you suggested, removes the need for PG_TRY, and just >> generally seems cleaner and more bulletproof. >> >> I've pushed a patch that does it that way. The 0001 patch will need >> to be rebased over that (might just require removal of some hunks, >> not sure). >> >> I also pushed 0002-to-timestamp-validation-v2.patch with some revisions >> (it'd broken acceptance of BC dates, among other things, but I think >> I fixed everything). >> >> Since you told us earlier that you'd be on vacation through the end of >> September, I'm assuming that nothing more will happen on this patch during >> this commitfest, so I will mark the CF entry Returned With Feedback. > > Behalf of Artur I've rebased patch, removed hunk dealing with broken > cache entries by copying it, which is no longer required after 83bed06 > commit. > > Commitfest status left untouched, I am not sure how to deal with > "Returned With Feedback" patch. Is there any chance that, this can be > considered again for committer review? You may be able to set the status back to "Ready for Committer", or else you can move the entry to the next CommitFest and do it there. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
2016-09-29 13:54 GMT+05:00 amul sul : > > On Thu, Sep 29, 2016 at 2:48 AM, Tom Lane wrote: > > > > I started looking at your 0001-to-timestamp-format-checking-v4.patch > > and this point immediately jumped out at me. Currently the code relies > > ... without any documentation ... on no elog being thrown out of > > parse_format(). That's at the very least trouble waiting to happen. > > There's a hack to deal with errors from within the NUMDesc_prepare > > subroutine, but it's a pretty ugly and underdocumented hack. And what > > you had here was randomly different from that solution, too. > > > > After a bit of thought it seemed to me that a much cleaner fix is to add > > a "valid" flag to the cache entries, which we can leave clear until we > > have finished parsing the new format string. That avoids adding extra > > data copying as you suggested, removes the need for PG_TRY, and just > > generally seems cleaner and more bulletproof. > > > > I've pushed a patch that does it that way. The 0001 patch will need > > to be rebased over that (might just require removal of some hunks, > > not sure). > > > > I also pushed 0002-to-timestamp-validation-v2.patch with some revisions > > (it'd broken acceptance of BC dates, among other things, but I think > > I fixed everything). Thank you for committing the 0002 part of the patch! I wanted to fix cache functions too, but wasn't sure about that. > > > > Since you told us earlier that you'd be on vacation through the end of > > September, I'm assuming that nothing more will happen on this patch during > > this commitfest, so I will mark the CF entry Returned With Feedback. > > Behalf of Artur I've rebased patch, removed hunk dealing with broken > cache entries by copying it, which is no longer required after 83bed06 > commit. > > Commitfest status left untouched, I am not sure how to deal with > "Returned With Feedback" patch. Is there any chance that, this can be > considered again for committer review? Thank you for fixing the patch! Today I have access to the internet and able to fix and test the patch. I've looked at your 0001-to-timestamp-format-checking-v5.patch. It seems nice to me. I suppose it is necessary to fix is_char_separator() declaration. from: static bool is_char_separator(char *str); to: static bool is_char_separator(const char *str); Because now in parse_format() *str argument is const. I attached new version of the patch, which fix is_char_separator() declaration too. Sorry for confusing! -- Artur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company 0001-to-timestamp-format-checking-v6.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Thu, Sep 29, 2016 at 2:48 AM, Tom Lane wrote: > Artur Zakirov writes: >> - now DCH_cache_getnew() is called after parse_format(). Because now >> parse_format() can raise an error and in the next attempt >> DCH_cache_search() could return broken cache entry. > > I started looking at your 0001-to-timestamp-format-checking-v4.patch > and this point immediately jumped out at me. Currently the code relies > ... without any documentation ... on no elog being thrown out of > parse_format(). That's at the very least trouble waiting to happen. > There's a hack to deal with errors from within the NUMDesc_prepare > subroutine, but it's a pretty ugly and underdocumented hack. And what > you had here was randomly different from that solution, too. > > After a bit of thought it seemed to me that a much cleaner fix is to add > a "valid" flag to the cache entries, which we can leave clear until we > have finished parsing the new format string. That avoids adding extra > data copying as you suggested, removes the need for PG_TRY, and just > generally seems cleaner and more bulletproof. > > I've pushed a patch that does it that way. The 0001 patch will need > to be rebased over that (might just require removal of some hunks, > not sure). > > I also pushed 0002-to-timestamp-validation-v2.patch with some revisions > (it'd broken acceptance of BC dates, among other things, but I think > I fixed everything). > > Since you told us earlier that you'd be on vacation through the end of > September, I'm assuming that nothing more will happen on this patch during > this commitfest, so I will mark the CF entry Returned With Feedback. Behalf of Artur I've rebased patch, removed hunk dealing with broken cache entries by copying it, which is no longer required after 83bed06 commit. Commitfest status left untouched, I am not sure how to deal with "Returned With Feedback" patch. Is there any chance that, this can be considered again for committer review? Thanks ! Regards, Amul 0001-to-timestamp-format-checking-v5.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
Artur Zakirov writes: > - now DCH_cache_getnew() is called after parse_format(). Because now > parse_format() can raise an error and in the next attempt > DCH_cache_search() could return broken cache entry. I started looking at your 0001-to-timestamp-format-checking-v4.patch and this point immediately jumped out at me. Currently the code relies ... without any documentation ... on no elog being thrown out of parse_format(). That's at the very least trouble waiting to happen. There's a hack to deal with errors from within the NUMDesc_prepare subroutine, but it's a pretty ugly and underdocumented hack. And what you had here was randomly different from that solution, too. After a bit of thought it seemed to me that a much cleaner fix is to add a "valid" flag to the cache entries, which we can leave clear until we have finished parsing the new format string. That avoids adding extra data copying as you suggested, removes the need for PG_TRY, and just generally seems cleaner and more bulletproof. I've pushed a patch that does it that way. The 0001 patch will need to be rebased over that (might just require removal of some hunks, not sure). I also pushed 0002-to-timestamp-validation-v2.patch with some revisions (it'd broken acceptance of BC dates, among other things, but I think I fixed everything). Since you told us earlier that you'd be on vacation through the end of September, I'm assuming that nothing more will happen on this patch during this commitfest, so I will mark the CF entry Returned With Feedback. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Fri, Sep 16, 2016 at 10:01 PM, Artur Zakirov wrote: > On 25.08.2016 13:26, amul sul wrote: >>> >>> Thanks. I've created the entry in >>> https://commitfest.postgresql.org/10/713/ >>> . You can add yourself as a reviewer. >>> >> >> Done, added myself as reviewer & changed status to "Ready for >> Committer". Thanks ! >> >> Regards, >> Amul Sul >> >> > > It seems there is no need to rebase patches. But I will not be able to fix > patches for two weeks because of travel if someone will have a note or > remark. > > So it would be nice if someone will be able to fix possible issues for above > reasone. Sure, Ill do that, if required. Regards, Amul -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On 25.08.2016 13:26, amul sul wrote: Thanks. I've created the entry in https://commitfest.postgresql.org/10/713/ . You can add yourself as a reviewer. Done, added myself as reviewer & changed status to "Ready for Committer". Thanks ! Regards, Amul Sul It seems there is no need to rebase patches. But I will not be able to fix patches for two weeks because of travel if someone will have a note or remark. So it would be nice if someone will be able to fix possible issues for above reasone. -- Artur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Thu, Aug 25, 2016 at 3:41 PM, Artur Zakirov wrote: >>> You are right. I assigned to prev_type NODE_TYPE_SPACE to be able to >>> execute such query: >>> >>> >>> SELECT to_timestamp('---2000JUN', ' MON'); >>> >>> >>> Will be it a proper behaviour? >> >> >> >> Looks good to me, no one will complain if something working on PG but not >> on Oracle. > > > Thanks. I've created the entry in https://commitfest.postgresql.org/10/713/ > . You can add yourself as a reviewer. > Done, added myself as reviewer & changed status to "Ready for Committer". Thanks ! Regards, Amul Sul -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
You are right. I assigned to prev_type NODE_TYPE_SPACE to be able to execute such query: SELECT to_timestamp('---2000JUN', ' MON'); Will be it a proper behaviour? Looks good to me, no one will complain if something working on PG but not on Oracle. Thanks. I've created the entry in https://commitfest.postgresql.org/10/713/ . You can add yourself as a reviewer. -- Artur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Thursday, August 25, 2016 1:56 PM, Artur Zakirov wrote: >> #2. Warning at compilation; >> >> formatting.c: In function ‘do_to_timestamp’: >> formatting.c:3049:37: warning: ‘prev_type’ may be used uninitialized in this >> function [-Wmaybe-uninitialized] >> if (prev_type == NODE_TYPE_SPACE || prev_type == NODE_TYPE_SEPARATOR) >> ^ >> formatting.c:2988:5: note: ‘prev_type’ was declared here >> prev_type; >> ^ >> >> You can avoid this by assigning zero (or introduce NODE_TYPE_INVAL ) to >> prev_type at following line: >> >> 256 + prev_type; > > >You are right. I assigned to prev_type NODE_TYPE_SPACE to be able to >execute such query: > > >SELECT to_timestamp('---2000JUN', ' MON'); > > >Will be it a proper behaviour? Looks good to me, no one will complain if something working on PG but not on Oracle. Thanks & Regards, Amul Sul -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
Hi, #1. Whitespace @ line # 317. Sorry, fixed. #2. Warning at compilation; formatting.c: In function ‘do_to_timestamp’: formatting.c:3049:37: warning: ‘prev_type’ may be used uninitialized in this function [-Wmaybe-uninitialized] if (prev_type == NODE_TYPE_SPACE || prev_type == NODE_TYPE_SEPARATOR) ^ formatting.c:2988:5: note: ‘prev_type’ was declared here prev_type; ^ You can avoid this by assigning zero (or introduce NODE_TYPE_INVAL ) to prev_type at following line: 256 + prev_type; You are right. I assigned to prev_type NODE_TYPE_SPACE to be able to execute such query: SELECT to_timestamp('---2000JUN', ' MON'); Will be it a proper behaviour? -- Artur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 5c1c4f6..36d8b3e 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -6146,9 +6146,12 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}'); to_timestamp and to_date - skip multiple blank spaces in the input string unless the + skip multiple blank spaces and printable non letter and non digit + characters in the input string and in the formatting string unless the FX option is used. For example, - to_timestamp('2000JUN', ' MON') works, but + to_timestamp('2000JUN', ' MON'), + to_timestamp('2000JUN', ' MON') + and to_timestamp('2000 JUN', 'MON') work, but to_timestamp('2000JUN', 'FX MON') returns an error because to_timestamp expects one space only. FX must be specified as the first item in diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c index bbd97dc..7430013 100644 --- a/src/backend/utils/adt/formatting.c +++ b/src/backend/utils/adt/formatting.c @@ -169,6 +169,8 @@ struct FormatNode #define NODE_TYPE_END 1 #define NODE_TYPE_ACTION 2 #define NODE_TYPE_CHAR 3 +#define NODE_TYPE_SEPARATOR 4 +#define NODE_TYPE_SPACE 5 #define SUFFTYPE_PREFIX 1 #define SUFFTYPE_POSTFIX 2 @@ -947,6 +949,7 @@ typedef struct NUMProc static const KeyWord *index_seq_search(char *str, const KeyWord *kw, const int *index); static const KeySuffix *suff_search(char *str, const KeySuffix *suf, int type); +static bool is_char_separator(char *str); static void NUMDesc_prepare(NUMDesc *num, FormatNode *n); static void parse_format(FormatNode *node, char *str, const KeyWord *kw, const KeySuffix *suf, const int *index, int ver, NUMDesc *Num); @@ -963,7 +966,6 @@ static void dump_node(FormatNode *node, int max); static const char *get_th(char *num, int type); static char *str_numth(char *dest, char *num, int type); static int adjust_partial_year_to_2020(int year); -static int strspace_len(char *str); static void from_char_set_mode(TmFromChar *tmfc, const FromCharDateMode mode); static void from_char_set_int(int *dest, const int value, const FormatNode *node); static int from_char_parse_int_len(int *dest, char **src, const int len, FormatNode *node); @@ -1036,6 +1038,17 @@ suff_search(char *str, const KeySuffix *suf, int type) return NULL; } +static bool +is_char_separator(char *str) +{ + return ((pg_mblen(str) == 1) && + /* printable character, but not letter and digit */ + ((*str >= '!' && *str <= '/') || + (*str >= ':' && *str <= '@') || + (*str >= '[' && *str <= '`') || + (*str >= '{' && *str <= '~'))); +} + /* -- * Prepare NUMDesc (number description struct) via FormatNode struct * -- @@ -1237,9 +1250,10 @@ parse_format(FormatNode *node, char *str, const KeyWord *kw, { const KeySuffix *s; FormatNode *n; + bool in_text = false, +in_backslash = false; int node_set = 0, -suffix, -last = 0; +suffix; #ifdef DEBUG_TO_FROM_CHAR elog(DEBUG_elog_output, "to_char/number(): run parser"); @@ -1251,6 +1265,55 @@ parse_format(FormatNode *node, char *str, const KeyWord *kw, { suffix = 0; + /* Previous character was a backslash */ + if (in_backslash) + { + /* After backslash should go non-space character */ + if (isspace(*str)) +ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("invalid escape sequence"))); + in_backslash = false; + + n->type = NODE_TYPE_CHAR; + n->character = *str; + n->key = NULL; + n->suffix = 0; + n++; + str++; + continue; + } + /* Previous character was a quote */ + else if (in_text) + { + if (*str == '"') + { +str++; +in_text = false; + } + else if (*str == '\\') + { +str++; +in_backslash = true; + } + else + { +if (ver == DCH_TYPE && is_char_separator(str)) + n->type = NODE_TYPE_SEPARATOR; +else if (isspace(*str)) + n->type = NODE_TYPE_SPACE; +else + n->type = NODE_TYPE_CHAR; + +n->character = *str; +n->key = NULL; +n->suffix = 0; +n++; +str++; + } + continue; +
Re: [HACKERS] Bug in to_timestamp().
Hi Artur Zakirov, 0001-to-timestamp-format-checking-v3.patch looks pretty reasonable to me, other than following concern: #1. Whitespace @ line # 317. #2. Warning at compilation; formatting.c: In function ‘do_to_timestamp’: formatting.c:3049:37: warning: ‘prev_type’ may be used uninitialized in this function [-Wmaybe-uninitialized] if (prev_type == NODE_TYPE_SPACE || prev_type == NODE_TYPE_SEPARATOR) ^ formatting.c:2988:5: note: ‘prev_type’ was declared here prev_type; ^ You can avoid this by assigning zero (or introduce NODE_TYPE_INVAL ) to prev_type at following line: 256 + prev_type; Regards, Amul Sul -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
Sorry. I did not get last two mails from Amul. Don't know why. So I reply to another mail. Documented as working case, but unfortunatly it does not : postgres=# SELECT to_timestamp('2000JUN', ' MON'); ERROR: invalid value "---" for "MON" DETAIL: The given value did not match any of the allowed values for this field. Indeed! Fixed it. Now this query executes without error. Added this case to tests. NODE_TYPE_CHAR assumption in else block seems incorrect. What if we have space after double quote? It will again have incorrect output without any error, see below: postgres=# SELECT to_timestamp('Year: 1976, Month: May, Day: 16', postgres(# '"Year:" , "Month:" FMMonth, "Day:" DD'); to_timestamp -- 0006-05-16 00:00:00-07:52:58 (1 row) I guess, we might need NODE_TYPE_SEPARATOR and NODE_TYPE_SPACE check as well? Agree. Fixed and added to tests. Unnecessary hunk? We should not touch any code unless it necessary to implement proposed feature, otherwise it will add unnecessary diff to the patch and eventually extra burden to review the code. Similarly hunk in the patch @ line # 313 - 410, nothing to do with to_timestamp behaviour improvement, IIUC. If you think this changes need to be in, please submit separate cleanup-patch. Fixed it. It was a typo. About lines # 313 - 410. It is necessary to avoid bugs. I wrote aboud it in https://www.postgresql.org/message-id/b2a39359-3282-b402-f4a3-057aae500ee7%40postgrespro.ru : - now DCH_cache_getnew() is called after parse_format(). Because now parse_format() can raise an error and in the next attempt DCH_cache_search() could return broken cache entry. For example, you can test incorrect inputs for to_timestamp(). Try to execute such query several times. -- Artur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 6355300..0fe50e1 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -6146,9 +6146,12 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}'); to_timestamp and to_date - skip multiple blank spaces in the input string unless the + skip multiple blank spaces and printable non letter and non digit + characters in the input string and in the formatting string unless the FX option is used. For example, - to_timestamp('2000JUN', ' MON') works, but + to_timestamp('2000JUN', ' MON'), + to_timestamp('2000JUN', ' MON') + and to_timestamp('2000 JUN', 'MON') work, but to_timestamp('2000JUN', 'FX MON') returns an error because to_timestamp expects one space only. FX must be specified as the first item in diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c index bbd97dc..a3dbcaf 100644 --- a/src/backend/utils/adt/formatting.c +++ b/src/backend/utils/adt/formatting.c @@ -169,6 +169,8 @@ struct FormatNode #define NODE_TYPE_END 1 #define NODE_TYPE_ACTION 2 #define NODE_TYPE_CHAR 3 +#define NODE_TYPE_SEPARATOR 4 +#define NODE_TYPE_SPACE 5 #define SUFFTYPE_PREFIX 1 #define SUFFTYPE_POSTFIX 2 @@ -947,6 +949,7 @@ typedef struct NUMProc static const KeyWord *index_seq_search(char *str, const KeyWord *kw, const int *index); static const KeySuffix *suff_search(char *str, const KeySuffix *suf, int type); +static bool is_char_separator(char *str); static void NUMDesc_prepare(NUMDesc *num, FormatNode *n); static void parse_format(FormatNode *node, char *str, const KeyWord *kw, const KeySuffix *suf, const int *index, int ver, NUMDesc *Num); @@ -963,7 +966,6 @@ static void dump_node(FormatNode *node, int max); static const char *get_th(char *num, int type); static char *str_numth(char *dest, char *num, int type); static int adjust_partial_year_to_2020(int year); -static int strspace_len(char *str); static void from_char_set_mode(TmFromChar *tmfc, const FromCharDateMode mode); static void from_char_set_int(int *dest, const int value, const FormatNode *node); static int from_char_parse_int_len(int *dest, char **src, const int len, FormatNode *node); @@ -1036,6 +1038,17 @@ suff_search(char *str, const KeySuffix *suf, int type) return NULL; } +static bool +is_char_separator(char *str) +{ + return ((pg_mblen(str) == 1) && + /* printable character, but not letter and digit */ + ((*str >= '!' && *str <= '/') || + (*str >= ':' && *str <= '@') || + (*str >= '[' && *str <= '`') || + (*str >= '{' && *str <= '~'))); +} + /* -- * Prepare NUMDesc (number description struct) via FormatNode struct * -- @@ -1237,9 +1250,10 @@ parse_format(FormatNode *node, char *str, const KeyWord *kw, { const KeySuffix *s; FormatNode *n; + bool in_text = false, +in_backslash = false; int node_set = 0, -suffix, -last = 0; +suffix; #ifdef DEBUG_TO_FROM_CHAR e
Re: [HACKERS] Bug in to_timestamp().
Hi Artur Zakirov, Please see following review comment for "0001-to-timestamp-format-checking-v2.patch" & share your thought: #1. 15 + to_timestamp('2000JUN', ' MON') Documented as working case, but unfortunatly it does not : postgres=# SELECT to_timestamp('2000JUN', ' MON'); ERROR: invalid value "---" for "MON" DETAIL: The given value did not match any of the allowed values for this field. #2. 102 + /* Previous character was a quote */ 103 + else if (in_text) 104 + { 105 + if (*str == '"') 106 + { 107 + str++; 108 + in_text = false; 109 + } 110 + else if (*str == '\\') 111 + { 112 + str++; 113 + in_backslash = true; 114 + } 115 + else 116 + { 117 + n->type = NODE_TYPE_CHAR; 118 + n->character = *str; 119 + n->key = NULL; 120 + n->suffix = 0; 121 + n++; 122 + str++; 123 + } 124 + continue; 125 + } 126 + NODE_TYPE_CHAR assumption in else block seems incorrect. What if we have space after double quote? It will again have incorrect output without any error, see below: postgres=# SELECT to_timestamp('Year: 1976, Month: May, Day: 16', postgres(# '"Year:" , "Month:" FMMonth, "Day:" DD'); to_timestamp -- 0006-05-16 00:00:00-07:52:58 (1 row) I guess, we might need NODE_TYPE_SEPARATOR and NODE_TYPE_SPACE check as well? #3. 296 - /* Ignore spaces before fields when not in FX (fixed width) mode */ 297 + /* Ignore spaces before fields when not in FX (fixed * width) mode */ 298 if (!fx_mode && n->key->id != DCH_FX) 299 { 300 - while (*s != '\0' && isspace((unsigned char) *s)) 301 + while (*s != '\0' && (isspace((unsigned char) *s))) 302 s++; Unnecessary hunk? We should not touch any code unless it necessary to implement proposed feature, otherwise it will add unnecessary diff to the patch and eventually extra burden to review the code. Similarly hunk in the patch @ line # 313 - 410, nothing to do with to_timestamp behaviour improvement, IIUC. If you think this changes need to be in, please submit separate cleanup-patch. >I attached second patch "0002-to-timestamp-validation-v2.patch". With it >PostgreSQL perform additional checks for date and time. But as I wrote >there is another patch in the thread "to_date_valid()" wich differs from >this patch. @community : I am not sure what to do with this patch, should we keep it as separate enhancement? Regards, Amul Sul -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Friday, August 19, 2016 12:42 AM, Robert Haas wrote: >On Wed, Aug 17, 2016 at 10:35 AM, amul sul wrote: > > >> Hmm. I haven't really looked into the code, but with applying both patches >> it looks precisely imitate Oracle's behaviour. Thanks. > > >This is good to hear, but for us to consider applying something like >this, somebody would need to look into the code pretty deeply. Sure Robert, Im planning to start initial review until next week at the earliest. Thanks Regards, Amul Sul -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Wed, Aug 17, 2016 at 10:35 AM, amul sul wrote: > Hmm. I haven't really looked into the code, but with applying both patches it > looks precisely imitate Oracle's behaviour. Thanks. This is good to hear, but for us to consider applying something like this, somebody would need to look into the code pretty deeply. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Wednesday, August 17, 2016 5:15 PM, Artur Zakirov wrote: >I attached new patch "0001-to-timestamp-format-checking-v2.patch". It >fixes behaviour for Amul's scenarious: > Great. > >> Following are few scenarios where we break existing behaviour: >> >> SELECT TO_TIMESTAMP('2015-12-31 13:43:36', ' MM DD HH24 MI SS'); >> SELECT TO_TIMESTAMP('2011$03!18 23_38_15', '-MM-DD HH24:MI:SS'); >> SELECT TO_TIMESTAMP('2011*03*18 23^38&15', '-MM-DD HH24:MI:SS'); >> SELECT TO_TIMESTAMP('2011*03!18 #%23^38$15', '-MM-DD$$$HH24:MI:SS'); >> >> But current patch behaviour is not that much bad either at least we have >> errors, but I am not sure about community acceptance. >> >> I would like to divert communities' attention on following case: >> SELECT TO_TIMESTAMP('2013--10-01', '-MM-DD'); > > >For queries above the patch gives an output without any error. > > >> Another is, shouldn’t we have error in following cases? >> SELECT TO_TIMESTAMP('2016-06-13 99:99:99', '-MM-DD HH24:MI:SS'); >> SELECT TO_TIMESTAMP('2016-02-30 15:43:36', '-MM-DD HH24:MI:SS'); > > >I attached second patch "0002-to-timestamp-validation-v2.patch". With it >PostgreSQL perform additional checks for date and time. But as I wrote >there is another patch in the thread "to_date_valid()" wich differs from >this patch. > Hmm. I haven't really looked into the code, but with applying both patches it looks precisely imitate Oracle's behaviour. Thanks. Regards, Amul Sul -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
I attached new patch "0001-to-timestamp-format-checking-v2.patch". It fixes behaviour for Amul's scenarious: Following are few scenarios where we break existing behaviour: SELECT TO_TIMESTAMP('2015-12-31 13:43:36', ' MM DD HH24 MI SS'); SELECT TO_TIMESTAMP('2011$03!18 23_38_15', '-MM-DD HH24:MI:SS'); SELECT TO_TIMESTAMP('2011*03*18 23^38&15', '-MM-DD HH24:MI:SS'); SELECT TO_TIMESTAMP('2011*03!18 #%23^38$15', '-MM-DD$$$HH24:MI:SS'); But current patch behaviour is not that much bad either at least we have errors, but I am not sure about community acceptance. I would like to divert communities' attention on following case: SELECT TO_TIMESTAMP('2013--10-01', '-MM-DD'); For queries above the patch gives an output without any error. Another is, shouldn’t we have error in following cases? SELECT TO_TIMESTAMP('2016-06-13 99:99:99', '-MM-DD HH24:MI:SS'); SELECT TO_TIMESTAMP('2016-02-30 15:43:36', '-MM-DD HH24:MI:SS'); I attached second patch "0002-to-timestamp-validation-v2.patch". With it PostgreSQL perform additional checks for date and time. But as I wrote there is another patch in the thread "to_date_valid()" wich differs from this patch. Sincerely, -- Artur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 7830334..559c55f 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -6083,9 +6083,12 @@ SELECT regexp_matches('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}'); to_timestamp and to_date - skip multiple blank spaces in the input string unless the + skip multiple blank spaces and printable non letter and non digit + characters in the input string and in the formatting string unless the FX option is used. For example, - to_timestamp('2000JUN', ' MON') works, but + to_timestamp('2000JUN', ' MON'), + to_timestamp('2000JUN', ' MON') + and to_timestamp('2000 JUN', 'MON') work, but to_timestamp('2000JUN', 'FX MON') returns an error because to_timestamp expects one space only. FX must be specified as the first item in diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c index bbd97dc..b14678d 100644 --- a/src/backend/utils/adt/formatting.c +++ b/src/backend/utils/adt/formatting.c @@ -169,6 +169,8 @@ struct FormatNode #define NODE_TYPE_END 1 #define NODE_TYPE_ACTION 2 #define NODE_TYPE_CHAR 3 +#define NODE_TYPE_SEPARATOR 4 +#define NODE_TYPE_SPACE 5 #define SUFFTYPE_PREFIX 1 #define SUFFTYPE_POSTFIX 2 @@ -947,6 +949,7 @@ typedef struct NUMProc static const KeyWord *index_seq_search(char *str, const KeyWord *kw, const int *index); static const KeySuffix *suff_search(char *str, const KeySuffix *suf, int type); +static bool is_char_separator(char *str); static void NUMDesc_prepare(NUMDesc *num, FormatNode *n); static void parse_format(FormatNode *node, char *str, const KeyWord *kw, const KeySuffix *suf, const int *index, int ver, NUMDesc *Num); @@ -963,7 +966,6 @@ static void dump_node(FormatNode *node, int max); static const char *get_th(char *num, int type); static char *str_numth(char *dest, char *num, int type); static int adjust_partial_year_to_2020(int year); -static int strspace_len(char *str); static void from_char_set_mode(TmFromChar *tmfc, const FromCharDateMode mode); static void from_char_set_int(int *dest, const int value, const FormatNode *node); static int from_char_parse_int_len(int *dest, char **src, const int len, FormatNode *node); @@ -1036,6 +1038,17 @@ suff_search(char *str, const KeySuffix *suf, int type) return NULL; } +static bool +is_char_separator(char *str) +{ + return ((pg_mblen(str) == 1) && + /* printable character, but not letter and digit */ + ((*str >= '!' && *str <= '/') || + (*str >= ':' && *str <= '@') || + (*str >= '[' && *str <= '`') || + (*str >= '{' && *str <= '~'))); +} + /* -- * Prepare NUMDesc (number description struct) via FormatNode struct * -- @@ -1237,9 +1250,10 @@ parse_format(FormatNode *node, char *str, const KeyWord *kw, { const KeySuffix *s; FormatNode *n; + bool in_text = false, +in_backslash = false; int node_set = 0, -suffix, -last = 0; +suffix; #ifdef DEBUG_TO_FROM_CHAR elog(DEBUG_elog_output, "to_char/number(): run parser"); @@ -1251,6 +1265,49 @@ parse_format(FormatNode *node, char *str, const KeyWord *kw, { suffix = 0; + /* Previous character was a backslash */ + if (in_backslash) + { + /* After backslash should go non-space character */ + if (isspace(*str)) +ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("invalid escape sequence"))); + in_backslash = false; + + n->type = NODE_TYPE_CHAR; + n->character = *str; + n->key = NULL; + n->suffix = 0; + n++; + str++; + continue; + } + /* Previo
Re: [HACKERS] Bug in to_timestamp().
On 15.08.2016 19:28, Robert Haas wrote: Well, what's the Oracle behavior in any of these cases? I don't think we can decide to change any of this behavior without knowing that. If a proposed behavior change is incompatible with our previous releases, I think it'd better at least be more compatible with Oracle. Otherwise, we're just changing from an established behavior that we invented ourselves to a new behavior we invented ourselves, which is only worthwhile if it's absolutely clear that the new behavior is way better. 1 - Oracle's output for first queries is: -> SELECT TO_TIMESTAMP('2015-12-31 13:43:36', ' MM DD HH24 MI SS') FROM dual; TO_TIMESTAMP('2015-12-3113:43:36','MMDDHH24MISS') --- 31-DEC-15 01.43.36.0 PM -> SELECT TO_TIMESTAMP('2011$03!18 23_38_15', '-MM-DD HH24:MI:SS') FROM dual; TO_TIMESTAMP('2011$03!1823_38_15','-MM-DDHH24:MI:SS') --- 18-MAR-11 11.38.15.0 PM -> SELECT TO_TIMESTAMP('2011*03!18 #%23^38$15', '-MM-DD$$$HH24:MI:SS') FROM dual; TO_TIMESTAMP('2011*03!18#%23^38$15','-MM-DD$$$HH24:MI:SS') --- 18-MAR-11 11.38.15.0 PM PostgreSQL with the patch gives "ERROR: expected space character in given string". I will fix this. 2 - Oracle's output for query with hyphen is: -> SELECT TO_TIMESTAMP('2013--10-01', '-MM-DD') FROM dual; SELECT TO_TIMESTAMP('2013--10-01', '-MM-DD') FROM dual * ERROR at line 1: ORA-01843: not a valid month Here PostgreSQL with the patch does not give an error. So I will fix this too. 3 - The last two queries give an error. This patch do not handle such queries intentionally, because there is the thread https://www.postgresql.org/message-id/57786490.9010201%40wars-nicht.de . That thread concerns to_date() function. But it should concerns to_timestamp() also. So I suppose that should be a different patch for this last case. -- Artur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
Monday, 15 August 2016 9:58 PM, Robert Haas wrote: >On Mon, Aug 15, 2016 at 10:56 AM, amul sul wrote: >> On Thursday, 11 August 2016 3:18 PM, Artur Zakirov >> wrote: [Skipped..] >Well, what's the Oracle behavior in any of these cases? I don't think >we can decide to change any of this behavior without knowing that. If >a proposed behavior change is incompatible with our previous releases, >I think it'd better at least be more compatible with Oracle. >Otherwise, we're just changing from an established behavior that we >invented ourselves to a new behavior we invented ourselves, which is >only worthwhile if it's absolutely clear that the new behavior is way >better. Following cases works as expected on Oracle (except 3rd one asking input value for &15). >> SELECT TO_TIMESTAMP('2015-12-31 13:43:36', ' MM DD HH24 MI SS'); >> SELECT TO_TIMESTAMP('2011$03!18 23_38_15', '-MM-DD HH24:MI:SS'); >> SELECT TO_TIMESTAMP('2011*03*18 23^38&15', '-MM-DD HH24:MI:SS'); >> SELECT TO_TIMESTAMP('2011*03!18 #%23^38$15', '-MM-DD$$$HH24:MI:SS'); And rest throwing error as shown below: >> SELECT TO_TIMESTAMP('2016-06-13 99:99:99', '-MM-DD HH24:MI:SS'); ERROR at line 1: ORA-01850: hour must be between 0 and 23 >> SELECT TO_TIMESTAMP('2016-02-30 15:43:36', '-MM-DD HH24:MI:SS'); ERROR at line 1: ORA-01839: date not valid for month specified >(Also, note that text formatted email is generally preferred to HTML >on this mailing list; the fact that your email is in a different font >than the rest of the thread makes it hard to read.) Understood. Will try to follow this, thanks. Regards, Amul Sul -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Mon, Aug 15, 2016 at 10:56 AM, amul sul wrote: > On Thursday, 11 August 2016 3:18 PM, Artur Zakirov > wrote: > >>Here is my patch. It is a proof of concept. >>Date/Time Formatting >> >>There are changes in date/time formatting rules: > -> now to_timestamp() and to_date() skip spaces in the input string and >>in the formatting string unless FX option is used, as Amul Sul wrote on >>first message of this thread. But Ex.2 gives an error now with this >>patch (should we fix this too?). > > Why not, currently we are skipping whitespace exists at the start of input > string but not if in format string. > > [Skipped… ] > >>Of course this patch can be completely wrong. But it tries to introduce >>more formal rules for formatting. >>I will be grateful for notes and remarks. > > Following are few scenarios where we break existing behaviour: > > SELECT TO_TIMESTAMP('2015-12-31 13:43:36', ' MM DD HH24 MI SS'); > SELECT TO_TIMESTAMP('2011$03!18 23_38_15', '-MM-DD HH24:MI:SS'); > SELECT TO_TIMESTAMP('2011*03*18 23^38&15', '-MM-DD HH24:MI:SS'); > SELECT TO_TIMESTAMP('2011*03!18 #%23^38$15', '-MM-DD$$$HH24:MI:SS'); > > But current patch behaviour is not that much bad either at least we have > errors, but I am not sure about community acceptance. > > I would like to divert communities' attention on following case: > SELECT TO_TIMESTAMP('2013--10-01', '-MM-DD'); > > Where the hyphen (-) is not skipped. So ultimately -10 is interpreted using > MM as negative 10. So the date goes back by that many months (and probably > additional days because of -31), and so the final output becomes 2012-01-30. > But the fix is not specific to hyphen case. Ideally the fix would have been > to handle it in from_char_parse_int(). Here, -10 is converted to int using > strtol. May be we could have done it using strtoul(). Is there any intention > behind not considering signed integers versus unsigned ones ? > > Another is, shouldn’t we have error in following cases? > SELECT TO_TIMESTAMP('2016-06-13 99:99:99', '-MM-DD HH24:MI:SS'); > SELECT TO_TIMESTAMP('2016-02-30 15:43:36', '-MM-DD HH24:MI:SS'); Well, what's the Oracle behavior in any of these cases? I don't think we can decide to change any of this behavior without knowing that. If a proposed behavior change is incompatible with our previous releases, I think it'd better at least be more compatible with Oracle. Otherwise, we're just changing from an established behavior that we invented ourselves to a new behavior we invented ourselves, which is only worthwhile if it's absolutely clear that the new behavior is way better. (Also, note that text formatted email is generally preferred to HTML on this mailing list; the fact that your email is in a different font than the rest of the thread makes it hard to read.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Thursday, 11 August 2016 3:18 PM, Artur Zakirov wrote: >Here is my patch. It is a proof of concept.>Date/Time >Formatting>>There are changes in date/time formatting >rules:-> now to_timestamp() and to_date() skip spaces in the input string and >>in the formatting string unless FX option is used, as Amul Sul wrote on >>first message of this thread. But Ex.2 gives an error now with this >patch >(should we fix this too?). Why not, currently we are skipping whitespace exists at the start of input string but not if in format string. [Skipped… ] >Of course this patch can be completely wrong. But it tries to introduce >more >formal rules for formatting.>I will be grateful for notes and remarks. Following are few scenarios where we break existing behaviour: SELECT TO_TIMESTAMP('2015-12-31 13:43:36', ' MM DD HH24 MI SS');SELECT TO_TIMESTAMP('2011$03!18 23_38_15', '-MM-DD HH24:MI:SS');SELECT TO_TIMESTAMP('2011*03*18 23^38&15', '-MM-DD HH24:MI:SS');SELECT TO_TIMESTAMP('2011*03!18 #%23^38$15', '-MM-DD$$$HH24:MI:SS'); But current patch behaviour is not that much bad either at least we have errors, but I am not sure about community acceptance. I would like to divert communities' attention on following case:SELECT TO_TIMESTAMP('2013--10-01', '-MM-DD'); Where the hyphen (-) is not skipped. So ultimately -10 is interpreted using MM as negative 10. So the date goes back by that many months (and probably additional days because of -31), and so the final output becomes 2012-01-30. But the fix is not specific to hyphen case. Ideally the fix would have been to handle it in from_char_parse_int(). Here, -10 is converted to int using strtol. May be we could have done it using strtoul(). Is there any intention behind not considering signed integers versus unsigned ones ? Another is, shouldn’t we have error in following cases? SELECT TO_TIMESTAMP('2016-06-13 99:99:99', '-MM-DD HH24:MI:SS'); SELECT TO_TIMESTAMP('2016-02-30 15:43:36', '-MM-DD HH24:MI:SS'); Thanks & Regards,Amul Sul
Re: [HACKERS] Bug in to_timestamp().
Hello, On 14.07.2016 12:16, Pavel Stehule wrote: last point was discussed in thread related to to_date_valid function. Regards Pavel Thank you. Here is my patch. It is a proof of concept. Date/Time Formatting There are changes in date/time formatting rules: - now to_timestamp() and to_date() skip spaces in the input string and in the formatting string unless FX option is used, as Amul Sul wrote on first message of this thread. But Ex.2 gives an error now with this patch (should we fix this too?). - in the code space characters and separator characters have different types of FormatNode. Separator characters are characters ',', '-', '.', '/' and ':'. This is done to have different rules of formatting to space and separator characters. If FX option isn't used then PostgreSQL do not insist that separator in the formatting string should match separator in the formatting string. But count of separators should be equal with or without FX option. - now PostgreSQL check is there a closing quote. Otherwise the error is raised. Still PostgreSQL do not insist that text character in the formatting string should match text character in the input string. It is not obvious if this should be fixed. Because we may have different character case or character with accent mark or without accent mark. But I suppose that it is not right just check text character count. For example, there is unicode version of space character U+00A0. Code changes - new defines: #define NODE_TYPE_SEPARATOR 4 #define NODE_TYPE_SPACE 5 - now DCH_cache_getnew() is called after parse_format(). Because now parse_format() can raise an error and in the next attempt DCH_cache_search() could return broken cache entry. This patch do not handle all noticed issues in this thread, since still there is not consensus about them. So this patch in a proof of concept status and it can be changed. Of course this patch can be completely wrong. But it tries to introduce more formal rules for formatting. I will be grateful for notes and remarks. -- Artur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 7830334..5656245 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -6083,9 +6083,10 @@ SELECT regexp_matches('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}'); to_timestamp and to_date - skip multiple blank spaces in the input string unless the - FX option is used. For example, - to_timestamp('2000JUN', ' MON') works, but + skip multiple blank spaces in the input string and in the formatting + string unless the FX option is used. For example, + to_timestamp('2000JUN', ' MON') + and to_timestamp('2000 JUN', 'MON') work, but to_timestamp('2000JUN', 'FX MON') returns an error because to_timestamp expects one space only. FX must be specified as the first item in @@ -6121,6 +6122,19 @@ SELECT regexp_matches('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}'); skips two input characters. + + + + In to_date and to_timestamp separator + characters ',', '-', + '.', '/' and ':' + outside of double-quoted strings skip the number of input characters + contained in the string unless the FX option is used. + If FX option is specified then consumed separator + character in the formatting string must match the separator character + in the input string. + + diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c index bbd97dc..ef39df9 100644 --- a/src/backend/utils/adt/formatting.c +++ b/src/backend/utils/adt/formatting.c @@ -169,6 +169,8 @@ struct FormatNode #define NODE_TYPE_END 1 #define NODE_TYPE_ACTION 2 #define NODE_TYPE_CHAR 3 +#define NODE_TYPE_SEPARATOR 4 +#define NODE_TYPE_SPACE 5 #define SUFFTYPE_PREFIX 1 #define SUFFTYPE_POSTFIX 2 @@ -947,6 +949,7 @@ typedef struct NUMProc static const KeyWord *index_seq_search(char *str, const KeyWord *kw, const int *index); static const KeySuffix *suff_search(char *str, const KeySuffix *suf, int type); +static bool is_char_separator(char *str); static void NUMDesc_prepare(NUMDesc *num, FormatNode *n); static void parse_format(FormatNode *node, char *str, const KeyWord *kw, const KeySuffix *suf, const int *index, int ver, NUMDesc *Num); @@ -963,7 +966,6 @@ static void dump_node(FormatNode *node, int max); static const char *get_th(char *num, int type); static char *str_numth(char *dest, char *num, int type); static int adjust_partial_year_to_2020(int year); -static int strspace_len(char *str); static void from_char_set_mode(TmFromChar *tmfc, const FromCharDateMode mode); static void from_char_set_int(int *dest, const int value, const FormatNode *node); stat
Re: [HACKERS] Bug in to_timestamp().
2016-07-14 11:05 GMT+02:00 Artur Zakirov : > On 23.06.2016 21:02, Tom Lane wrote: > >> Robert Haas writes: >> >>> On Thu, Jun 23, 2016 at 1:40 PM, Tom Lane wrote: >>> At the very least I'd want to see a thought-through proposal that addresses all three of these interrelated points: * what should a space in the format match * what should a non-space, non-format-code character in the format match * how should we handle fields that are not exactly the width suggested by the format >>> >> I'm not averse to some further study of those issues, and I think the >>> first two are closely related. The third one strikes me as a somewhat >>> separate consideration that doesn't need to be addressed by the same >>> patch. >>> >> >> If you think those issues are not interrelated, you have not thought >> about it carefully enough. >> >> As an example, what we can do to handle not-expected-width fields is >> very different if the format is "DDMMYY" versus if it is "DD-MM-YY". >> In the first case we have little choice but to believe that each >> field is exactly two digits wide. In the second case, depending on >> how we decide to define matching of "-", we might be able to allow >> the field widths to vary so that they're effectively "whatever is >> between the dashes". But that would require insisting that "-" >> match a "-", or at least a non-alphanumeric, which is not how it >> behaves today. >> >> I don't want to twiddle these behaviors in 9.6 and then again next year. >> >> regards, tom lane >> >> >> > Hi, > > I want to start work on this patch. > > As a conclusion: > - need a decision about three questions: > > >> * what should a space in the format match >> * what should a non-space, non-format-code character in the format match >> * how should we handle fields that are not exactly the width suggested >> by the format >> > > - nobody wants solve this issue in 9.6. > > And I have question: what about wrong input in date argument? For example, > from Alex's message: > > postgres=# SELECT TO_TIMESTAMP('2016-02-30 15:43:36', '-MM-DD >> HH24:MI:SS'); >>to_timestamp >> >> 2016-03-01 15:43:36+03 >> (1 row) >> > > Here '2016-02-30' is wrong date. I didn't see any conclusion about this > case in the thread. > last point was discussed in thread related to to_date_valid function. Regards Pavel > > -- > Artur Zakirov > Postgres Professional: http://www.postgrespro.com > Russian Postgres Company > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] Bug in to_timestamp().
On 23.06.2016 21:02, Tom Lane wrote: Robert Haas writes: On Thu, Jun 23, 2016 at 1:40 PM, Tom Lane wrote: At the very least I'd want to see a thought-through proposal that addresses all three of these interrelated points: * what should a space in the format match * what should a non-space, non-format-code character in the format match * how should we handle fields that are not exactly the width suggested by the format I'm not averse to some further study of those issues, and I think the first two are closely related. The third one strikes me as a somewhat separate consideration that doesn't need to be addressed by the same patch. If you think those issues are not interrelated, you have not thought about it carefully enough. As an example, what we can do to handle not-expected-width fields is very different if the format is "DDMMYY" versus if it is "DD-MM-YY". In the first case we have little choice but to believe that each field is exactly two digits wide. In the second case, depending on how we decide to define matching of "-", we might be able to allow the field widths to vary so that they're effectively "whatever is between the dashes". But that would require insisting that "-" match a "-", or at least a non-alphanumeric, which is not how it behaves today. I don't want to twiddle these behaviors in 9.6 and then again next year. regards, tom lane Hi, I want to start work on this patch. As a conclusion: - need a decision about three questions: * what should a space in the format match * what should a non-space, non-format-code character in the format match * how should we handle fields that are not exactly the width suggested by the format - nobody wants solve this issue in 9.6. And I have question: what about wrong input in date argument? For example, from Alex's message: postgres=# SELECT TO_TIMESTAMP('2016-02-30 15:43:36', '-MM-DD HH24:MI:SS'); to_timestamp 2016-03-01 15:43:36+03 (1 row) Here '2016-02-30' is wrong date. I didn't see any conclusion about this case in the thread. -- Artur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Thu, Jun 23, 2016 at 02:00:49PM -0400, Robert Haas wrote: > > Ok. I'm having trouble seeing this justified as a bug fix - the docs > > clearly state our behavior is intentional. Improved behavior, yes, but > > that's a feature and we're in beta2. Please be specific if you believe I've > > misinterpreted project policies on this matter. > > I would not vote to back-patch a change in this area because I think > that could break SQL code that works today. But I think the current > behavior is pretty well indefensible. It's neither intuitive nor > compatible with Oracle. And there is plenty of existing precedent for > making this sort of change during the beta period. We regard it as a > bug fix which is too dangerous to back-patch; therefore, it is neither > subject to feature freeze nor does it go into existing stable > releases. Whether to treat this particular issue in that particular > way is something that needs to be hammered out in discussion. Tom > raises the valid point that we need to make sure that we've thoroughly > studied this issue and fixed all of the related cases before > committing anything -- we don't want to change the behavior in > 9.6beta, release 9.6, and then have to change it again for 9.7. But > there is certainly no project policy which says we can't change this > now for 9.6 if we decide that (1) the current behavior is wrong and > (2) we are quite sure we know how to fix it. If you are not going to backpatch this for compatibility reasons, I don't think changing it during beta makes sense either because you open the chance of breaking applications that have already been tested on earlier 9.6 betas. This would only make sense if the to_timestamp bugs were new in 9.6. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Fri, Jun 24, 2016 at 5:16 PM, Tom Lane wrote: > Robert Haas writes: >> On Fri, Jun 24, 2016 at 12:26 PM, Steve Crawford >> wrote: >>> To me, 2016-02-30 is an invalid date that should generate an error. > >> I don't particularly disagree with that, but on the other hand, as >> mentioned earlier, to_timestamp() is here for Oracle compatibility, >> and if it doesn't do what Oracle's function does, then (1) it's not >> useful for people migrating from Oracle and (2) we're making up the >> behavior out of whole cloth. I think things that we invent ourselves >> should reject stuff like this, but in a compatibility function we >> might want to, say, have compatibility. > > Agreed, mostly, but ... how far are we prepared to go on that? The one > thing I know about that is different from Oracle and is not something that > most people would consider clearly wrong is the behavior of the FM prefix. > We think it's a prefix that modifies only the next format code; they think > it's a toggle. If we make that act like Oracle, we will silently break an > awful lot of applications, and there will be *no way* to write code that > is correct under both interpretations. (And no, I do not want to hear > "let's fix it with a GUC".) So I'm afraid we're between a rock and a hard > place on that one --- but if we let that stand, the argument that Oracle's > to_timestamp should be treated as right by definition loses a lot of air. Well, I think that you're making several logical jumps that I personally would decline to make. First, I don't think every issue with these functions needs to be handled in the same way as every other. Just because we're willing or unwilling to break compatibility in one area doesn't mean we have to make the same decision in every case. We're allowed to take into effect the likely impact of making a given change in deciding whether it's worth it. Second, if in one or more areas we decide that a hard backward compatibility break would be too painful, then I think it's a good idea to ask ourselves how we could ease the migration pain for people. And I'd phrase that as an open-ended question rather than "should we add a GUC?". For example, one idea here is to create a to_timstamp_old() function that retains the existing behavior of to_timestamp() without any change, and then add a new to_timestamp() function and fix every Oracle incompatibility we can find as thoroughly as we can do in one release cycle. So we fix this whitespace stuff, we fix the FM modifier, and anything else that comes up, we fix it all. Then, if people run into trouble with the new behavior when they upgrade, we tell them that they can either fix their application or, if they want the old behavior, they can use to_timestamp_old(). We can also document the differences between to_timestamp() and to_timestamp_old() so that people can easily figure out whether those differences are significant to them. Another idea is to add an optional third argument to to_timestamp() that can be used to set compatibility behaviors. I'm not altogether convinced that it's worth the effort to provide lots of backward-compatibility here. Presumably, only a small percentage of people use to_timestamp(), and only a percentage of those are going to rely on the details we're talking about changing. So it might be that if we just up and change this, a few people will be grumpy and then they'll update their code and that will be it. On the other hand, maybe it'll be a real pain in the butt for lots of people and we'll lose users. I don't know how to judge how significant these changes will be to users, and I think that the level of impact does matter in deciding what to do. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Fri, Jun 24, 2016 at 3:43 PM, Joshua D. Drake wrote: > On 06/24/2016 02:16 PM, Tom Lane wrote: > >> Robert Haas writes: >> >>> On Fri, Jun 24, 2016 at 12:26 PM, Steve Crawford >>> wrote: >>> To me, 2016-02-30 is an invalid date that should generate an error. >>> >> I don't particularly disagree with that, but on the other hand, as >>> mentioned earlier, to_timestamp() is here for Oracle compatibility, >>> and if it doesn't do what Oracle's function does, then (1) it's not >>> useful for people migrating from Oracle and (2) we're making up the >>> behavior out of whole cloth. I think things that we invent ourselves >>> should reject stuff like this, but in a compatibility function we >>> might want to, say, have compatibility. >>> >> >> Agreed, mostly, but ... how far are we prepared to go on that? >> > > We don't at all. Our goal has never been Oracle compatibility. Yes, we > have "made allowances" but we aren't in a position that requires that > anymore. > > Let's just do it right. > > Sincerely, > > JD > > /me speaking as someone who handles many, many migrations, none of which > have ever said, "do we have Oracle compatibility available". > > Tongue (partlyish) in cheek: Developer: I need a database to support my project. Based on my research this PostgreSQL thing is awesome so we will use it. PostgreSQL: Welcome to our community! Developer: I need to convert a string to a timestamp. This to_timestamp() function I tried does not operate as I expect based on the documentation. PostgreSQL: Ah, yes, grasshopper. You are young and do not understand the Things That Must Not Be Documented . In time you will grow a gray ponytail and/or white beard and learn the history and ways of every database that came before. Only then will you come to understand how The Functions *truly* behave. Developer: Are you #@%!$ kidding me? I will allow that there may be selected cases where a good argument could be made for intentionally overly permissive behavior in the pursuit of compatibility. But in those cases the documentation should specify clearly and in detail the deviant behavior and reason for its existence. As one who selected PostgreSQL from the start, I am more interested in the functions working correctly. Cheers, Steve
Re: [HACKERS] Bug in to_timestamp().
On 06/24/2016 02:16 PM, Tom Lane wrote: Robert Haas writes: On Fri, Jun 24, 2016 at 12:26 PM, Steve Crawford wrote: To me, 2016-02-30 is an invalid date that should generate an error. I don't particularly disagree with that, but on the other hand, as mentioned earlier, to_timestamp() is here for Oracle compatibility, and if it doesn't do what Oracle's function does, then (1) it's not useful for people migrating from Oracle and (2) we're making up the behavior out of whole cloth. I think things that we invent ourselves should reject stuff like this, but in a compatibility function we might want to, say, have compatibility. Agreed, mostly, but ... how far are we prepared to go on that? We don't at all. Our goal has never been Oracle compatibility. Yes, we have "made allowances" but we aren't in a position that requires that anymore. Let's just do it right. Sincerely, JD /me speaking as someone who handles many, many migrations, none of which have ever said, "do we have Oracle compatibility available". -- Command Prompt, Inc. http://the.postgres.company/ +1-503-667-4564 PostgreSQL Centered full stack support, consulting and development. Everyone appreciates your honesty, until you are honest with them. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On 25/06/16 08:33, Robert Haas wrote: On Fri, Jun 24, 2016 at 12:26 PM, Steve Crawford wrote: My observation has been that the PostgreSQL development group aims for correctness and the elimination of surprising results. This was part of the reason to eliminate a number of automatic casts to dates in earlier versions. To me, 2016-02-30 is an invalid date that should generate an error. Automatically and silently changing it to be 2016-03-01 strikes me as a behavior I'd expect from a certain other open-source database, not PostgreSQL. I don't particularly disagree with that, but on the other hand, as mentioned earlier, to_timestamp() is here for Oracle compatibility, and if it doesn't do what Oracle's function does, then (1) it's not useful for people migrating from Oracle and (2) we're making up the behavior out of whole cloth. I think things that we invent ourselves should reject stuff like this, but in a compatibility function we might want to, say, have compatibility. How about a to_timestamp_strict(), in addition? Its very existence will help people (who bother to read the documentation) to look more closely at the differences between the definitions, and allow them to choose the most appropriate for their use case. Cheers, Gavin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
Robert Haas writes: > On Fri, Jun 24, 2016 at 12:26 PM, Steve Crawford > wrote: >> To me, 2016-02-30 is an invalid date that should generate an error. > I don't particularly disagree with that, but on the other hand, as > mentioned earlier, to_timestamp() is here for Oracle compatibility, > and if it doesn't do what Oracle's function does, then (1) it's not > useful for people migrating from Oracle and (2) we're making up the > behavior out of whole cloth. I think things that we invent ourselves > should reject stuff like this, but in a compatibility function we > might want to, say, have compatibility. Agreed, mostly, but ... how far are we prepared to go on that? The one thing I know about that is different from Oracle and is not something that most people would consider clearly wrong is the behavior of the FM prefix. We think it's a prefix that modifies only the next format code; they think it's a toggle. If we make that act like Oracle, we will silently break an awful lot of applications, and there will be *no way* to write code that is correct under both interpretations. (And no, I do not want to hear "let's fix it with a GUC".) So I'm afraid we're between a rock and a hard place on that one --- but if we let that stand, the argument that Oracle's to_timestamp should be treated as right by definition loses a lot of air. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Fri, Jun 24, 2016 at 12:26 PM, Steve Crawford wrote: > My observation has been that the PostgreSQL development group aims for > correctness and the elimination of surprising results. This was part of the > reason to eliminate a number of automatic casts to dates in earlier > versions. > > To me, 2016-02-30 is an invalid date that should generate an error. > Automatically and silently changing it to be 2016-03-01 strikes me as a > behavior I'd expect from a certain other open-source database, not > PostgreSQL. I don't particularly disagree with that, but on the other hand, as mentioned earlier, to_timestamp() is here for Oracle compatibility, and if it doesn't do what Oracle's function does, then (1) it's not useful for people migrating from Oracle and (2) we're making up the behavior out of whole cloth. I think things that we invent ourselves should reject stuff like this, but in a compatibility function we might want to, say, have compatibility. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On 06/24/2016 09:26 AM, Steve Crawford wrote: My observation has been that the PostgreSQL development group aims for correctness and the elimination of surprising results. This was part of the reason to eliminate a number of automatic casts to dates in earlier versions. To me, 2016-02-30 is an invalid date that should generate an error. Automatically and silently changing it to be 2016-03-01 strikes me as a behavior I'd expect from a certain other open-source database, not PostgreSQL. I don't think anybody could argue with that in good faith. Sincerely, JD -- Command Prompt, Inc. http://the.postgres.company/ +1-503-667-4564 PostgreSQL Centered full stack support, consulting and development. Everyone appreciates your honesty, until you are honest with them. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
My observation has been that the PostgreSQL development group aims for correctness and the elimination of surprising results. This was part of the reason to eliminate a number of automatic casts to dates in earlier versions. To me, 2016-02-30 is an invalid date that should generate an error. Automatically and silently changing it to be 2016-03-01 strikes me as a behavior I'd expect from a certain other open-source database, not PostgreSQL. Cheers, Steve On Fri, Jun 24, 2016 at 8:52 AM, Alex Ignatov wrote: > > Alex Ignatov > Postgres Professional: http://www.postgrespro.com > The Russian Postgres Company > > On 20.06.2016 17:09, Albe Laurenz wrote: > >> Tom Lane wrote: >> >>> I don't necessarily have an opinion yet. I would like to see more than >>> just an unsupported assertion about what Oracle's behavior is. Also, >>> how should FM mode affect this? >>> >> I can supply what Oracle 12.1 does: >> >> SQL> SELECT to_timestamp('2016-06-13 15:43:36', ' /MM/DD HH24:MI:SS') >> AS ts FROM dual; >> >> TS >> >> 2016-06-13 15:43:36.0 AD >> >> SQL> SELECT to_timestamp('2016-06-13 15:43:36', '/MM/DD HH24:MI:SS') >> AS ts FROM dual; >> >> TS >> >> 2016-06-13 15:43:36.0 AD >> >> SQL> SELECT to_timestamp('2016-06-1315:43:36', '/MM/DD >> HH24:MI:SS') AS ts FROM dual; >> >> TS >> >> 2016-06-13 15:43:36.0 AD >> >> (to_timestamp_tz behaves the same way.) >> >> So Oracle seems to make no difference between one or more spaces. >> >> Yours, >> Laurenz Albe >> >> Guys, do we need to change this behavior or may be you can tell me that > is normal because this and this: > > postgres=# SELECT TO_TIMESTAMP('2016-02-30 15:43:36', '-MM-DD > HH24:MI:SS'); > to_timestamp > > 2016-03-01 15:43:36+03 > (1 row) > > but on the other side we have : > > postgres=# select '2016-02-30 15:43:36'::timestamp; > ERROR: date/time field value out of range: "2016-02-30 15:43:36" > LINE 1: select '2016-02-30 15:43:36'::timestamp; > > Another bug in to_timestamp/date()? > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] Bug in to_timestamp().
Alex Ignatov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company On 20.06.2016 17:09, Albe Laurenz wrote: Tom Lane wrote: I don't necessarily have an opinion yet. I would like to see more than just an unsupported assertion about what Oracle's behavior is. Also, how should FM mode affect this? I can supply what Oracle 12.1 does: SQL> SELECT to_timestamp('2016-06-13 15:43:36', ' /MM/DD HH24:MI:SS') AS ts FROM dual; TS 2016-06-13 15:43:36.0 AD SQL> SELECT to_timestamp('2016-06-13 15:43:36', '/MM/DD HH24:MI:SS') AS ts FROM dual; TS 2016-06-13 15:43:36.0 AD SQL> SELECT to_timestamp('2016-06-1315:43:36', '/MM/DD HH24:MI:SS') AS ts FROM dual; TS 2016-06-13 15:43:36.0 AD (to_timestamp_tz behaves the same way.) So Oracle seems to make no difference between one or more spaces. Yours, Laurenz Albe Guys, do we need to change this behavior or may be you can tell me that is normal because this and this: postgres=# SELECT TO_TIMESTAMP('2016-02-30 15:43:36', '-MM-DD HH24:MI:SS'); to_timestamp 2016-03-01 15:43:36+03 (1 row) but on the other side we have : postgres=# select '2016-02-30 15:43:36'::timestamp; ERROR: date/time field value out of range: "2016-02-30 15:43:36" LINE 1: select '2016-02-30 15:43:36'::timestamp; Another bug in to_timestamp/date()? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On 23.06.2016 20:40, Tom Lane wrote: Robert Haas writes: On Thu, Jun 23, 2016 at 1:12 PM, David G. Johnston wrote: My understanding is that is not going to change for 9.6. That's exactly what is under discussion here. I would definitely agree with David on that point. Making to_timestamp noticeably better on this score seems like a nontrivial project, and post-beta is not the time for that sort of thing, even if we had full consensus on what to do. I'd suggest somebody work on a patch and put it up for review in the next cycle. Now, if you were to narrowly define the problem as "whether to skip non-spaces for a space in the format", maybe that could be fixed post-beta, but I think that's a wrongheaded approach. to_timestamp's issues with input that doesn't match the format are far wider than that. IMO we should try to resolve the whole problem with one coherent change, not make incremental incompatible changes at the margins. At the very least I'd want to see a thought-through proposal that addresses all three of these interrelated points: * what should a space in the format match * what should a non-space, non-format-code character in the format match * how should we handle fields that are not exactly the width suggested by the format regards, tom lane Totally agree that we need more discussion about error handling in this function! Also this behavior is observed in to_date() and to_number() function: postgres=# SELECT TO_DATE('2!0!1!6!0!6-/-/-/-/-/-/-1!/-/-/-/-/-/-/-3!', '-MM-DD'); to_date 0002-01-01 (1 row) postgres=# postgres=# select to_number('1$#@!!,2,%,%4,5,@%5@4..8-', '999G999D9S'); to_number --- 12 (1 row) On the our side we have some discussions about to write a patch that will change this incorrect behavior. So stay tuned. -- Alex Ignatov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Thu, Jun 23, 2016 at 2:00 PM, Robert Haas wrote: > On Thu, Jun 23, 2016 at 1:46 PM, David G. Johnston > wrote: > >> Sheesh. Who put you in charge of this? You basically seem like you > >> are trying to shut up anyone who supports this change, and I don't > >> think that's right. > > > > I'm all for a change in this area - though I'm not impacted enough to > > actually work on a design proposal. > > You weren't for a change in this area a few emails ago; back then, you > said "I don't see that changing it is a worthwhile endeavor". > I still don't see changing to_timestamp as being the right approach...but that's just my opinion. I do think that this area "text to timestamp conversions" could stand to be improved. I may not have been clear on that distinction. But changing the behavior of a function that has been around since 2000 seems to be just asking for trouble. > > And I'm not sure how asking for ideas > > constitutes trying to shut people up. Especially since if no one does > float > > a proposal we'll simply have this discussion next year when someone new > > discovers how badly behaved this function is. > > In my opinion, telling people that their emails are not constructive > and that no change is possible for 9.6 is pretty much the same thing > as trying to shut people up. And that's what you did. > I guess I should be a bit more careful to make sure that two separate responses on different aspects of a topic cannot be so easily construed as "this thread is pointless". To be clear I did and still do believe that getting a change in this area into 10.0 is worthwhile; and that our policy (and present circumstances) appears to preclude this changing in 9.6. But as noted below this is just an observation. > >> My main point is that I'm inclined to deprecate it. > >> > >> I can almost guarantee that would make a lot of users very unhappy. > >> This function is widely used. > > > > Tell people not to use. We'd leave it in, unchanged, on backward > > compatibility grounds. This seems like acceptable behavior for the > project. > > Am I mistaken? > > Yes. We're not going to deprecate widely-used functionality. We > might, however, fix it, contrary to your representations upthread. > At this point I feel you are cherry-picking my words to fit your present feelings. I stand by everything I've written upthread regarding deprecation and fixing. I'm personally in favor of the former. I'll add that you are a committer, I am not. The one thing going for change is if we indeed exactly match the reference behavior and then document it as being a compatibility function. I hadn't fully pondered this goal and how its plays into changing 16 year old behavior. Obviously a new name for the function doesn't cut it in this scenario. > >> > My second point is if you are going to use this badly designed > function > >> > you > >> > need to protect yourself. > >> > >> I agree that anyone using this function should test their format > >> strings carefully. > >> > >> > My understanding is that is not going to change for 9.6. > >> > >> That's exactly what is under discussion here. > >> > > > > Ok. I'm having trouble seeing this justified as a bug fix - the docs > > clearly state our behavior is intentional. Improved behavior, yes, but > > that's a feature and we're in beta2. Please be specific if you believe > I've > > misinterpreted project policies on this matter. > > I would not vote to back-patch a change in this area because I think > that could break SQL code that works today. But I think the current > behavior is pretty well indefensible. It's neither intuitive nor > compatible with Oracle. And there is plenty of existing precedent for > making this sort of change during the beta period. We regard it as a > bug fix which is too dangerous to back-patch; therefore, it is neither > subject to feature freeze nor does it go into existing stable > releases. Whether to treat this particular issue in that particular > way is something that needs to be hammered out in discussion. Tom > raises the valid point that we need to make sure that we've thoroughly > studied this issue and fixed all of the related cases before > committing anything -- we don't want to change the behavior in > 9.6beta, release 9.6, and then have to change it again for 9.7. But > there is certainly no project policy which says we can't change this > now for 9.6 if we decide that (1) the current behavior is wrong and > (2) we are quite sure we know how to fix it. > > Thank You. I still conclude that given the general lack of complaints, the fact we are at beta2, the age of the feature and nature of the "bug", and the non-existence of a working patch even for HEAD, that 9.6 is not going to see this behavior changed and you will be loath to back-patch a fix once 9.6 becomes stable. I'll admit the possibility does exist but am not all that keen on couching every statement of this form I make. If you find my i
Re: [HACKERS] Bug in to_timestamp().
On 6/23/16 12:29 PM, Alvaro Herrera wrote: David G. Johnston wrote: On Thursday, June 23, 2016, Alex Ignatov wrote: Arguing just like that one can say that we don't even need exception like "division by zero". Just use well-formed numbers in denominator... Input data sometimes can be generated automagically. Without exception throwing debugging stored function containing to_timestamp can be painful. to_timestamp with its present behavior is, IMO, a poorly designed function that would never be accepted today. I'm not sure about that. to_timestamp was added to improve compatibility with Oracle, by commit b866d2e2d794. I suppose the spec should follow what's documented here, http://docs.oracle.com/cd/B19306_01/server.102/b14200/functions193.htm http://docs.oracle.com/cd/B19306_01/server.102/b14200/sql_elements004.htm#i34924 and that wherever we deviate from that, is a bug that should be fixed. +1 I'm also in favor of a parsing function that actually follows the format specifier, but not enough to write a patch. One thing that I think could happen for 9.6 is documenting how you could get the desired results with one of the regex functions. Not the nicest way to handle this problem, but it is workable and having a regex example available for people to start with would be very helpful. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
Robert Haas writes: > On Thu, Jun 23, 2016 at 1:40 PM, Tom Lane wrote: >> At the very least I'd want to see a thought-through proposal that >> addresses all three of these interrelated points: >> >> * what should a space in the format match >> * what should a non-space, non-format-code character in the format match >> * how should we handle fields that are not exactly the width suggested >> by the format > I'm not averse to some further study of those issues, and I think the > first two are closely related. The third one strikes me as a somewhat > separate consideration that doesn't need to be addressed by the same > patch. If you think those issues are not interrelated, you have not thought about it carefully enough. As an example, what we can do to handle not-expected-width fields is very different if the format is "DDMMYY" versus if it is "DD-MM-YY". In the first case we have little choice but to believe that each field is exactly two digits wide. In the second case, depending on how we decide to define matching of "-", we might be able to allow the field widths to vary so that they're effectively "whatever is between the dashes". But that would require insisting that "-" match a "-", or at least a non-alphanumeric, which is not how it behaves today. I don't want to twiddle these behaviors in 9.6 and then again next year. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Thu, Jun 23, 2016 at 1:46 PM, David G. Johnston wrote: >> Sheesh. Who put you in charge of this? You basically seem like you >> are trying to shut up anyone who supports this change, and I don't >> think that's right. > > I'm all for a change in this area - though I'm not impacted enough to > actually work on a design proposal. You weren't for a change in this area a few emails ago; back then, you said "I don't see that changing it is a worthwhile endeavor". > And I'm not sure how asking for ideas > constitutes trying to shut people up. Especially since if no one does float > a proposal we'll simply have this discussion next year when someone new > discovers how badly behaved this function is. In my opinion, telling people that their emails are not constructive and that no change is possible for 9.6 is pretty much the same thing as trying to shut people up. And that's what you did. >> My main point is that I'm inclined to deprecate it. >> >> I can almost guarantee that would make a lot of users very unhappy. >> This function is widely used. > > Tell people not to use. We'd leave it in, unchanged, on backward > compatibility grounds. This seems like acceptable behavior for the project. > Am I mistaken? Yes. We're not going to deprecate widely-used functionality. We might, however, fix it, contrary to your representations upthread. >> > My second point is if you are going to use this badly designed function >> > you >> > need to protect yourself. >> >> I agree that anyone using this function should test their format >> strings carefully. >> >> > My understanding is that is not going to change for 9.6. >> >> That's exactly what is under discussion here. >> > > Ok. I'm having trouble seeing this justified as a bug fix - the docs > clearly state our behavior is intentional. Improved behavior, yes, but > that's a feature and we're in beta2. Please be specific if you believe I've > misinterpreted project policies on this matter. I would not vote to back-patch a change in this area because I think that could break SQL code that works today. But I think the current behavior is pretty well indefensible. It's neither intuitive nor compatible with Oracle. And there is plenty of existing precedent for making this sort of change during the beta period. We regard it as a bug fix which is too dangerous to back-patch; therefore, it is neither subject to feature freeze nor does it go into existing stable releases. Whether to treat this particular issue in that particular way is something that needs to be hammered out in discussion. Tom raises the valid point that we need to make sure that we've thoroughly studied this issue and fixed all of the related cases before committing anything -- we don't want to change the behavior in 9.6beta, release 9.6, and then have to change it again for 9.7. But there is certainly no project policy which says we can't change this now for 9.6 if we decide that (1) the current behavior is wrong and (2) we are quite sure we know how to fix it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Thursday, June 23, 2016, Robert Haas wrote: > On Thu, Jun 23, 2016 at 1:12 PM, David G. Johnston > > wrote: > > to_timestamp with its present behavior is, IMO, a poorly designed > function > > that would never be accepted today. Concrete proposals for either > fixing or > > deprecating (or both) are welcome. Fixing it should not cause > unnecessary > > errors to be raised. > > Sheesh. Who put you in charge of this? You basically seem like you > are trying to shut up anyone who supports this change, and I don't > think that's right. > I'm all for a change in this area - though I'm not impacted enough to actually work on a design proposal. And I'm not sure how asking for ideas constitutes trying to shut people up. Especially since if no one does float a proposal we'll simply have this discussion next year when someone new discovers how badly behaved this function is. > My main point is that I'm inclined to deprecate it. > > I can almost guarantee that would make a lot of users very unhappy. > This function is widely used. > > Tell people not to use. We'd leave it in, unchanged, on backward compatibility grounds. This seems like acceptable behavior for the project. Am I mistaken? > > My second point is if you are going to use this badly designed function > you > > need to protect yourself. > > I agree that anyone using this function should test their format > strings carefully. > > > My understanding is that is not going to change for 9.6. > > That's exactly what is under discussion here. > > Ok. I'm having trouble seeing this justified as a bug fix - the docs clearly state our behavior is intentional. Improved behavior, yes, but that's a feature and we're in beta2. Please be specific if you believe I've misinterpreted project policies on this matter. David J.
Re: [HACKERS] Bug in to_timestamp().
On Thu, Jun 23, 2016 at 1:40 PM, Tom Lane wrote: > Robert Haas writes: >> On Thu, Jun 23, 2016 at 1:12 PM, David G. Johnston >> wrote: >>> My understanding is that is not going to change for 9.6. > >> That's exactly what is under discussion here. > > I would definitely agree with David on that point. Making to_timestamp > noticeably better on this score seems like a nontrivial project, and > post-beta is not the time for that sort of thing, even if we had full > consensus on what to do. I'd suggest somebody work on a patch and put > it up for review in the next cycle. > > Now, if you were to narrowly define the problem as "whether to skip > non-spaces for a space in the format", maybe that could be fixed > post-beta, but I think that's a wrongheaded approach. to_timestamp's > issues with input that doesn't match the format are far wider than that. > IMO we should try to resolve the whole problem with one coherent change, > not make incremental incompatible changes at the margins. > > At the very least I'd want to see a thought-through proposal that > addresses all three of these interrelated points: > > * what should a space in the format match > * what should a non-space, non-format-code character in the format match > * how should we handle fields that are not exactly the width suggested > by the format I'm not averse to some further study of those issues, and I think the first two are closely related. The third one strikes me as a somewhat separate consideration that doesn't need to be addressed by the same patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
Robert Haas writes: > On Thu, Jun 23, 2016 at 1:12 PM, David G. Johnston > wrote: >> My understanding is that is not going to change for 9.6. > That's exactly what is under discussion here. I would definitely agree with David on that point. Making to_timestamp noticeably better on this score seems like a nontrivial project, and post-beta is not the time for that sort of thing, even if we had full consensus on what to do. I'd suggest somebody work on a patch and put it up for review in the next cycle. Now, if you were to narrowly define the problem as "whether to skip non-spaces for a space in the format", maybe that could be fixed post-beta, but I think that's a wrongheaded approach. to_timestamp's issues with input that doesn't match the format are far wider than that. IMO we should try to resolve the whole problem with one coherent change, not make incremental incompatible changes at the margins. At the very least I'd want to see a thought-through proposal that addresses all three of these interrelated points: * what should a space in the format match * what should a non-space, non-format-code character in the format match * how should we handle fields that are not exactly the width suggested by the format regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
David G. Johnston wrote: > On Thursday, June 23, 2016, Alex Ignatov wrote: > > > Arguing just like that one can say that we don't even need exception like > > "division by zero". Just use well-formed numbers in denominator... > > Input data sometimes can be generated automagically. Without exception > > throwing debugging stored function containing to_timestamp can be painful. > > to_timestamp with its present behavior is, IMO, a poorly designed function > that would never be accepted today. I'm not sure about that. to_timestamp was added to improve compatibility with Oracle, by commit b866d2e2d794. I suppose the spec should follow what's documented here, http://docs.oracle.com/cd/B19306_01/server.102/b14200/functions193.htm http://docs.oracle.com/cd/B19306_01/server.102/b14200/sql_elements004.htm#i34924 and that wherever we deviate from that, is a bug that should be fixed. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Thu, Jun 23, 2016 at 1:12 PM, David G. Johnston wrote: > to_timestamp with its present behavior is, IMO, a poorly designed function > that would never be accepted today. Concrete proposals for either fixing or > deprecating (or both) are welcome. Fixing it should not cause unnecessary > errors to be raised. Sheesh. Who put you in charge of this? You basically seem like you are trying to shut up anyone who supports this change, and I don't think that's right. Alex's opinion is just as valid as yours - neither more nor less - and he has every right to express it without being told by you that his emails are "not constructive". > My main point is that I'm inclined to deprecate it. I can almost guarantee that would make a lot of users very unhappy. This function is widely used. > My second point is if you are going to use this badly designed function you > need to protect yourself. I agree that anyone using this function should test their format strings carefully. > My understanding is that is not going to change for 9.6. That's exactly what is under discussion here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Thu, Jun 23, 2016 at 12:37 PM, David G. Johnston wrote > To be honest I don't see how this is relevant to quoted content. And you've > already made this point quite clearly - repeating it isn't constructive. > This behavior has existed for a long time and I don't see that changing it > is a worthwhile endeavor. I believe a new function is required that has > saner behavior. Otherwise given good input and a well-formed parse string > the function does exactly what it is designed to do. Avoid giving it > garbage and you will be fine. Maybe wrap the call to the in a function that > also checks for the expected layout and RAISE EXCEPTION if it doesn't match. Well, I think other people are allowed to disagree about whether changing it is a worthwhile endeavor. I think there's an excellent argument that the current behavior is stupid and broken and probably nobody is intentionally relying on it. Obviously, if somebody is relying on the existing behavior and we change it and it breaks things, then that's bad, and a good argument for worrying about backward-compatibility - e.g. by adding a new function. But who would actually like the behavior that an extra space in the format string causes non-whitespace characters to get skipped? Next you'll be arguing that we can't fix a server crash that's triggered by a certain query because somebody might be relying on that query to restart the server... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Thursday, June 23, 2016, Alex Ignatov wrote: > Arguing just like that one can say that we don't even need exception like > "division by zero". Just use well-formed numbers in denominator... > Input data sometimes can be generated automagically. Without exception > throwing debugging stored function containing to_timestamp can be painful. > to_timestamp with its present behavior is, IMO, a poorly designed function that would never be accepted today. Concrete proposals for either fixing or deprecating (or both) are welcome. Fixing it should not cause unnecessary errors to be raised. My main point is that I'm inclined to deprecate it. My second point is if you are going to use this badly designed function you need to protect yourself. My understanding is that is not going to change for 9.6. David J.
Re: [HACKERS] Bug in to_timestamp().
On 23.06.2016 19:37, David G. Johnston wrote: On Thu, Jun 23, 2016 at 12:16 PM, Alex Ignatov mailto:a.igna...@postgrespro.ru>>wrote: On 23.06.2016 16:30, Bruce Momjian wrote: On Thu, Jun 23, 2016 at 07:41:26AM +, amul sul wrote: On Monday, 20 June 2016 8:53 PM, Alex Ignatov mailto:a.igna...@postgrespro.ru>> wrote: On 13.06.2016 18:52, amul sul wrote: And it wont stop on some simple whitespace. By using to_timestamp you can get any output results by providing illegal input parameters values: postgres=# SELECT TO_TIMESTAMP('2016-06-13 99 :99:99', 'MMDD HH24:MI:SS'); to_timestamp 2016-01-06 14:40:39+03 (1 row) We do consume extra space from input string, but not if it is in format string, see below: postgres=# SELECT TO_TIMESTAMP('2016-06-13 15:43:36', '/MM/DD HH24:MI:SS'); to_timestamp 2016-06-13 15:43:36-07 (1 row) We should have same treatment for format string too. Thoughts? Comments? Well, the user specifies the format string, while the input string comes from the data, so I don't see having them behave the same as necessary. To be honest they not just behave differently. to_timestamp is just incorrectly handles input data and nothing else.There is no excuse for such behavior: postgres=# SELECT TO_TIMESTAMP('20:-16-06:13: 15_43:!36', '/MM/DD HH24:MI:SS'); to_timestamp -- 0018-08-05 13:15:43+02:30:17 (1 row) T o be honest I don't see how this is relevant to quoted content. And you've already made this point quite clearly - repeating it isn't constructive. This behavior has existed for a long time and I don't see that changing it is a worthwhile endeavor. I believe a new function is required that has saner behavior. Otherwise given good input and a well-formed parse string the function does exactly what it is designed to do. Avoid giving it garbage and you will be fine. Maybe wrap the call to the in a function that also checks for the expected layout and RAISE EXCEPTION if it doesn't match. David J. Arguing just like that one can say that we don't even need exception like "division by zero". Just use well-formed numbers in denominator... Input data sometimes can be generated automagically. Without exception throwing debugging stored function containing to_timestamp can be painful.
Re: [HACKERS] Bug in to_timestamp().
On Thu, Jun 23, 2016 at 12:16 PM, Alex Ignatov wrote: > > On 23.06.2016 16:30, Bruce Momjian wrote: > >> On Thu, Jun 23, 2016 at 07:41:26AM +, amul sul wrote: >> >>> On Monday, 20 June 2016 8:53 PM, Alex Ignatov >>> wrote: >>> >>> >>> On 13.06.2016 18:52, amul sul wrote: > And it wont stop on some simple whitespace. By using to_timestamp you can get any output results by providing illegal input parameters values: postgres=# SELECT TO_TIMESTAMP('2016-06-13 99:99:99', 'MMDD HH24:MI:SS'); to_timestamp 2016-01-06 14:40:39+03 (1 row) >>> We do consume extra space from input string, but not if it is in format >>> string, see below: >>> >>> postgres=# SELECT TO_TIMESTAMP('2016-06-13 15:43:36', '/MM/DD >>> HH24:MI:SS'); >>> to_timestamp >>> >>> 2016-06-13 15:43:36-07 >>> (1 row) >>> >>> We should have same treatment for format string too. >>> >>> Thoughts? Comments? >>> >> Well, the user specifies the format string, while the input string comes >> from the data, so I don't see having them behave the same as necessary. >> >> > To be honest they not just behave differently. to_timestamp is just > incorrectly handles input data and nothing else.There is no excuse for > such behavior: > > postgres=# SELECT TO_TIMESTAMP('20:-16-06:13: 15_43:!36', '/MM/DD > HH24:MI:SS'); > to_timestamp > -- > 0018-08-05 13:15:43+02:30:17 > (1 row) > T o be honest I don't see how this is relevant to quoted content. And you've already made this point quite clearly - repeating it isn't constructive. This behavior has existed for a long time and I don't see that changing it is a worthwhile endeavor. I believe a new function is required that has saner behavior. Otherwise given good input and a well-formed parse string the function does exactly what it is designed to do. Avoid giving it garbage and you will be fine. Maybe wrap the call to the in a function that also checks for the expected layout and RAISE EXCEPTION if it doesn't match. David J.
Re: [HACKERS] Bug in to_timestamp().
On 23.06.2016 16:30, Bruce Momjian wrote: On Thu, Jun 23, 2016 at 07:41:26AM +, amul sul wrote: On Monday, 20 June 2016 8:53 PM, Alex Ignatov wrote: On 13.06.2016 18:52, amul sul wrote: And it wont stop on some simple whitespace. By using to_timestamp you can get any output results by providing illegal input parameters values: postgres=# SELECT TO_TIMESTAMP('2016-06-13 99:99:99', 'MMDD HH24:MI:SS'); to_timestamp 2016-01-06 14:40:39+03 (1 row) We do consume extra space from input string, but not if it is in format string, see below: postgres=# SELECT TO_TIMESTAMP('2016-06-13 15:43:36', '/MM/DD HH24:MI:SS'); to_timestamp 2016-06-13 15:43:36-07 (1 row) We should have same treatment for format string too. Thoughts? Comments? Well, the user specifies the format string, while the input string comes from the data, so I don't see having them behave the same as necessary. To be honest they not just behave differently. to_timestamp is just incorrectly handles input data and nothing else.There is no excuse for such behavior: postgres=# SELECT TO_TIMESTAMP('20:-16-06:13: 15_43:!36', '/MM/DD HH24:MI:SS'); to_timestamp -- 0018-08-05 13:15:43+02:30:17 (1 row) Alex Ignatov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Thu, Jun 23, 2016 at 07:41:26AM +, amul sul wrote: > On Monday, 20 June 2016 8:53 PM, Alex Ignatov > wrote: > > > >>On 13.06.2016 18:52, amul sul wrote: > >And it wont stop on some simple whitespace. By using to_timestamp you > >can get any output results by providing illegal input parameters values: > > >postgres=# SELECT TO_TIMESTAMP('2016-06-13 99:99:99', 'MMDD > >HH24:MI:SS'); > > to_timestamp > > > > 2016-01-06 14:40:39+03 > > > > (1 row) > > We do consume extra space from input string, but not if it is in format > string, see below: > > postgres=# SELECT TO_TIMESTAMP('2016-06-13 15:43:36', '/MM/DD > HH24:MI:SS'); > to_timestamp > > 2016-06-13 15:43:36-07 > (1 row) > > We should have same treatment for format string too. > > Thoughts? Comments? Well, the user specifies the format string, while the input string comes from the data, so I don't see having them behave the same as necessary. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Monday, 20 June 2016 8:53 PM, Alex Ignatov wrote: >>On 13.06.2016 18:52, amul sul wrote: >And it wont stop on some simple whitespace. By using to_timestamp you >can get any output results by providing illegal input parameters values: >postgres=# SELECT TO_TIMESTAMP('2016-06-13 99:99:99', 'MMDD >HH24:MI:SS'); > to_timestamp > > 2016-01-06 14:40:39+03 > > (1 row) We do consume extra space from input string, but not if it is in format string, see below: postgres=# SELECT TO_TIMESTAMP('2016-06-13 15:43:36', '/MM/DD HH24:MI:SS'); to_timestamp 2016-06-13 15:43:36-07 (1 row) We should have same treatment for format string too. Thoughts? Comments? Regards, Amul Sul -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On 13.06.2016 18:52, amul sul wrote: Hi, It's look like bug in to_timestamp() function when format string has more whitespaces compare to input string, see below: Ex.1: Two white spaces before HH24 whereas one before input time string postgres=# SELECT TO_TIMESTAMP('2016-06-13 15:43:36', '/MM/DD HH24:MI:SS'); to_timestamp 2016-06-13 05:43:36-07 <— incorrect time (1 row) Ex.2: One whitespace before format string postgres=# SELECT TO_TIMESTAMP('2016/06/13 15:43:36', ' /MM/DD HH24:MI:SS'); to_timestamp -- 0016-06-13 15:43:36-07:52:58 <— incorrect year (1 row) If there are one or more consecutive whitespace in the format, we should skip those as long as we could get an actual field. Thoughts? Thanks & Regards, Amul Sul From docs about to_timestamp() ( https://www.postgresql.org/docs/9.5/static/functions-formatting.html) "These functions interpret input liberally, with minimal error checking. While they produce valid output, the conversion can yield unexpected results. For example, input to these functions is not restricted by normal ranges, thus to_date('20096040','MMDD') returns 2014-01-17 rather than causing an error. Casting does not have this behavior." And it wont stop on some simple whitespace. By using to_timestamp you can get any output results by providing illegal input parameters values: postgres=# SELECT TO_TIMESTAMP('2016-06-13 99:99:99', 'MMDD HH24:MI:SS'); to_timestamp 2016-01-06 14:40:39+03 (1 row) Alex Ignatov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On 20.06.2016 16:36, Tom Lane wrote: Robert Haas writes: On Mon, Jun 13, 2016 at 12:25 PM, Robert Haas wrote: I think a space in the format string should skip a whitespace character in the input string, but not a non-whitespace character. It's my understanding that these functions exist in no small part for compatibility with Oracle, and Oracle declines to skip the digit '1' on the basis of an extra space in the format string, which IMHO is the behavior any reasonable user would expect. So Amul and I are of one opinion and Tom is of another. Anyone else have an opinion? I don't necessarily have an opinion yet. I would like to see more than just an unsupported assertion about what Oracle's behavior is. Also, how should FM mode affect this? regards, tom lane Oracle: SQL> SELECT TO_TIMESTAMP('2016-06-13 99:99:99', 'MMDD HH24:MI:SS') from dual; SELECT TO_TIMESTAMP('2016-06-13 99:99:99', 'MMDD HH24:MI:SS') from dual * ERROR at line 1: ORA-01843: not a valid month PG: postgres=# SELECT TO_TIMESTAMP('2016-06-13 99:99:99', 'MMDD HH24:MI:SS'); to_timestamp 2016-01-06 14:40:39+03 (1 row) I know about: "These functions interpret input liberally, with minimal error checking. While they produce valid output, the conversion can yield unexpected results" from docs but by providing illegal input parameters we have no any exceptions or errors about that. I think that to_timestamp() need to has more format checking than it has now. Alex Ignatov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
Tom Lane wrote: > I don't necessarily have an opinion yet. I would like to see more than > just an unsupported assertion about what Oracle's behavior is. Also, > how should FM mode affect this? I can supply what Oracle 12.1 does: SQL> SELECT to_timestamp('2016-06-13 15:43:36', ' /MM/DD HH24:MI:SS') AS ts FROM dual; TS 2016-06-13 15:43:36.0 AD SQL> SELECT to_timestamp('2016-06-13 15:43:36', '/MM/DD HH24:MI:SS') AS ts FROM dual; TS 2016-06-13 15:43:36.0 AD SQL> SELECT to_timestamp('2016-06-1315:43:36', '/MM/DD HH24:MI:SS') AS ts FROM dual; TS 2016-06-13 15:43:36.0 AD (to_timestamp_tz behaves the same way.) So Oracle seems to make no difference between one or more spaces. Yours, Laurenz Albe -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
Robert Haas writes: > On Mon, Jun 13, 2016 at 12:25 PM, Robert Haas wrote: >> I think a space in the format string should skip a whitespace >> character in the input string, but not a non-whitespace character. >> It's my understanding that these functions exist in no small part for >> compatibility with Oracle, and Oracle declines to skip the digit '1' >> on the basis of an extra space in the format string, which IMHO is the >> behavior any reasonable user would expect. > So Amul and I are of one opinion and Tom is of another. Anyone else > have an opinion? I don't necessarily have an opinion yet. I would like to see more than just an unsupported assertion about what Oracle's behavior is. Also, how should FM mode affect this? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Mon, Jun 20, 2016 at 8:19 AM, Robert Haas wrote: > On Mon, Jun 13, 2016 at 12:25 PM, Robert Haas > wrote: > > On Mon, Jun 13, 2016 at 12:12 PM, Tom Lane wrote: > >> amul sul writes: > >>> It's look like bug in to_timestamp() function when format string has > more whitespaces compare to input string, see below: > >> > >> No, I think this is a case of "input doesn't match the format string". > >> > >> As a rule of thumb, using to_timestamp() for input that could be parsed > >> just fine by the standard timestamp input function is not a particularly > >> good idea. to_timestamp() is meant to deal with input that is in a > >> well-defined format that happens to not be parsable by timestamp_in. > >> This example doesn't meet either of those preconditions. > > > > I think a space in the format string should skip a whitespace > > character in the input string, but not a non-whitespace character. > > It's my understanding that these functions exist in no small part for > > compatibility with Oracle, and Oracle declines to skip the digit '1' > > on the basis of an extra space in the format string, which IMHO is the > > behavior any reasonable user would expect. > > So Amul and I are of one opinion and Tom is of another. Anyone else > have an opinion? > > At least Tom's position has the benefit of being consistent with current behavior. The current implementation doesn't actually care what literal value you specify - any non-special character consumes a single token from the input, period. SELECT TO_TIMESTAMP('2016-06-13 15:43:36', '/MM/DD--HH24:MI:SS'); SELECT TO_TIMESTAMP('2016-06-13 15:43:36', '/MM/DD-HH24:MI:SS'); Both the above exhibit the same behavior as if you used a space instead of the hyphen as the group separator. The documentation should be updated to make this particular dynamic more clear. I don't see changing the general behavior of these "date formatting" functions a worthwhile endeavor. Adding a less-liberal "parse_timestamp" function I could get behind. IOW, the user seems to be happy with the fact that the "/" in the date matches his "-" but them complains that the space matches the number "1". You don't get to have it both ways. [re-reads the third usage note] Or maybe you do. We already define space as a being a greedy operator (when not used in conjunction with FX). A greedy space-space sequence makes little sense on its face and if we are going to be helpful here we should treat it as a single greedy space matcher. Note that "returns an error because to_timestamp expects one space only" is wrong - it errors because only a single space is captured and then the attempt to parse 'JUN' using "MON" fails. The following query doesn't fail though it exhibits the same space discrepancy (it just gives the same "wrong" result). SELECT TO_TIMESTAMP('2016-06-13 15:43:36', 'FX/MM/DD HH24:MI:SS'); Given that we already partially special-case the space expression I'd be inclined to consider Robert's and Amul's position on the matter. I think I'd redefine our treatment of space to be "zero or more" instead of "one or more" and require that it only match a literal space in the input. Having considered that, I'm not convinced its worth a compatibility break. I'd much rather deprecate these versions and write slightly-less-liberal versions named . In any case I'd called the present wording a bug. Instead: A single space consumes a single token of input and then, unless the FX modifier is present, consumes zero or more subsequent literal spaces. Thus, using two spaces in a row without the FX modifier, while allowed, is unlikely to give you a satisfactory result. The first space will consume all available consecutive spaces so that the second space will be guaranteed to consume a non-space token from the input. David J.
Re: [HACKERS] Bug in to_timestamp().
On Mon, Jun 13, 2016 at 12:25 PM, Robert Haas wrote: > On Mon, Jun 13, 2016 at 12:12 PM, Tom Lane wrote: >> amul sul writes: >>> It's look like bug in to_timestamp() function when format string has more >>> whitespaces compare to input string, see below: >> >> No, I think this is a case of "input doesn't match the format string". >> >> As a rule of thumb, using to_timestamp() for input that could be parsed >> just fine by the standard timestamp input function is not a particularly >> good idea. to_timestamp() is meant to deal with input that is in a >> well-defined format that happens to not be parsable by timestamp_in. >> This example doesn't meet either of those preconditions. > > I think a space in the format string should skip a whitespace > character in the input string, but not a non-whitespace character. > It's my understanding that these functions exist in no small part for > compatibility with Oracle, and Oracle declines to skip the digit '1' > on the basis of an extra space in the format string, which IMHO is the > behavior any reasonable user would expect. So Amul and I are of one opinion and Tom is of another. Anyone else have an opinion? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Monday, 13 June 2016 9:57 PM, Robert Haas wrote: >I think a space in the format string should skip a whitespace >character in the input string, but not a non-whitespace character. +1, enough the benefit is we are giving correct output. PFA patch proposing this fix. Regards, Amul Sul. 0001-RM37358-space-in-the-format-string-should-skip-a-whi.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
On Mon, Jun 13, 2016 at 12:12 PM, Tom Lane wrote: > amul sul writes: >> It's look like bug in to_timestamp() function when format string has more >> whitespaces compare to input string, see below: > > No, I think this is a case of "input doesn't match the format string". > > As a rule of thumb, using to_timestamp() for input that could be parsed > just fine by the standard timestamp input function is not a particularly > good idea. to_timestamp() is meant to deal with input that is in a > well-defined format that happens to not be parsable by timestamp_in. > This example doesn't meet either of those preconditions. I think a space in the format string should skip a whitespace character in the input string, but not a non-whitespace character. It's my understanding that these functions exist in no small part for compatibility with Oracle, and Oracle declines to skip the digit '1' on the basis of an extra space in the format string, which IMHO is the behavior any reasonable user would expect. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in to_timestamp().
amul sul writes: > It's look like bug in to_timestamp() function when format string has more > whitespaces compare to input string, see below: No, I think this is a case of "input doesn't match the format string". As a rule of thumb, using to_timestamp() for input that could be parsed just fine by the standard timestamp input function is not a particularly good idea. to_timestamp() is meant to deal with input that is in a well-defined format that happens to not be parsable by timestamp_in. This example doesn't meet either of those preconditions. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers