Обсуждение: patch: make_timestamp function

On Thu, Dec 12, 2013 at 08:50:26PM -0200, Fabrízio de Royes Mello wrote:
> On Thu, Dec 12, 2013 at 3:11 PM, Pavel Stehule <pavel.stehule@gmail.com>wrote:
>
> > Hello
> >
> > this patch try to complete a set of functions make_date and make_timestamp.
> >
> >
> Could we have the 'make_timestamptz' function too?

Wouldn't this just be:

SELECT make_timestamp(...) at time zone 'foo';

(assuming make_timestamp actually returns a timestamp and not a
timestamptz).

or do you mean something else?

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> He who writes carelessly confesses thereby at the very outset that he does
> not attach much importance to his own thoughts.  -- Arthur Schopenhauer

<div dir="ltr"><div class="gmail_extra"><br />On Fri, Dec 13, 2013 at 3:53 PM, Martijn van Oosterhout <<a
href="mailto:kleptog@svana.org">kleptog@svana.org</a>>wrote:<br />><br />> On Thu, Dec 12, 2013 at 08:50:26PM
-0200,Fabrízio de Royes Mello wrote:<br /> > > On Thu, Dec 12, 2013 at 3:11 PM, Pavel Stehule <<a
href="mailto:pavel.stehule@gmail.com">pavel.stehule@gmail.com</a>>wrote:<br/>> ><br />> > > Hello<br
/>>> ><br />> > > this patch try to complete a set of functions make_date and make_timestamp.<br />
>> ><br />> > ><br />> > Could we have the 'make_timestamptz' function too?<br />><br />>
Wouldn'tthis just be:<br />><br />> SELECT make_timestamp(...) at time zone 'foo';<br /> ><br />> (assuming
make_timestampactually returns a timestamp and not a<br />> timestamptz).<br />><br />> or do you mean
somethingelse?<br />><br /><br /></div><div class="gmail_extra">Your example will convert the timestamp into time
zonedefined by 'at time zone...'.<br /><br /></div><div class="gmail_extra">I think the goal of the
"make_date/time/timestamp"function series is build a date/time/timestamp from scratch, so the use of 'make_timestamptz'
isto build a specific timestamp with timezone and don't convert it.<br /></div><div class="gmail_extra"><br
/></div><divclass="gmail_extra">Regards,<br /><br /></div><div class="gmail_extra">--<br />Fabrízio de Royes Mello<br
/>Consultoria/CoachingPostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/> >> Blog sobre TI: <a
href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/>>> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/> >> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a></div></div>

Fabrízio de Royes Mello <fabriziomello@gmail.com> writes:
> I think the goal of the "make_date/time/timestamp" function series is build
> a date/time/timestamp from scratch, so the use of 'make_timestamptz' is to
> build a specific timestamp with timezone and don't convert it.

Yeah; we don't really want to incur an extra timezone rotation just to get
to a timestamptz.  However, it's not clear to me if make_timestamptz()
needs to have an explicit zone parameter or not.  It could just assume
that you meant the active timezone.
        regards, tom lane

On 12/13/2013 02:35 PM, Tom Lane wrote:
> Fabrízio de Royes Mello <fabriziomello@gmail.com> writes:
>> I think the goal of the "make_date/time/timestamp" function series is build
>> a date/time/timestamp from scratch, so the use of 'make_timestamptz' is to
>> build a specific timestamp with timezone and don't convert it.
> Yeah; we don't really want to incur an extra timezone rotation just to get
> to a timestamptz.  However, it's not clear to me if make_timestamptz()
> needs to have an explicit zone parameter or not.  It could just assume
> that you meant the active timezone.
>
>             


Why not overload the function, with one version having the explicit TZ 
param?

cheers

andrew

On 12/13/13 1:49 PM, Fabrízio de Royes Mello wrote:
>
> On Fri, Dec 13, 2013 at 5:35 PM, Tom Lane <tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>> wrote:
>  >
>  > Fabrízio de Royes Mello <fabriziomello@gmail.com <mailto:fabriziomello@gmail.com>> writes:
>  > > I think the goal of the "make_date/time/timestamp" function series is build
>  > > a date/time/timestamp from scratch, so the use of 'make_timestamptz' is to
>  > > build a specific timestamp with timezone and don't convert it.
>  >
>  > Yeah; we don't really want to incur an extra timezone rotation just to get
>  > to a timestamptz.  However, it's not clear to me if make_timestamptz()
>  > needs to have an explicit zone parameter or not.  It could just assume
>  > that you meant the active timezone.
>  >
>
> +1. And if you want a different timezone you can just set the 'timezone' GUC.

