Thread: Bug in tm2timestamp
AFAICT, there's a bug in tm2timestamp(). You can't do this: postgres=# select '1999-12-31T24:00:00'::timestamptz; ERROR: timestamp out of range: "1999-12-31T24:00:00" But that's a perfectly legal date. It works fine for any other year - and AFAICT this is because of the POSTGRES_EPOCH_JDATE being 2000-01-01. The check in 1693 and forward comes with *result=0 and date=-1 in this case, which AFAICT is fine. I'm not entirely sure what that check is guarding against (which may be because I just came off a flight from canada and don't really have the brain in full gear ATM). Perhaps it just needs an extra exclusion for this special case? --Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> AFAICT, there's a bug in tm2timestamp(). You can't do this:
> postgres=# select '1999-12-31T24:00:00'::timestamptz;
> ERROR: timestamp out of range: "1999-12-31T24:00:00"
> But that's a perfectly legal date. It works fine for any other year -
> and AFAICT this is because of the POSTGRES_EPOCH_JDATE being
> 2000-01-01.
> The check in 1693 and forward comes with *result=0 and date=-1 in this
> case, which AFAICT is fine.
Meh. Looks like I fixed this back in 2003, but didn't fix it right.
Will try again.
> I'm not entirely sure what that check is guarding against (which may
> be because I just came off a flight from canada and don't really have
> the brain in full gear ATM). Perhaps it just needs an extra exclusion
> for this special case?
I think we should just tweak the tests so that if "date" is 0 or -1,
we don't consider it an overflow even if the time adjustment pushes
the result to the other sign.
BTW, it strikes me that it's a bit silly to go to all this effort
here, and then ignore the possibility of overflow in the dt2local
adjustment just below. But we'd have to change the API of that
function, which I don't especially feel like doing right now.
regards, tom lane
Tom Lane wrote: > BTW, it strikes me that it's a bit silly to go to all this effort > here, and then ignore the possibility of overflow in the dt2local > adjustment just below. But we'd have to change the API of that > function, which I don't especially feel like doing right now. Another point worth considering is that most of this is duplicated by ecpg's libpgtypes. Do we want to fix that one too, or do we just let it continue to be broken? I note that other bugs are already unfixed in ecpg's copy. One other idea to consider is moving these things to src/common, so we would have a single implementation. I already have a patch that implements most of that, but it's only 90% there because it's missing support for some things that the current code manipulates as global variables (via GUC), and I didn't want to waste more time fixing that. AFAICS it's just a SMOP, though, but I had postponed that whole effort to the 9.4 cycle to avoid stalling 9.3 even longer. But in light of this bug and other already fixed date/time bugs, perhaps it's warranted? Opinions please. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Alvaro Herrera wrote: > Tom Lane wrote: > > > BTW, it strikes me that it's a bit silly to go to all this effort > > here, and then ignore the possibility of overflow in the dt2local > > adjustment just below. But we'd have to change the API of that > > function, which I don't especially feel like doing right now. > > Another point worth considering is that most of this is duplicated by > ecpg's libpgtypes. Bah, ignore this. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Alvaro Herrera wrote:
>> Another point worth considering is that most of this is duplicated by
>> ecpg's libpgtypes.
> Bah, ignore this.
Huh? I think you're quite right that it'd be a good idea to get rid of
the duplicated code, if we could. It's always been the backend's free
reliance on palloc and elog/ereport that's been stopping that. I think
we might now have a realistic solution for palloc, but what about elog?
regards, tom lane
On Mon, Mar 4, 2013 at 8:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> AFAICT, there's a bug in tm2timestamp(). You can't do this: >> postgres=# select '1999-12-31T24:00:00'::timestamptz; >> ERROR: timestamp out of range: "1999-12-31T24:00:00" > >> But that's a perfectly legal date. It works fine for any other year - >> and AFAICT this is because of the POSTGRES_EPOCH_JDATE being >> 2000-01-01. > >> The check in 1693 and forward comes with *result=0 and date=-1 in this >> case, which AFAICT is fine. > > Meh. Looks like I fixed this back in 2003, but didn't fix it right. > Will try again. > >> I'm not entirely sure what that check is guarding against (which may >> be because I just came off a flight from canada and don't really have >> the brain in full gear ATM). Perhaps it just needs an extra exclusion >> for this special case? > > I think we should just tweak the tests so that if "date" is 0 or -1, > we don't consider it an overflow even if the time adjustment pushes > the result to the other sign. That's pretty much what I thought - thanks for confirming, and for putting the fix in! > BTW, it strikes me that it's a bit silly to go to all this effort > here, and then ignore the possibility of overflow in the dt2local > adjustment just below. But we'd have to change the API of that > function, which I don't especially feel like doing right now. Yeah, and it wouldn't be a good backpatchable thing anyway... -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > Alvaro Herrera wrote: > >> Another point worth considering is that most of this is duplicated by > >> ecpg's libpgtypes. > > > Bah, ignore this. > > Huh? I think you're quite right that it'd be a good idea to get rid of > the duplicated code, if we could. It's always been the backend's free > reliance on palloc and elog/ereport that's been stopping that. I think > we might now have a realistic solution for palloc, but what about elog? Well, for instance ecpg's EncodeSpecialTimestamp uses a true/false return value instead of raising an error (though its only caller does not check it). There were few uses of elog/ereport that were really problematic, though I admit that even a single one could mean going through a lot of hoops. Simply crafting an implementation of elog/ereport for frontend and ecpg is probably not going to work, though, because ecpg normally returns error codes for the caller to figure out. Maybe we could create a layer on top of ereport, that gets both the error message, sqlstate etc, and also a return code for ECPG. Or a replacement, so instead of ereport() we have, say, cmn_ereport() that expands to errstart/errfinish on backend and something else on ecpg. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Mar 04, 2013 at 05:08:26PM -0300, Alvaro Herrera wrote: > Another point worth considering is that most of this is duplicated by > ecpg's libpgtypes. Do we want to fix that one too, or do we just let it > continue to be broken? I note that other bugs are already unfixed in > ecpg's copy. One other idea to consider is moving these things to Meaning that a fix wasn't put there, too? > But in light of this bug and other already fixed date/time bugs, perhaps > it's warranted? Opinions please. I'd love to go to a single source. Most of libpgtypes was taken from the backend back when it was developed. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at gmail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
On Mon, Mar 04, 2013 at 05:55:26PM -0300, Alvaro Herrera wrote: > error codes for the caller to figure out. Maybe we could create a layer > on top of ereport, that gets both the error message, sqlstate etc, and > ... Couldn't we just create ecpg's own version of ereport, that does "the right thing" for ecpg? Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at gmail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
Michael Meskes wrote: > On Mon, Mar 04, 2013 at 05:08:26PM -0300, Alvaro Herrera wrote: > > Another point worth considering is that most of this is duplicated by > > ecpg's libpgtypes. Do we want to fix that one too, or do we just let it > > continue to be broken? I note that other bugs are already unfixed in > > ecpg's copy. One other idea to consider is moving these things to > > Meaning that a fix wasn't put there, too? Yes, a fix was put there by Tom (which is why I retracted my comment initially). I did note that the ecpg code has diverged from the backend code; it's not unlikely that other bug fixes have not gone to the ecpg copy. But I didn't investigate each difference in detail. > > But in light of this bug and other already fixed date/time bugs, perhaps > > it's warranted? Opinions please. > > I'd love to go to a single source. Most of libpgtypes was taken from the > backend back when it was developed. I will keep that in mind, if I get back to moving the timestamp/datetime code to src/common. It's not a high priority item right now. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services