BUG #16419: wrong parsing BC year in to_date() function
The following bug has been logged on the website:
Bug reference: 16419
Logged by: Saeed Hubaishan
Email address: dar_alathar@hotmail.com
PostgreSQL version: 12.2
Operating system: Windows 10x64
Description:
select to_date('-1-01-01','yyyy-mm-dd');
will get
0002-01-01 BC
On Wed, May 6, 2020 at 2:58 PM PG Bug reporting form <noreply@postgresql.org>
wrote:
The following bug has been logged on the website:
Bug reference: 16419
Logged by: Saeed Hubaishan
Email address: dar_alathar@hotmail.com
PostgreSQL version: 12.2
Operating system: Windows 10x64
Description:select to_date('-1-01-01','yyyy-mm-dd');
will get
0002-01-01 BC
Yep...
select to_date('1','YYYY')::text; // Year 1 AD
select to_date('0','YYYY')::text; // Year 1 BC (there is no year zero)
select to_date('-1','YYYY')::text; // Year 2 BC
to_date tries very hard to not error - if you need to use it make sure your
data conforms to the format you specify.
David J.
Any one suppose that these functions return the same:
make_date(-1,1,1)
to_date('-1-01-01','yyyy-mm-dd')
But make_date will give 0001-01-01 BC
And to_date will give 0002-01-01 BC
If you think this is right behavior I think this must be documented
من: David G. Johnston <david.g.johnston@gmail.com>
تم الإرسال: Thursday, May 7, 2020 1:45:14 AM
إلى: dar_alathar@hotmail.com <dar_alathar@hotmail.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
الموضوع: Re: BUG #16419: wrong parsing BC year in to_date() function
On Wed, May 6, 2020 at 2:58 PM PG Bug reporting form <noreply@postgresql.org<mailto:noreply@postgresql.org>> wrote:
The following bug has been logged on the website:
Bug reference: 16419
Logged by: Saeed Hubaishan
Email address: dar_alathar@hotmail.com<mailto:dar_alathar@hotmail.com>
PostgreSQL version: 12.2
Operating system: Windows 10x64
Description:
select to_date('-1-01-01','yyyy-mm-dd');
will get
0002-01-01 BC
Yep...
select to_date('1','YYYY')::text; // Year 1 AD
select to_date('0','YYYY')::text; // Year 1 BC (there is no year zero)
select to_date('-1','YYYY')::text; // Year 2 BC
to_date tries very hard to not error - if you need to use it make sure your data conforms to the format you specify.
David J.
من: David G. Johnston<mailto:david.g.johnston@gmail.com>
إرسال: الخميس, 14 رمضان, 1441 01:45 ص
الموضوع: Re: BUG #16419: wrong parsing BC year in to_date() function
On Wed, May 6, 2020 at 2:58 PM PG Bug reporting form <noreply@postgresql.org<mailto:noreply@postgresql.org>> wrote:
The following bug has been logged on the website:
Bug reference: 16419
Logged by: Saeed Hubaishan
Email address: dar_alathar@hotmail.com<mailto:dar_alathar@hotmail.com>
PostgreSQL version: 12.2
Operating system: Windows 10x64
Description:
select to_date('-1-01-01','yyyy-mm-dd');
will get
0002-01-01 BC
Yep...
select to_date('1','YYYY')::text; // Year 1 AD
select to_date('0','YYYY')::text; // Year 1 BC (there is no year zero)
select to_date('-1','YYYY')::text; // Year 2 BC
to_date tries very hard to not error - if you need to use it make sure your data conforms to the format you specify.
David J.
Attachments:
On Wed, May 6, 2020 at 6:31 PM دار الآثار للنشر والتوزيع-صنعاء Dar
Alathar-Yemen <dar_alathar@hotmail.com> wrote:
Any one suppose that these functions return the same:
make_date(-1,1,1)
to_date('-1-01-01','yyyy-mm-dd')But make_date will give 0001-01-01 BC
And to_date will give 0002-01-01 BC
Interesting...and a fair point.
What seems to be happening here is that to_date is trying to be helpful by
doing:
select to_date('0000','YYYY'); // 0001-01-01 BC
It does this seemingly by subtracting one from the year, making it
positive, then (I infer) appending "BC" to the result. Thus for the year
"-1" it yields "0002-01-01 BC"
make_date just chooses to reject the year 0 and treat the negative as an
alternative to specifying BC
There seems to be zero tests for to_date involving negative years, and the
documentation doesn't talk of them.
I'll let the -hackers speak up as to how they want to go about handling
to_date (research how it behaves in the other database it tries to emulate
and either document or possibly change the behavior in v14) but do suggest
that a simple explicit description of how to_date works in the presence of
negative years be back-patched. A bullet in the usage notes section
probably suffices:
"If a YYYY format string captures a negative year, or 0000, it will treat
it as a BC year after decreasing the value by one. So 0000 maps to 1 BC
and -1 maps to 2 BC and so on."
So, no, make_date and to_date do not agree on this point; and they do not
have to. There is no way to specify "BC" in make_date function so using
negative there makes sense. You can specify BC in the input string for
to_date and indeed that is the only supported (documented) way to do so.
David J.
On Wed, May 6, 2020 at 8:12 PM David G. Johnston <david.g.johnston@gmail.com>
wrote:
It does this seemingly by subtracting one from the year, making it
positive, then (I infer) appending "BC" to the result. Thus for the year
"-1" it yields "0002-01-01 BC"
Specifically:
/*
* There is no 0 AD. Years go from 1 BC to 1 AD, so we make it
* positive and map year == -1 to year zero, and shift all negative
* years up one. For interval years, we just return the year.
*/
#define ADJUST_YEAR(year, is_interval) ((is_interval) ? (year) : ((year) <=
0 ? -((year) - 1) : (year)))
The code comment took me a bit to process - seems like the following would
be better (if its right - I don't know why interval is a pure no-op while
non-interval normalizes to a positive integer).
Years go from 1 BC to 1 AD, so we adjust the year zero, and all negative
years, by shifting them away one year, We then return the positive value
of the result because the caller tracks the BC/AD aspect of the year
separately and only deals with positive year values coming out of this
macro. Intervals denote the distance away from 0 a year is so we can
simply take the supplied value and return it. Interval processing code
expects a negative result for intervals going into BC.
David J.
To make "to_date" work as "make_date" with negative years these llines:
https://github.com/postgres/postgres/blob/fb544735f11480a697fcab791c058adc166be1fa/src/backend/utils/adt/formatting.c#L4559-L4560 :
if (tmfc.bc && tm->tm_year > 0)
tm->tm_year = -(tm->tm_year - 1);
must be changed to:
if (tmfc.bc && tm->tm_year > 0)
{
tm->tm_year = -(tm->tm_year - 1);
}
else if (tm->tm_year < 0) {
tm->tm_year ++;
}
research how it behaves in the other database it tries to emulate and either document or possibly change the behavior in v14
As in https://stackoverflow.com/questions/6779521/how-do-i-insert-a-bc-date-into-oracle and http://rwijk.blogspot.com/2008/10/year-zero.html
In Oracle
to_date('-4700/01/01','syyyy/mm/dd')
returns
01/01/4700 BC
In documents https://docs.oracle.com/cd/B28359_01/olap.111/b28126/dml_commands_1029.htm#OLADM780
YEAR
SYEAR
Year, spelled out; S prefixes BC dates with a minus sign (-).
YYYY
SYYYY
4-digit year; S prefixes BC dates with a minus sign.
Redirecting to -hackers for visibility. I feel there needs to be something
done here, even if just documentation (a bullet in the usage notes section
- and a code comment update for the macro) pointing this out and not
changing any behavior.
David J.
On Wed, May 6, 2020 at 8:12 PM David G. Johnston <david.g.johnston@gmail.com>
wrote:
On Wed, May 6, 2020 at 6:31 PM دار الآثار للنشر والتوزيع-صنعاء Dar
Alathar-Yemen <dar_alathar@hotmail.com> wrote:Any one suppose that these functions return the same:
make_date(-1,1,1)
to_date('-1-01-01','yyyy-mm-dd')But make_date will give 0001-01-01 BC
And to_date will give 0002-01-01 BC
Interesting...and a fair point.
What seems to be happening here is that to_date is trying to be helpful by
doing:select to_date('0000','YYYY'); // 0001-01-01 BC
It does this seemingly by subtracting one from the year, making it
positive, then (I infer) appending "BC" to the result. Thus for the year
"-1" it yields "0002-01-01 BC"make_date just chooses to reject the year 0 and treat the negative as an
alternative to specifying BCThere seems to be zero tests for to_date involving negative years, and the
documentation doesn't talk of them.I'll let the -hackers speak up as to how they want to go about handling
to_date (research how it behaves in the other database it tries to emulate
and either document or possibly change the behavior in v14) but do suggest
that a simple explicit description of how to_date works in the presence of
negative years be back-patched. A bullet in the usage notes section
probably suffices:"If a YYYY format string captures a negative year, or 0000, it will treat
it as a BC year after decreasing the value by one. So 0000 maps to 1 BC
and -1 maps to 2 BC and so on."So, no, make_date and to_date do not agree on this point; and they do not
have to. There is no way to specify "BC" in make_date function so using
negative there makes sense. You can specify BC in the input string for
to_date and indeed that is the only supported (documented) way to do so.
[and the next email]
Show quoted text
Specifically:
/*
* There is no 0 AD. Years go from 1 BC to 1 AD, so we make it
* positive and map year == -1 to year zero, and shift all negative
* years up one. For interval years, we just return the year.
*/
#define ADJUST_YEAR(year, is_interval) ((is_interval) ? (year) : ((year)
<= 0 ? -((year) - 1) : (year)))The code comment took me a bit to process - seems like the following would
be better (if its right - I don't know why interval is a pure no-op while
non-interval normalizes to a positive integer).Years go from 1 BC to 1 AD, so we adjust the year zero, and all negative
years, by shifting them away one year, We then return the positive value
of the result because the caller tracks the BC/AD aspect of the year
separately and only deals with positive year values coming out of this
macro. Intervals denote the distance away from 0 a year is so we can
simply take the supplied value and return it. Interval processing code
expects a negative result for intervals going into BC.David J.
On Tue, 2020-05-12 at 18:09 -0700, David G. Johnston wrote:
Redirecting to -hackers for visibility. I feel there needs to be something done here, even if just documentation (a bullet in the usage notes section - and a code comment update for the macro)
pointing this out and not changing any behavior.David J.
On Wed, May 6, 2020 at 8:12 PM David G. Johnston <david.g.johnston@gmail.com> wrote:
On Wed, May 6, 2020 at 6:31 PM دار الآثار للنشر والتوزيع-صنعاء Dar Alathar-Yemen <dar_alathar@hotmail.com> wrote:
Any one suppose that these functions return the same:
make_date(-1,1,1)
to_date('-1-01-01','yyyy-mm-dd')But make_date will give 0001-01-01 BC
And to_date will give 0002-01-01 BC
Interesting...and a fair point.
What seems to be happening here is that to_date is trying to be helpful by doing:
select to_date('0000','YYYY'); // 0001-01-01 BC
It does this seemingly by subtracting one from the year, making it positive, then (I infer) appending "BC" to the result. Thus for the year "-1" it yields "0002-01-01 BC"
make_date just chooses to reject the year 0 and treat the negative as an alternative to specifying BC
There seems to be zero tests for to_date involving negative years, and the documentation doesn't talk of them.
I'll let the -hackers speak up as to how they want to go about handling to_date (research how it behaves in the other database it tries to emulate and either document or possibly change the
behavior in v14) but do suggest that a simple explicit description of how to_date works in the presence of negative years be back-patched. A bullet in the usage notes section probably suffices:"If a YYYY format string captures a negative year, or 0000, it will treat it as a BC year after decreasing the value by one. So 0000 maps to 1 BC and -1 maps to 2 BC and so on."
So, no, make_date and to_date do not agree on this point; and they do not have to. There is no way to specify "BC" in make_date function so using negative there makes sense. You can specify BC
in the input string for to_date and indeed that is the only supported (documented) way to do so.[and the next email]
Specifically:
/*
* There is no 0 AD. Years go from 1 BC to 1 AD, so we make it
* positive and map year == -1 to year zero, and shift all negative
* years up one. For interval years, we just return the year.
*/
#define ADJUST_YEAR(year, is_interval) ((is_interval) ? (year) : ((year) <= 0 ? -((year) - 1) : (year)))The code comment took me a bit to process - seems like the following would be better (if its right - I don't know why interval is a pure no-op while non-interval normalizes to a positive integer).
Years go from 1 BC to 1 AD, so we adjust the year zero, and all negative years, by shifting them away one year, We then return the positive value of the result because the caller tracks the BC/AD
aspect of the year separately and only deals with positive year values coming out of this macro. Intervals denote the distance away from 0 a year is so we can simply take the supplied value and
return it. Interval processing code expects a negative result for intervals going into BC.David J.
Since "to_date" is an Oracle compatibility function, here is what Oracle 18.4 has to say to that:
SQL> SELECT to_date('0000', 'YYYY') FROM dual;
SELECT to_date('0000', 'YYYY') FROM dual
*
ERROR at line 1:
ORA-01841: (full) year must be between -4713 and +9999, and not be 0
SQL> SELECT to_date('-0001', 'YYYY') FROM dual;
SELECT to_date('-0001', 'YYYY') FROM dual
*
ERROR at line 1:
ORA-01841: (full) year must be between -4713 and +9999, and not be 0
SQL> SELECT to_date('-0001', 'SYYYY') FROM dual;
TO_DATE('-0001','SYYYY
----------------------
0001-05-01 00:00:00 BC
Yours,
Laurenz Albe
On Tue, May 12, 2020 at 8:56 PM Laurenz Albe <laurenz.albe@cybertec.at>
wrote:
On Tue, 2020-05-12 at 18:09 -0700, David G. Johnston wrote:
Redirecting to -hackers for visibility. I feel there needs to be
something done here, even if just documentation (a bullet in the usage
notes section - and a code comment update for the macro)pointing this out and not changing any behavior.
Since "to_date" is an Oracle compatibility function, here is what Oracle
18.4 has to say to that:SQL> SELECT to_date('0000', 'YYYY') FROM dual;
SELECT to_date('0000', 'YYYY') FROM dual
*
ERROR at line 1:
ORA-01841: (full) year must be between -4713 and +9999, and not be 0
Attached is a concrete patch (back-patchable hopefully) documenting the
current reality.
As noted in the patch commit message (commentary really):
make_timestamp not agreeing with make_date on how to handle negative years
should probably just be fixed - but that is for someone else to handle.
Whether to actually change the behavior of to_date is up for debate though
I would presume it would not be back-patched.
David J.
Attachments:
v1-001-to-date-behavior-in-bc-bug16419.patchapplication/octet-stream; name=v1-001-to-date-behavior-in-bc-bug16419.patchDownload
commit 553a298004352baacdac2b91c5c762947730e83c
Author: David G. Johnston <david.g.johnston@gmail.com>
Date: Wed Jul 15 16:11:08 2020 +0000
Document BC date construction behaviors
Bug# 16419 complains that to_date('-1-01-01','yyyy-mm-dd') returns
'0002-01-01 BC' violating POLA.
Add tests and documentation describing how the to_* and make_* functions
behave when given input <= 0.
This documents two bugs:
Our reference implementation for to_date doesn't accept zero as a
year nor shifts negative years one year earlier.
make_timestamp will not accept a negative year while make_date will.
The second bug seems worth fixing and back-patching in lieu of the
documentation change herein.
The first bug seems worth fixing in HEAD while back-patching
that portion of this patch.
I (DGJ) choose this approach (with suggested back-patch) as changing
the behaviors is outside of my current skillset.
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 9d71678029..4c86dbfb06 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -7720,6 +7720,18 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
</caution>
</listitem>
+ <listitem>
+ <para>
+ In <function>to_timestamp</function> and <function>to_date</function>,
+ the year <literal>0</literal> is an accepted input and all dates less
+ than or equal to <literal>0</literal> are treated as BC dates. Since
+ the year <literal>0 BC</literal> is not defined, and the representation
+ for BC dates is to show a positive year and the BC indicator, the
+ conversion for inputs <= 0 is effectively
+ <literal>abs(input - 1) || 'BC'</literal>.
+ </para>
+ </listitem>
+
<listitem>
<para>
In <function>to_timestamp</function>, millisecond
@@ -9087,6 +9099,13 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
</tbody>
</tgroup>
</table>
+
+ <warning>
+ <para>
+ The <literal>make_timestamp</literal> function does not accept negative years or,
+ otherwise provide a way to indicate BC, so it can only be used to construct AD dates.
+ </para>
+ </warning>
<para>
<indexterm>
diff --git a/src/test/regress/expected/date.out b/src/test/regress/expected/date.out
index 4cdf1635f2..136d1567f8 100644
--- a/src/test/regress/expected/date.out
+++ b/src/test/regress/expected/date.out
@@ -1449,3 +1449,54 @@ select make_time(10, 55, 100.1);
ERROR: time field value out of range: 10:55:100.1
select make_time(24, 0, 2.1);
ERROR: time field value out of range: 24:00:2.1
+-- Behavior around "year 0"
+SELECT to_date('0'::text ,'YYYY'); -- BUG: Should fail per our reference implementation
+ to_date
+---------------
+ 01-01-0001 BC
+(1 row)
+
+SELECT to_date('-1'::text ,'YYYY'); -- And this should result in the year -1 BC (Jan 1)
+ to_date
+---------------
+ 01-01-0002 BC
+(1 row)
+
+select make_date(1, 1, 1);
+ make_date
+------------
+ 01-01-0001
+(1 row)
+
+select make_date(1, 0, 0);
+ERROR: date field value out of range: 1-00-00
+select make_date(0, 0, 0);
+ERROR: date field value out of range: 0-00-00
+select make_date(0, 1, 1);
+ERROR: date field value out of range: 0-01-01
+select make_date(-1, 1, 1);
+ make_date
+---------------
+ 01-01-0001 BC
+(1 row)
+
+SELECT date '1-01-01 BC';
+ date
+---------------
+ 01-01-0001 BC
+(1 row)
+
+SELECT date '-1-01-01';
+ERROR: invalid input syntax for type date: "-1-01-01"
+LINE 1: SELECT date '-1-01-01';
+ ^
+SELECT date '-0001-01-01';
+ERROR: invalid input syntax for type date: "-0001-01-01"
+LINE 1: SELECT date '-0001-01-01';
+ ^
+SELECT date_part('year', '0001-01-01 BC'::date);
+ date_part
+-----------
+ -1
+(1 row)
+
diff --git a/src/test/regress/expected/timestamptz.out b/src/test/regress/expected/timestamptz.out
index 639b50308e..f6dd324e62 100644
--- a/src/test/regress/expected/timestamptz.out
+++ b/src/test/regress/expected/timestamptz.out
@@ -2501,6 +2501,41 @@ SELECT to_timestamp('-Infinity'::float);
SELECT to_timestamp('NaN'::float);
ERROR: timestamp cannot be NaN
+-- Behavior around "year 0"
+SELECT to_timestamp('0'::text, 'YYYY'); -- BUG: Should fail per reference implementation
+ to_timestamp
+---------------------------------
+ Sat Jan 01 00:00:00 0001 UTC BC
+(1 row)
+
+SELECT to_timestamp('-1'::text, 'YYYY');
+ to_timestamp
+---------------------------------
+ Fri Jan 01 00:00:00 0002 UTC BC
+(1 row)
+
+SELECT make_timestamptz(1, 1, 1, 0, 0, 0);
+ make_timestamptz
+------------------------------
+ Mon Jan 01 00:00:00 0001 UTC
+(1 row)
+
+SELECT make_timestamptz(0, 0, 0, 0, 0, 0);
+ERROR: date field value out of range: 0-00-00
+SELECT make_timestamptz(0, 1, 1, 0, 0, 0);
+ERROR: date field value out of range: 0-01-01
+SELECT make_timestamptz(-1, 1, 1, 0, 0, 0); -- BUG: make_date(-1, 1, 1) works
+ERROR: date field value out of range: -1-01-01
+SELECT timestamptz '-1-01-01 00:00:00';
+ERROR: invalid input syntax for type timestamp with time zone: "-1-01-01 00:00:00"
+LINE 1: SELECT timestamptz '-1-01-01 00:00:00';
+ ^
+SELECT timestamptz '1-01-01 00:00:00 BC';
+ timestamptz
+---------------------------------
+ Sat Jan 01 00:00:00 0001 UTC BC
+(1 row)
+
SET TimeZone to 'Europe/Moscow';
SELECT '2011-03-26 21:00:00 UTC'::timestamptz;
timestamptz
diff --git a/src/test/regress/sql/date.sql b/src/test/regress/sql/date.sql
index 1c3adf70ce..788ae11ee5 100644
--- a/src/test/regress/sql/date.sql
+++ b/src/test/regress/sql/date.sql
@@ -334,9 +334,23 @@ SELECT EXTRACT(MICROSEC FROM DATE 'infinity'); -- ERROR: timestamp units "
select make_date(2013, 7, 15);
select make_date(-44, 3, 15);
select make_time(8, 20, 0.0);
+
-- should fail
select make_date(2013, 2, 30);
select make_date(2013, 13, 1);
select make_date(2013, 11, -1);
select make_time(10, 55, 100.1);
select make_time(24, 0, 2.1);
+
+-- Behavior around "year 0"
+SELECT to_date('0'::text ,'YYYY'); -- BUG: Should fail per our reference implementation
+SELECT to_date('-1'::text ,'YYYY'); -- And this should result in the year -1 BC (Jan 1)
+select make_date(1, 1, 1);
+select make_date(1, 0, 0);
+select make_date(0, 0, 0);
+select make_date(0, 1, 1);
+select make_date(-1, 1, 1);
+SELECT date '1-01-01 BC';
+SELECT date '-1-01-01';
+SELECT date '-0001-01-01';
+SELECT date_part('year', '0001-01-01 BC'::date);
diff --git a/src/test/regress/sql/timestamptz.sql b/src/test/regress/sql/timestamptz.sql
index 300302dafd..f36dea33bf 100644
--- a/src/test/regress/sql/timestamptz.sql
+++ b/src/test/regress/sql/timestamptz.sql
@@ -428,6 +428,15 @@ SELECT to_timestamp(' Infinity'::float);
SELECT to_timestamp('-Infinity'::float);
SELECT to_timestamp('NaN'::float);
+-- Behavior around "year 0"
+SELECT to_timestamp('0'::text, 'YYYY'); -- BUG: Should fail per reference implementation
+SELECT to_timestamp('-1'::text, 'YYYY');
+SELECT make_timestamptz(1, 1, 1, 0, 0, 0);
+SELECT make_timestamptz(0, 0, 0, 0, 0, 0);
+SELECT make_timestamptz(0, 1, 1, 0, 0, 0);
+SELECT make_timestamptz(-1, 1, 1, 0, 0, 0); -- BUG: make_date(-1, 1, 1) works
+SELECT timestamptz '-1-01-01 00:00:00';
+SELECT timestamptz '1-01-01 00:00:00 BC';
SET TimeZone to 'Europe/Moscow';
On Wed, Jul 15, 2020 at 09:26:53AM -0700, David G. Johnston wrote:
On Tue, May 12, 2020 at 8:56 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
On Tue, 2020-05-12 at 18:09 -0700, David G. Johnston wrote:
Redirecting to -hackers for visibility.� I feel there needs to be
something done here, even if just documentation (a bullet in the usage
notes section - and a code comment update for the macro)pointing this out and not changing any behavior.
Since "to_date" is an Oracle compatibility function, here is what Oracle
18.4 has to say to that:SQL> SELECT to_date('0000', 'YYYY') FROM dual;
SELECT to_date('0000', 'YYYY') FROM dual
� � � � � � � �*
ERROR at line 1:
ORA-01841: (full) year must be between -4713 and +9999, and not be 0Attached is a concrete patch (back-patchable hopefully) documenting the current
reality.As noted in the patch commit message (commentary really):
make_timestamp not agreeing with make_date on how to handle negative years
should probably just be fixed - but that is for someone else to handle.Whether to actually change the behavior of to_date is up for debate though I
would presume it would not be back-patched.
OK, so, looking at this thread, we have to_date() treating -1 as -2 BC,
make_date() treating -1 as 1 BC, and we have Oracle, which to_date() is
supposed to match, making -1 as 1 BC.
Because we already have the to_date/make_date inconsistency, and the -1
to -2 BC mapping is confusing, and doesn't match Oracle, I think the
clean solution is to change PG 14 to treat -1 as 1 BC, and document the
incompatibility in the release notes.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Thu, Sep 3, 2020 at 6:21 PM Bruce Momjian <bruce@momjian.us> wrote:
On Wed, Jul 15, 2020 at 09:26:53AM -0700, David G. Johnston wrote:
Whether to actually change the behavior of to_date is up for debate
though I
would presume it would not be back-patched.
OK, so, looking at this thread, we have to_date() treating -1 as -2 BC,
make_date() treating -1 as 1 BC, and we have Oracle, which to_date() is
supposed to match, making -1 as 1 BC.Because we already have the to_date/make_date inconsistency, and the -1
to -2 BC mapping is confusing, and doesn't match Oracle, I think the
clean solution is to change PG 14 to treat -1 as 1 BC, and document the
incompatibility in the release notes.
I agree that someone else should write another patch to fix the behavior
for v14. Still suggest committing the proposed patch to master and all
supported versions to document the existing behavior correctly. The fix
patch can work from that.
David J.
On Fri, Sep 4, 2020 at 12:45:36PM -0700, David G. Johnston wrote:
On Thu, Sep 3, 2020 at 6:21 PM Bruce Momjian <bruce@momjian.us> wrote:
On Wed, Jul 15, 2020 at 09:26:53AM -0700, David G. Johnston wrote:
Whether to actually change the behavior of to_date is up for debate
though I
would presume it would not be back-patched.
OK, so, looking at this thread, we have to_date() treating -1 as -2 BC,
make_date() treating -1 as 1 BC, and we have Oracle, which to_date() is
supposed to match, making -1 as 1 BC.Because we already have the to_date/make_date inconsistency, and the -1
to -2 BC mapping is confusing, and doesn't match Oracle, I think the
clean solution is to change PG 14 to treat -1 as 1 BC, and document the
incompatibility in the release notes.I agree that someone else should write another patch to fix the behavior for
v14.� Still suggest committing the proposed patch to master and all supported
versions to document the existing behavior correctly.� The fix patch can work
from that.
I think we need to apply the patches to all branches at the same time.
I am not sure we want to document a behavior we know will change in PG
14.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On 2020-09-04 21:45, David G. Johnston wrote:
On Thu, Sep 3, 2020 at 6:21 PM Bruce Momjian <bruce@momjian.us
<mailto:bruce@momjian.us>> wrote:On Wed, Jul 15, 2020 at 09:26:53AM -0700, David G. Johnston wrote:
Whether to actually change the behavior of to_date is up for
debate though I
would presume it would not be back-patched.
OK, so, looking at this thread, we have to_date() treating -1 as -2 BC,
make_date() treating -1 as 1 BC, and we have Oracle, which to_date() is
supposed to match, making -1 as 1 BC.Because we already have the to_date/make_date inconsistency, and the -1
to -2 BC mapping is confusing, and doesn't match Oracle, I think the
clean solution is to change PG 14 to treat -1 as 1 BC, and document the
incompatibility in the release notes.I agree that someone else should write another patch to fix the behavior
for v14. Still suggest committing the proposed patch to master and all
supported versions to document the existing behavior correctly. The fix
patch can work from that.
Adding support for negative years in make_timestamp seems pretty
straightforward; see attached patch.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Support-negative-years-in-make_timestamp.patchtext/plain; charset=UTF-8; name=0001-Support-negative-years-in-make_timestamp.patch; x-mac-creator=0; x-mac-type=0Download
From ef9b0cee4aee60268d4c72e8452e74e7bbc18ed1 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 4 Sep 2020 23:02:09 +0200
Subject: [PATCH] Support negative years in make_timestamp()
make_date() already supported it; adding it was trivial.
Bug: #16419
Discussion: https://www.postgresql.org/message-id/flat/16419-d8d9db0a7553f01b@postgresql.org
---
src/backend/utils/adt/timestamp.c | 14 +++++++++-----
src/test/regress/expected/date.out | 2 ++
src/test/regress/expected/timestamp.out | 11 ++++++++++-
src/test/regress/sql/date.sql | 1 +
src/test/regress/sql/timestamp.sql | 5 ++++-
5 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 5fe304cea7..4128e3a739 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -556,17 +556,21 @@ make_timestamp_internal(int year, int month, int day,
TimeOffset date;
TimeOffset time;
int dterr;
+ bool bc = false;
Timestamp result;
tm.tm_year = year;
tm.tm_mon = month;
tm.tm_mday = day;
- /*
- * Note: we'll reject zero or negative year values. Perhaps negatives
- * should be allowed to represent BC years?
- */
- dterr = ValidateDate(DTK_DATE_M, false, false, false, &tm);
+ /* Handle negative years as BC */
+ if (tm.tm_year < 0)
+ {
+ bc = true;
+ tm.tm_year = -tm.tm_year;
+ }
+
+ dterr = ValidateDate(DTK_DATE_M, false, false, bc, &tm);
if (dterr != 0)
ereport(ERROR,
diff --git a/src/test/regress/expected/date.out b/src/test/regress/expected/date.out
index 4cdf1635f2..c03329fb5a 100644
--- a/src/test/regress/expected/date.out
+++ b/src/test/regress/expected/date.out
@@ -1439,6 +1439,8 @@ select make_time(8, 20, 0.0);
(1 row)
-- should fail
+select make_date(0, 7, 15);
+ERROR: date field value out of range: 0-07-15
select make_date(2013, 2, 30);
ERROR: date field value out of range: 2013-02-30
select make_date(2013, 13, 1);
diff --git a/src/test/regress/expected/timestamp.out b/src/test/regress/expected/timestamp.out
index 5f97505a30..9655116090 100644
--- a/src/test/regress/expected/timestamp.out
+++ b/src/test/regress/expected/timestamp.out
@@ -1704,9 +1704,18 @@ SELECT '' AS to_char_12, to_char(d, 'FF1 FF2 FF3 FF4 FF5 FF6 ff1 ff2 ff3 ff4 ff
(4 rows)
-- timestamp numeric fields constructor
-SELECT make_timestamp(2014,12,28,6,30,45.887);
+SELECT make_timestamp(2014, 12, 28, 6, 30, 45.887);
make_timestamp
------------------------------
Sun Dec 28 06:30:45.887 2014
(1 row)
+SELECT make_timestamp(-44, 3, 15, 12, 30, 15);
+ make_timestamp
+-----------------------------
+ Fri Mar 15 12:30:15 0044 BC
+(1 row)
+
+-- should fail
+select make_timestamp(0, 7, 15, 12, 30, 15);
+ERROR: date field value out of range: 0-07-15
diff --git a/src/test/regress/sql/date.sql b/src/test/regress/sql/date.sql
index 1c3adf70ce..77f94ed27b 100644
--- a/src/test/regress/sql/date.sql
+++ b/src/test/regress/sql/date.sql
@@ -335,6 +335,7 @@ CREATE TABLE DATE_TBL (f1 date);
select make_date(-44, 3, 15);
select make_time(8, 20, 0.0);
-- should fail
+select make_date(0, 7, 15);
select make_date(2013, 2, 30);
select make_date(2013, 13, 1);
select make_date(2013, 11, -1);
diff --git a/src/test/regress/sql/timestamp.sql b/src/test/regress/sql/timestamp.sql
index 7b58c3cfa5..727ee50084 100644
--- a/src/test/regress/sql/timestamp.sql
+++ b/src/test/regress/sql/timestamp.sql
@@ -240,4 +240,7 @@ CREATE TABLE TIMESTAMP_TBL (d1 timestamp(2) without time zone);
) d(d);
-- timestamp numeric fields constructor
-SELECT make_timestamp(2014,12,28,6,30,45.887);
+SELECT make_timestamp(2014, 12, 28, 6, 30, 45.887);
+SELECT make_timestamp(-44, 3, 15, 12, 30, 15);
+-- should fail
+select make_timestamp(0, 7, 15, 12, 30, 15);
--
2.28.0
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested
Patch looks good to me.
The new status of this patch is: Ready for Committer
Bruce Momjian <bruce@momjian.us> writes:
On Fri, Sep 4, 2020 at 12:45:36PM -0700, David G. Johnston wrote:
Because we already have the to_date/make_date inconsistency, and the -1
to -2 BC mapping is confusing, and doesn't match Oracle, I think the
clean solution is to change PG 14 to treat -1 as 1 BC, and document the
incompatibility in the release notes.
I agree that someone else should write another patch to fix the behavior for
v14. Still suggest committing the proposed patch to master and all supported
versions to document the existing behavior correctly. The fix patch can work
from that.
I think we need to apply the patches to all branches at the same time.
I am not sure we want to document a behavior we know will change in PG
14.
I think this is nuts. The current behavior is obviously broken;
we should just treat it as a bug and fix it, including back-patching.
I do not think there is a compatibility problem of any significance.
Who out there is going to have an application that is relying on the
ability to insert BC dates in this way?
regards, tom lane
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
Adding support for negative years in make_timestamp seems pretty
straightforward; see attached patch.
In hopes of moving things along, I pushed that, along with documentation
additions. I couldn't quite convince myself that it was a bug fix
though, so no back-patch. (I don't think we really need any doc
changes about it in the back branches, either.)
regards, tom lane
I wrote:
I think this is nuts. The current behavior is obviously broken;
we should just treat it as a bug and fix it, including back-patching.
I do not think there is a compatibility problem of any significance.
Who out there is going to have an application that is relying on the
ability to insert BC dates in this way?
Concretely, I propose the attached. This adjusts Dar Alathar-Yemen's
patch (it didn't do the right thing IMO for the combination of bc
and year < 0) and adds test cases and docs.
Oracle would have us throw an error for year zero, but our historical
behavior has been to read it as 1 BC. That's not so obviously wrong
that I'd want to change it in the back branches. Maybe it could be
done as a follow-up change in HEAD.
regards, tom lane
Attachments:
fix-to-date-for-negative-years.patchtext/x-diff; charset=us-ascii; name=fix-to-date-for-negative-years.patchDownload
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 62dd738230..ec8451d1b9 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -7678,6 +7678,15 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
</para>
</listitem>
+ <listitem>
+ <para>
+ In <function>to_timestamp</function> and <function>to_date</function>,
+ negative years are treated as signifying BC. If you write both a
+ negative year and an explicit <literal>BC</literal> field, you get AD
+ again. An input of year zero is treated as 1 BC.
+ </para>
+ </listitem>
+
<listitem>
<para>
In <function>to_timestamp</function> and <function>to_date</function>,
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index b91ff7bb80..3bb01cdb65 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -4569,8 +4569,11 @@ do_to_timestamp(text *date_txt, text *fmt, Oid collid, bool std,
{
/* If a 4-digit year is provided, we use that and ignore CC. */
tm->tm_year = tmfc.year;
- if (tmfc.bc && tm->tm_year > 0)
- tm->tm_year = -(tm->tm_year - 1);
+ if (tmfc.bc)
+ tm->tm_year = -tm->tm_year;
+ /* correct for our representation of BC years */
+ if (tm->tm_year < 0)
+ tm->tm_year++;
}
fmask |= DTK_M(YEAR);
}
diff --git a/src/test/regress/expected/horology.out b/src/test/regress/expected/horology.out
index c8c33a0fc0..7f82dcfbfe 100644
--- a/src/test/regress/expected/horology.out
+++ b/src/test/regress/expected/horology.out
@@ -2916,6 +2916,45 @@ SELECT to_date('2458872', 'J');
01-23-2020
(1 row)
+--
+-- Check handling of BC dates
+--
+SELECT to_date('44-02-01 BC','YYYY-MM-DD BC');
+ to_date
+---------------
+ 02-01-0044 BC
+(1 row)
+
+SELECT to_date('-44-02-01','YYYY-MM-DD');
+ to_date
+---------------
+ 02-01-0044 BC
+(1 row)
+
+SELECT to_date('-44-02-01 BC','YYYY-MM-DD BC');
+ to_date
+------------
+ 02-01-0044
+(1 row)
+
+SELECT to_timestamp('44-02-01 11:12:13 BC','YYYY-MM-DD HH24:MI:SS BC');
+ to_timestamp
+---------------------------------
+ Fri Feb 01 11:12:13 0044 PST BC
+(1 row)
+
+SELECT to_timestamp('-44-02-01 11:12:13','YYYY-MM-DD HH24:MI:SS');
+ to_timestamp
+---------------------------------
+ Fri Feb 01 11:12:13 0044 PST BC
+(1 row)
+
+SELECT to_timestamp('-44-02-01 11:12:13 BC','YYYY-MM-DD HH24:MI:SS BC');
+ to_timestamp
+------------------------------
+ Mon Feb 01 11:12:13 0044 PST
+(1 row)
+
--
-- Check handling of multiple spaces in format and/or input
--
@@ -3183,6 +3222,12 @@ SELECT to_date('2016 366', 'YYYY DDD'); -- ok
SELECT to_date('2016 367', 'YYYY DDD');
ERROR: date/time field value out of range: "2016 367"
+SELECT to_date('0000-02-01','YYYY-MM-DD'); -- allowed, though it shouldn't be
+ to_date
+---------------
+ 02-01-0001 BC
+(1 row)
+
--
-- Check behavior with SQL-style fixed-GMT-offset time zone (cf bug #8572)
--
diff --git a/src/test/regress/sql/horology.sql b/src/test/regress/sql/horology.sql
index c464e6766c..fed21a53c8 100644
--- a/src/test/regress/sql/horology.sql
+++ b/src/test/regress/sql/horology.sql
@@ -426,6 +426,17 @@ SELECT to_date('1 4 1902', 'Q MM YYYY'); -- Q is ignored
SELECT to_date('3 4 21 01', 'W MM CC YY');
SELECT to_date('2458872', 'J');
+--
+-- Check handling of BC dates
+--
+
+SELECT to_date('44-02-01 BC','YYYY-MM-DD BC');
+SELECT to_date('-44-02-01','YYYY-MM-DD');
+SELECT to_date('-44-02-01 BC','YYYY-MM-DD BC');
+SELECT to_timestamp('44-02-01 11:12:13 BC','YYYY-MM-DD HH24:MI:SS BC');
+SELECT to_timestamp('-44-02-01 11:12:13','YYYY-MM-DD HH24:MI:SS');
+SELECT to_timestamp('-44-02-01 11:12:13 BC','YYYY-MM-DD HH24:MI:SS BC');
+
--
-- Check handling of multiple spaces in format and/or input
--
@@ -511,6 +522,7 @@ SELECT to_date('2015 366', 'YYYY DDD');
SELECT to_date('2016 365', 'YYYY DDD'); -- ok
SELECT to_date('2016 366', 'YYYY DDD'); -- ok
SELECT to_date('2016 367', 'YYYY DDD');
+SELECT to_date('0000-02-01','YYYY-MM-DD'); -- allowed, though it shouldn't be
--
-- Check behavior with SQL-style fixed-GMT-offset time zone (cf bug #8572)
On Tue, Sep 29, 2020 at 01:26:29PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Fri, Sep 4, 2020 at 12:45:36PM -0700, David G. Johnston wrote:
Because we already have the to_date/make_date inconsistency, and the -1
to -2 BC mapping is confusing, and doesn't match Oracle, I think the
clean solution is to change PG 14 to treat -1 as 1 BC, and document the
incompatibility in the release notes.I agree that someone else should write another patch to fix the behavior for
v14.� Still suggest committing the proposed patch to master and all supported
versions to document the existing behavior correctly.� The fix patch can work
from that.I think we need to apply the patches to all branches at the same time.
I am not sure we want to document a behavior we know will change in PG
14.I think this is nuts. The current behavior is obviously broken;
we should just treat it as a bug and fix it, including back-patching.
I do not think there is a compatibility problem of any significance.
Who out there is going to have an application that is relying on the
ability to insert BC dates in this way?
You are agreeing with what I am suggesting then?
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Bruce Momjian <bruce@momjian.us> writes:
On Tue, Sep 29, 2020 at 01:26:29PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Fri, Sep 4, 2020 at 12:45:36PM -0700, David G. Johnston wrote:
Because we already have the to_date/make_date inconsistency, and the -1
to -2 BC mapping is confusing, and doesn't match Oracle, I think the
clean solution is to change PG 14 to treat -1 as 1 BC, and document the
incompatibility in the release notes.
I agree that someone else should write another patch to fix the behavior for
v14. Still suggest committing the proposed patch to master and all supported
versions to document the existing behavior correctly. The fix patch can work
from that.
I think this is nuts. The current behavior is obviously broken;
we should just treat it as a bug and fix it, including back-patching.
I do not think there is a compatibility problem of any significance.
Who out there is going to have an application that is relying on the
ability to insert BC dates in this way?
You are agreeing with what I am suggesting then?
Hm, I read your reference to "the release notes" as suggesting that
we should change it only in a major release, ie HEAD only (and it
looks like David read it the same). If you meant minor release notes,
then we're on the same page.
regards, tom lane
On Wed, Sep 30, 2020 at 02:11:54PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Tue, Sep 29, 2020 at 01:26:29PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Fri, Sep 4, 2020 at 12:45:36PM -0700, David G. Johnston wrote:
Because we already have the to_date/make_date inconsistency, and the -1
to -2 BC mapping is confusing, and doesn't match Oracle, I think the
clean solution is to change PG 14 to treat -1 as 1 BC, and document the
incompatibility in the release notes.I agree that someone else should write another patch to fix the behavior for
v14.� Still suggest committing the proposed patch to master and all supported
versions to document the existing behavior correctly.� The fix patch can work
from that.I think this is nuts. The current behavior is obviously broken;
we should just treat it as a bug and fix it, including back-patching.
I do not think there is a compatibility problem of any significance.
Who out there is going to have an application that is relying on the
ability to insert BC dates in this way?You are agreeing with what I am suggesting then?
Hm, I read your reference to "the release notes" as suggesting that
we should change it only in a major release, ie HEAD only (and it
looks like David read it the same). If you meant minor release notes,
then we're on the same page.
Yes, I was thinking just the major release notes. What are you
suggesting, and what did you ultimately decide to do? What I didn't
want to do was to document the old behavior in the old docs and change
it in PG 14.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Bruce Momjian <bruce@momjian.us> writes:
On Wed, Sep 30, 2020 at 02:11:54PM -0400, Tom Lane wrote:
Hm, I read your reference to "the release notes" as suggesting that
we should change it only in a major release, ie HEAD only (and it
looks like David read it the same). If you meant minor release notes,
then we're on the same page.
Yes, I was thinking just the major release notes. What are you
suggesting, and what did you ultimately decide to do? What I didn't
want to do was to document the old behavior in the old docs and change
it in PG 14.
Actually, I was just finishing up back-patching the patch I posted
yesterday. I think we should just fix it, not document that it's
broken.
regards, tom lane
On Wed, Sep 30, 2020 at 02:50:31PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Wed, Sep 30, 2020 at 02:11:54PM -0400, Tom Lane wrote:
Hm, I read your reference to "the release notes" as suggesting that
we should change it only in a major release, ie HEAD only (and it
looks like David read it the same). If you meant minor release notes,
then we're on the same page.Yes, I was thinking just the major release notes. What are you
suggesting, and what did you ultimately decide to do? What I didn't
want to do was to document the old behavior in the old docs and change
it in PG 14.Actually, I was just finishing up back-patching the patch I posted
yesterday. I think we should just fix it, not document that it's
broken.
Agreed, that's what I wanted. You stated in a later email you couldn't
convince yourself of the backpatch, which is why I asked.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Bruce Momjian <bruce@momjian.us> writes:
On Wed, Sep 30, 2020 at 02:50:31PM -0400, Tom Lane wrote:
Actually, I was just finishing up back-patching the patch I posted
yesterday. I think we should just fix it, not document that it's
broken.
Agreed, that's what I wanted. You stated in a later email you couldn't
convince yourself of the backpatch, which is why I asked.
Oh, I see where our wires are crossed. I meant that I couldn't
convince myself to back-patch the make_timestamp() change.
(I'm still willing to listen to an argument to do so, if anyone
wants to make one --- but that part feels more like a feature
addition than a bug fix.)
regards, tom lane
On Wed, Sep 30, 2020 at 03:11:06PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Wed, Sep 30, 2020 at 02:50:31PM -0400, Tom Lane wrote:
Actually, I was just finishing up back-patching the patch I posted
yesterday. I think we should just fix it, not document that it's
broken.Agreed, that's what I wanted. You stated in a later email you couldn't
convince yourself of the backpatch, which is why I asked.Oh, I see where our wires are crossed. I meant that I couldn't
convince myself to back-patch the make_timestamp() change.
(I'm still willing to listen to an argument to do so, if anyone
wants to make one --- but that part feels more like a feature
addition than a bug fix.)
OK, at least this is addressed fully in PG 14 and beyond.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Tue, Sep 29, 2020 at 1:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think this is nuts. The current behavior is obviously broken;
we should just treat it as a bug and fix it, including back-patching.
I do not think there is a compatibility problem of any significance.
Who out there is going to have an application that is relying on the
ability to insert BC dates in this way?
I think that's entirely the wrong way to look at it. If nobody is
using the feature, then it will not break anything to change the
behavior, but on the other hand there is no reason to fix the bug
either. But if people are using the feature, making it behave
differently in the next minor release is going to break their
applications. I disagree *strongly* with making such changes in stable
branches and feel that the change to those branches should be
reverted.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Sep 29, 2020 at 1:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think this is nuts. The current behavior is obviously broken;
we should just treat it as a bug and fix it, including back-patching.
I do not think there is a compatibility problem of any significance.
Who out there is going to have an application that is relying on the
ability to insert BC dates in this way?
I think that's entirely the wrong way to look at it. If nobody is
using the feature, then it will not break anything to change the
behavior, but on the other hand there is no reason to fix the bug
either. But if people are using the feature, making it behave
differently in the next minor release is going to break their
applications. I disagree *strongly* with making such changes in stable
branches and feel that the change to those branches should be
reverted.
By that logic, we should never fix any bug in a back branch.
regards, tom lane
On Wed, Sep 30, 2020 at 5:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
By that logic, we should never fix any bug in a back branch.
No, by that logic, we should not change any behavior in a back-branch
upon which a customer is plausibly relying. No one relies on a certain
query causing a server crash, for example, or a cache lookup failure,
so fixing those things can only help people. But there is no reason at
all why someone shouldn't be relying on this very old and
long-established behavior not to change in a minor release.
One reason they might do that is because there was a discussion about
what I believe to this exact same case 4 years ago in which you and I
both endorsed the position you are now claiming is so unreasonable
that nobody will mind if we change it in a minor release.
/messages/by-id/CAKOSWNmwCH0wx6MApc1A8ww++EQmG07AZ3t6w_XjRrV1xeZpTA@mail.gmail.com
So you now think this should be back-patched when previously you
didn't even think it was be good enough for master.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Sep 30, 2020 at 5:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
By that logic, we should never fix any bug in a back branch.
No, by that logic, we should not change any behavior in a back-branch
upon which a customer is plausibly relying.
I guess where we differ here is on the idea that somebody is plausibly
relying on to_date() to parse a BC date inaccurately.
One reason they might do that is because there was a discussion about
what I believe to this exact same case 4 years ago in which you and I
both endorsed the position you are now claiming is so unreasonable
that nobody will mind if we change it in a minor release.
/messages/by-id/CAKOSWNmwCH0wx6MApc1A8ww++EQmG07AZ3t6w_XjRrV1xeZpTA@mail.gmail.com
What I complained about in that thread was mainly that that
patch was simultaneously trying to get stricter (throw error for
year zero) and laxer (parse negative years as BC).
Also, we did not in that thread have the information that Oracle
treats negative years as BC. Now that we do, the situation is
different, and I'm willing to change my mind about it. Admittedly,
Oracle seems to require an "S" in the format to parse a leading
dash as meaning a negative year. But given that our code is willing
to read the case as a negative year without that, it seems pretty
silly to decide that it should read it as an off-by-one negative
year.
regards, tom lane
On Wed, Sep 30, 2020 at 06:10:38PM -0400, Robert Haas wrote:
On Wed, Sep 30, 2020 at 5:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
By that logic, we should never fix any bug in a back branch.
No, by that logic, we should not change any behavior in a back-branch
upon which a customer is plausibly relying. No one relies on a certain
query causing a server crash, for example, or a cache lookup failure,
so fixing those things can only help people. But there is no reason at
all why someone shouldn't be relying on this very old and
long-established behavior not to change in a minor release.
That is an interesting distinction.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Bruce Momjian <bruce@momjian.us> writes:
On Wed, Sep 30, 2020 at 06:10:38PM -0400, Robert Haas wrote:
On Wed, Sep 30, 2020 at 5:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
By that logic, we should never fix any bug in a back branch.
No, by that logic, we should not change any behavior in a back-branch
upon which a customer is plausibly relying. No one relies on a certain
query causing a server crash, for example, or a cache lookup failure,
so fixing those things can only help people. But there is no reason at
all why someone shouldn't be relying on this very old and
long-established behavior not to change in a minor release.
That is an interesting distinction.
I don't want to sound like I'm totally without sympathy for Robert's
argument. But I do say it's a judgment call, and my judgment remains
that this patch is appropriate to back-patch.
We do not have, and never have had, a project policy against
back-patching non-crash-related behavioral changes. If we did,
we would not for example put timezone database updates into the
back branches. It's not terribly hard to imagine such updates
breaking applications that expected the meaning of, say,
'2022-04-01 12:34 Europe/Paris' to hold still. But we do it
anyway.
As another not-too-old example, I'll cite Robert's own commits
0278d3f79/a08bfe742. The argument for a back-patch there was
pretty much only that we were writing an alleged tar file that
didn't conform to the letter of the POSIX spec. It's possible
to imagine that somebody had written bespoke archive-reading
code that failed after we changed the output; but that didn't
seem probable enough to justify continuing to violate the standard.
In this case the "standard" in question is the Oracle-derived
expectation that to_date will read negative years as BC, and
I'd argue that the possibility that someone already has code
that relies on getting an off-by-one result is outweighed by the
likelihood that the misbehavior will hurt somebody in the future.
This calculus would obviously change if we knew of such code
or thought it was really probable for it to exist. That's
what makes it a judgment call.
regards, tom lane
On Wed, Sep 30, 2020 at 07:26:55PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Wed, Sep 30, 2020 at 06:10:38PM -0400, Robert Haas wrote:
On Wed, Sep 30, 2020 at 5:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
By that logic, we should never fix any bug in a back branch.
No, by that logic, we should not change any behavior in a back-branch
upon which a customer is plausibly relying. No one relies on a certain
query causing a server crash, for example, or a cache lookup failure,
so fixing those things can only help people. But there is no reason at
all why someone shouldn't be relying on this very old and
long-established behavior not to change in a minor release.That is an interesting distinction.
I don't want to sound like I'm totally without sympathy for Robert's
argument. But I do say it's a judgment call, and my judgment remains
that this patch is appropriate to back-patch.
Agreed. I was just thinking it was an interesting classification that
no one relies on crashes, or query failures.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Wed, Sep 30, 2020 at 7:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
We do not have, and never have had, a project policy against
back-patching non-crash-related behavioral changes. If we did,
we would not for example put timezone database updates into the
back branches. It's not terribly hard to imagine such updates
breaking applications that expected the meaning of, say,
'2022-04-01 12:34 Europe/Paris' to hold still. But we do it
anyway.As another not-too-old example, I'll cite Robert's own commits
0278d3f79/a08bfe742. The argument for a back-patch there was
pretty much only that we were writing an alleged tar file that
didn't conform to the letter of the POSIX spec. It's possible
to imagine that somebody had written bespoke archive-reading
code that failed after we changed the output; but that didn't
seem probable enough to justify continuing to violate the standard.
Right. Ultimately, this comes down to a judgement call about what you
think people are likely to rely on, and what you think they are
unlikely to rely on. If I recall correctly, I thought that case was a
close call, and back-patched because you argued for it. Either way, it
does seem very unlikely that someone would write archive-reading code
that relies on the presence of an extra 511 zero bytes, because (1) it
would be a lot easier to just use 'tar', (2) such code would fail if
used with a tar archive generated by anything other than PostgreSQL,
and (3) such code would fail on a tar archive generated by PostgreSQL
but without using -R. It is just barely plausible that someone has a
version of 'tar' that fails on the bogus archive and will work with
that fix, though I would guess that's also pretty unlikely.
But the present case does not seem to me to be comparable. If someone
is using to_date() to construct date values, I can't see why they
wouldn't test it, find out how it works with BC values, and then make
the application that generates the input to that function do the right
thing for the actual behavior of the function. There are discussions
of the behavior of to_date with YYYY = 0 going back at least 10 years
on this mailing list, and more recent discussions of the behavior of
negative numbers. Point being: I knew about the behavior that was here
reported as a bug and have known about it for years, and if I were
still an application developer I can easily imagine having coded to
it. I don't know why someone else should not have done the same. The
fact that we've suddenly discovered that this is not what Oracle does
doesn't mean that no users have discovered that it is what PostgreSQL
does.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Sep 30, 2020 at 5:24 PM Robert Haas <robertmhaas@gmail.com> wrote:
The
fact that we've suddenly discovered that this is not what Oracle does
doesn't mean that no users have discovered that it is what PostgreSQL
does.
Presently I cannot seem to make up my mind so I'm going to go with my
original opinion which was to only change the behavior in v14. In part
because it seems appropriate given our generally laissez-faire attitude
toward this particular feature.
David J.
Robert Haas <robertmhaas@gmail.com> writes:
Right. Ultimately, this comes down to a judgement call about what you
think people are likely to rely on, and what you think they are
unlikely to rely on.
Good, so at least we agree on that principle.
But the present case does not seem to me to be comparable. If someone
is using to_date() to construct date values, I can't see why they
wouldn't test it, find out how it works with BC values, and then make
the application that generates the input to that function do the right
thing for the actual behavior of the function. There are discussions
of the behavior of to_date with YYYY = 0 going back at least 10 years
on this mailing list, and more recent discussions of the behavior of
negative numbers.
Sure, we have at least two bug reports proving that people have
investigated this. What I'm saying is unlikely is that there are any
production applications in which it matters. I doubt that, say, the
Italian government has a citizenry database in which they've recorded
Julius Caesar's birthday; and even if they do, they're probably not
squirting the data through to_date; and even if they are, they're more
likely using the positive-year-with-BC representation, because that's
the only one that PG will emit. Even if they've got code that somehow
relies on to_date working this way, it's almost certainly getting zero
use in practice.
I probably wouldn't have taken an interest in this at all, were it
not for the proposal that we document the misbehavior. Doing that
rather than fixing it just seems silly.
regards, tom lane