Why wouldn't we have a version that optionally accepts the timezone? That mirrors what you can currently do with a cast
fromtext, and having to set the GUC if you need a different TZ would be a real PITA.
 
-- 
Jim C. Nasby, Data Architect                       jim@nasby.net
512.569.9461 (cell)                         http://jim.nasby.net

On 12/15/13, 12:59 PM, Pavel Stehule wrote:
>     Why wouldn't we have a version that optionally accepts the timezone? That mirrors what you can currently do with
acast from text, and having to set the GUC if you need a different TZ would be a real PITA.
 
>
>
> It is not bad idea.
>
> What will be format for timezone in this case? Is a doble enough?

Sorry for not seeing this earlier, but no, I think double is barking up the wrong tree. It should accept the same
timezoneidentifiers that the rest of the system does, like blah AT TIME ZONE foo and SET timezone = foo;
 

Specifically, it needs to support things like 'GMT' and 'CST6CDT'.

I can see an argument for another version that accepts numeric so if you want to do -11.5 you don't have to wrap it in
quotes...
-- 
Jim C. Nasby, Data Architect                       jim@nasby.net
512.569.9461 (cell)                         http://jim.nasby.net

Pavel Stehule escribió:

> but AT TIME ZONE allows double (but decimal parts is ignored quietly)
> 
> postgres=# select make_time(10,20,30) at time zone '+10.2';
>   timezone
> -------------
>  23:20:30-10
> 
> so I propose (and I implemented) a variant with int as time zone
> 
> and we can (if we would) implement next one with text as time zone

Yeah, I think a constructor should allow a text timezone.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Yeah, I think a constructor should allow a text timezone.

Yes.  I think a numeric timezone parameter is about 99% useless,
and if you do happen to need that behavior you can just cast the
numeric to text no?
        regards, tom lane

On Tue, Dec 17, 2013 at 06:07:38PM +0100, Pavel Stehule wrote:
> Hello
> 
> 
> 2013/12/17 Pavel Stehule <pavel.stehule@gmail.com>
> 
> 
> 
> 
>     2013/12/17 Tom Lane <tgl@sss.pgh.pa.us>
> 
>         Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>         > Yeah, I think a constructor should allow a text timezone.
> 
>         Yes.  I think a numeric timezone parameter is about 99% useless,
>         and if you do happen to need that behavior you can just cast the
>         numeric to text no?
> 
> 
>     yes, it is possible. Although fully numeric API is much more consistent.
> 
> 
> 
> I was wrong - there are timezones with minutes like Iran = '1:30';
> 
> so int in hours is bad type - so only text is probably best

I think India is the big non-integer timezone offset country:
http://www.timeanddate.com/worldclock/city.html?n=176UTC/GMT +5:30 hours

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

Hi,

I've done a quick review of this patch:

1) patch applies fine to the current HEAD, with a few hunks offset  by a few lines

2) the compilation fails because of duplicate OIDs in pg_proc, so  I had to change 3969-3975 to 4033-4039, then it
compilesfine
 

3) make installcheck works fine

4) No regression tests for make_time / make_date.

5) The documentation is incomplete - make_date / make_time are missing.

6) The documentation should mention that when the 'timezone' parameter  is not set explicitly, the current timezone is
used.

7) Why do the functions accept only the timezone abbreviation, not the  full name? I find it rather confusing, because
the'timezone' option  uses the full name, and we're using this as the default. But doing  'show timestamp' and using
thereturned value fails. Is it possible  to fix this somehow?
 



regards
Tomas

Looks good to me.


Regards,
Marko Tiikkaja

Pavel Stehule escribió:

> > 7) Why do the functions accept only the timezone abbreviation, not the
> >    full name? I find it rather confusing, because the 'timezone' option
> >    uses the full name, and we're using this as the default. But doing
> >    'show timestamp' and using the returned value fails. Is it possible
> >    to fix this somehow?
>
> A only abbreviation is allowed for timetz type. Timestamp can work with
> full time zone names. A rules (behave) should be same as input functions
> for types: timestamptz and timetz.
>
> postgres=# select '10:10:10 CET'::timetz;
>    timetz
> ─────────────
>  10:10:10+01
> (1 row)
>
> postgres=# select '10:10:10 Europe/Prague'::timetz;
> ERROR:  invalid input syntax for type time with time zone: "10:10:10
> Europe/Prague"
> LINE 1: select '10:10:10 Europe/Prague'::timetz;
>                ^
>
> This limit is due used routines limits.

I think this is a strange limitation, and perhaps it should be fixed
rather than inflicting the limitation on the new function.

I tweaked your patch a bit, attached; other than defining what to do
about full TZ names in timetz, this seems ready to commit.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Pavel Stehule escribió:

> I though about it, and now I am thinking so timezone in format
> 'Europe/Prague' is together with time ambiguous
> 
> We can do it, but we have to expect so calculation will be related to
> current date - and I am not sure if it is correct, because someone can
> write some like
> 
> make_date(x,x,x) + make_timetz(..) - and result will be damaged.

Hmm, I see your point --- the make_timetz() call would use today's
timezone displacement, which might be different from the one used in the
make_date() result.  That would result in a botched timestamptz
sometimes, but it might escape testing because it's subtle and depends
on the input data.

However, your proposal is to use an abbreviation timezone, thereby
forcing the user to select the correct timezone i.e. the one that
matches the make_date() arguments.  I'm not sure this is much of an
improvement, because then the user is faced with the difficult problem
of figuring out the correct abbreviation in the first place.

I think there is little we can do to solve the problem at this level; it
seems to me that the right solution here is to instruct users to use
make_date() only in conjunction with make_time(), that is, produce a
timezone-less timestamp; and then apply a AT TIME ZONE operator to the
result.  That could take a full timezone name, and that would always
work correctly.

My conclusion here is that the "time with time zone" datatype is broken
in itself, because of this kind of ambiguity.  Maybe we should just
avoid offering more functionality on top of it, that is get rid of
make_timetz() in this patch?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

<p dir="ltr"><br /> Dne 19. 2. 2014 21:20 "Alvaro Herrera" <<a
href="mailto:alvherre@2ndquadrant.com">alvherre@2ndquadrant.com</a>>napsal(a):<br /> ><br /> > Pavel Stehule
escribió:<br/> ><br /> > > I though about it, and now I am thinking so timezone in format<br /> > >
'Europe/Prague'is together with time ambiguous<br /> > ><br /> > > We can do it, but we have to expect so
calculationwill be related to<br /> > > current date - and I am not sure if it is correct, because someone can<br
/>> > write some like<br /> > ><br /> > > make_date(x,x,x) + make_timetz(..) - and result will be
damaged.<br/> ><br /> > Hmm, I see your point --- the make_timetz() call would use today's<br /> > timezone
displacement,which might be different from the one used in the<br /> > make_date() result.  That would result in a
botchedtimestamptz<br /> > sometimes, but it might escape testing because it's subtle and depends<br /> > on the
inputdata.<br /> ><br /> > However, your proposal is to use an abbreviation timezone, thereby<br /> > forcing
theuser to select the correct timezone i.e. the one that<br /> > matches the make_date() arguments.  I'm not sure
thisis much of an<br /> > improvement, because then the user is faced with the difficult problem<br /> > of
figuringout the correct abbreviation in the first place.<br /> ><br /> > I think there is little we can do to
solvethe problem at this level; it<br /> > seems to me that the right solution here is to instruct users to use<br
/>> make_date() only in conjunction with make_time(), that is, produce a<br /> > timezone-less timestamp; and
thenapply a AT TIME ZONE operator to the<br /> > result.  That could take a full timezone name, and that would
always<br/> > work correctly.<br /> ><br /> > My conclusion here is that the "time with time zone" datatype is
broken<br/> > in itself, because of this kind of ambiguity.  Maybe we should just<br /> > avoid offering more
functionalityon top of it, that is get rid of<br /> > make_timetz() in this patch?<br /> ><p dir="ltr">+1<p
dir="ltr">Pavel<br/> > --<br /> > Álvaro Herrera                <a
href="http://www.2ndQuadrant.com/">http://www.2ndQuadrant.com/</a><br/> > PostgreSQL Development, 24x7 Support,
Training& Services<br /> 

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> My conclusion here is that the "time with time zone" datatype is broken
> in itself, because of this kind of ambiguity.

That's the conclusion that's been arrived at by pretty much everybody
who's looked at it with any care.

> Maybe we should just
> avoid offering more functionality on top of it, that is get rid of
> make_timetz() in this patch?

+1.  We don't need to encourage people to use that type.
        regards, tom lane

Pavel Stehule escribió:
> Hello
>
> updated patch without timetz support

Great, thanks.

While testing, I noticed something strange regarding numeric timezone
specification.  Basically the way any particular value is handled is
underspecified, or maybe just completely wacko.  Consider the attached
function, which will try to construct a timestamptz value with all
possible values for timezone in the -1000 to 1000 range, ignoring those
that cause errors for whatever reason, and then subtract the obtained
timestamptz from the base value.  The output is also attached.

First of all you can see that there are plenty of values for which the
constructor will simply fail.

Second, the way signs are considered or not seems arbitrary.  Note that
if you say either '-2' or '2', you will end up with the same timestamptz
value.  But at -16 the value jumps to the opposite sign.

For negative values, this continues up to -99; but at -100, apparently
it stops considering the value a number of hours, and it considers
hours-and-minutes with a missing colon separator.  Which works up to
-159; at -160 and up to -167 it uses a different interpretation again
(not sure what).  Then values -168 and below are not valid; -200 is
valid again (2 hours)  For the rest of the interval,

For positive values, apparently there's no funny interpretation; the
number is taken to be a number of hours up to 167.  There's no valid
value above that.  However, if you prepend a plus sign, the result is
completely different and there are valid values up to +1559.  The funny
behavior in +160 through +167 is there too.

Not sure what to make of this; certainly it's not my interest to fix it.
However I wonder if we should really offer the capability to pass
numeric timezone values.  Seems it'd be saner to allow just symbolic
names, either abbreviations or full names.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Pavel Stehule escribió:

> so still I prefer to allow numeric time zones.
> 
> What I can:
> 
> a) disallow numeric only timezone without prefix "+" or "-"
> 
> or
> 
> b) add "+" prefix to time zone, when number is possitive.
> 
> I prefer @a.

I can live with (a) too.  But I wonder if we should restrict the allowed
tz even further, for example to say that there must always be either 2
digits (no colon) or 4 digits, with or without a colon.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Pavel Stehule escribió:
> Hello
> 
> updated version - a precheck is very simple, and I what I tested it is
> enough

Okay, thanks.  I pushed it after some more editorialization.  I don't
think I broke anything, but please have a look.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Pavel Stehule escribió:
> 2014-03-04 19:12 GMT+01:00 Alvaro Herrera <alvherre@2ndquadrant.com>:
> 
> > Pavel Stehule escribió:
> > > Hello
> > >
> > > updated version - a precheck is very simple, and I what I tested it is
> > > enough
> >
> > Okay, thanks.  I pushed it after some more editorialization.  I don't
> > think I broke anything, but please have a look.
> 
> It looks well

Coypu is showing a strange failure though:

http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=coypu&dt=2014-03-04%2018%3A22%3A31 select make_interval(secs :=
'inf');
!     make_interval    
! ---------------------
!  @ 0.000001 secs ago
! (1 row)

I realize that we have some hacks in float4in and float8in to deal with
these portability issues ...  Maybe the fix is just take out the test.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Pavel Stehule escribió:
> Hi
> 
> I hope, so this patch fix it

wtf?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Mar 6, 2014 at 1:26 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2014-03-05 16:22 GMT+01:00 Alvaro Herrera <alvherre@2ndquadrant.com>:
>
>> Pavel Stehule escribió:
>> > Hi
>> >
>> > I hope, so this patch fix it
>>
>> wtf?
>
>
> I tried to fix
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=f1ba94bcd9717b94b36868d6905547e313f3a359
>
> Tom did it better than me.

The patch you attached was one from Heikki, not anything you wrote for
yourself, and utterly unrelated to the topic of this thread.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company