Thread: timestamp datatype cleanup

timestamp datatype cleanup

From
Warren Turkal
Date:
PosgreSQL hackers,

Here's an initial bit of my attempt at cleaning up the the timestamp datatype.
I have gone through the backend and made a couple small changes to stop using
the HAVE_INT64_TIMESTAMP define to select a type in code by creating typedefs
in a header and using the typedef in the code. I think this small bit is ready
for inclusion for this small bit, but I have a couple questions for further
work.

1) Is there a reason that header information is duplicated between normal
posgresql include and ecpg includes instead of defining the info in one place
and #including it into the files that need it?

2) Would it be reasonable to change timestamp.h into a file that includes other
files that define the specific parts depending on HAVE_INT64_TIMESTAMP instead
of testing for HAVE_INT64_TIMESTAMP many times throughout timestamp.h? I think
this might more cleanly separate the logic for the different timestamp types.

Thanks,
wt


Re: timestamp datatype cleanup

From
Michael Meskes
Date:
On Sun, Mar 09, 2008 at 12:32:20AM -0800, Warren Turkal wrote:
> 1) Is there a reason that header information is duplicated between normal
> posgresql include and ecpg includes instead of defining the info in one place
> and #including it into the files that need it?

As long as both implementations are kept in sync I don't see a reason.

Michael
-- 
Michael Meskes
Email: Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!


Re: timestamp datatype cleanup

From
Andrew Chernow
Date:
Michael Meskes wrote:
> On Sun, Mar 09, 2008 at 12:32:20AM -0800, Warren Turkal wrote:
>> 1) Is there a reason that header information is duplicated between normal
>> posgresql include and ecpg includes instead of defining the info in one place
>> and #including it into the files that need it?

Merlin and I ran into this question while working on the libpq type system, 
found here:

http://archives.postgresql.org/pgsql-patches/2008-03/msg00057.php

It opens up the binary parameterized interface in libpq by providing data type 
converters, to and from external binary format and C data types, when performing 
queries or getting results.  This required copying and pasting some code from 
the backend for a couple types ... like datetime and numeric.  We had to work 
around several backend compile-time checks by using run-time checks; being how 
servers are compiled differently.

The most modular approach would be to abstract the backend/utils/adt API so the 
backend and client stuff can share the functionality.  We did mention this 
during one of our patch submissions, but didn't push it as it is a large change 
outside the scope of our patch.
>> As long as both implementations are kept in sync I don't see a reason.

Sharing the backend data type converters with client stuff is an obvious win, 
but its a tedious process probably lacking any motivation.  We did look at it 
though, it is possible.

-- 
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/


Re: timestamp datatype cleanup

From
Michael Meskes
Date:
> >> As long as both implementations are kept in sync I don't see a reason.
>
> Sharing the backend data type converters with client stuff is an obvious 
> win, but its a tedious process probably lacking any motivation.  We did 
> look at it though, it is possible.

I thought we were just talking about some definitions. If we were to
move the whole logic into one place we might be facing a
performance penalty inside the backend, maybe not a large one though.
Others around here are better suited to judge this.

Michael
-- 
Michael Meskes
Email: Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!


Re: timestamp datatype cleanup

From
Andrew Chernow
Date:
 >>I thought we were just talking about some definitions.

I was expanding on your "#1" question, which was directly talking about "shared" 
headers rather than just cleaning out HAVE_INT64_TIMESTAMP.  I had the same 
experience but also ran into the need for "shared" library code; which BTW ecpg 
could benefit from as well.
>> performance penalty inside the backend

The idea requires reorganizing, not reimplementing.  There shouldn't be a change 
in performance in either direction.

-- 
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/


Re: timestamp datatype cleanup

From
Tom Lane
Date:
Michael Meskes <meskes@postgresql.org> writes:
>>> As long as both implementations are kept in sync I don't see a reason.
>> 
>> Sharing the backend data type converters with client stuff is an obvious 
>> win, but its a tedious process probably lacking any motivation.  We did 
>> look at it though, it is possible.

> I thought we were just talking about some definitions. If we were to
> move the whole logic into one place we might be facing a
> performance penalty inside the backend, maybe not a large one though.
> Others around here are better suited to judge this.

It's already a huge maintenance problem that there are two copies of the
datetime code (and no, they are *not* being kept in sync; most patches
to the core code have not gone into ecpg).  The prospect of extending
that mess to even more datatypes fills me with dread.

I don't know what we could do about it though.  We can't really
replicate the conveniences of the backend coding environment in a
frontend application, and yet I surely don't want to tighten up the
expectations on backend datatypes to try to make them work in a
frontend-ish environment.  The chances of that happening reliably
are about nil.

My feeling is that we should resist the temptation to provide any such
functionality on the frontend side (and yes, that inclues ripping out
ecpg's datetime stuff).  It'll be a black hole sucking infinite numbers
of maintenance man-hours, with not nearly enough benefit in return.
        regards, tom lane


Re: timestamp datatype cleanup

From
Andrew Chernow
Date:
 >>conveniences of the backend coding environment in a frontend application

I don't think this is required.  I'm thinking about light-weight wrappers around 
A-2-B style converters, no ties to the backend or client.  A free standing util 
library.  The idea doesn't require a backend environment.
>>want to tighten up the expectations on backend datatypes to try>>to make them work in a frontend-ish environment.

Making timestamp2tm or tm2timestamp (for example) work in the backend as well as 
the client seems rather straight forward, unless I am missing something.  I am 
not talking about making the data type functional: like add, divide, multiply 
funtions.  I am only referring to data type converters.
>>The chances of that happening reliably are about nil.

Why?  Again, I could be missing something.

-- 
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/


Re: timestamp datatype cleanup

From
"Warren Turkal"
Date:
Give the discussion on this. Is this small patch being considered for
inclusion? If not, what do I need to change to make it acceptable?

Thanks,
wt

On Sun, Mar 9, 2008 at 1:32 AM, Warren Turkal <turkal@google.com> wrote:
> PosgreSQL hackers,
>
>  Here's an initial bit of my attempt at cleaning up the the timestamp datatype.
>  I have gone through the backend and made a couple small changes to stop using
>  the HAVE_INT64_TIMESTAMP define to select a type in code by creating typedefs
>  in a header and using the typedef in the code. I think this small bit is ready
>  for inclusion for this small bit, but I have a couple questions for further
>  work.
>
>  1) Is there a reason that header information is duplicated between normal
>  posgresql include and ecpg includes instead of defining the info in one place
>  and #including it into the files that need it?
>
>  2) Would it be reasonable to change timestamp.h into a file that includes other
>  files that define the specific parts depending on HAVE_INT64_TIMESTAMP instead
>  of testing for HAVE_INT64_TIMESTAMP many times throughout timestamp.h? I think
>  this might more cleanly separate the logic for the different timestamp types.
>
>  Thanks,
>  wt
>
>  --
>  Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>  To make changes to your subscription:
>  http://www.postgresql.org/mailpref/pgsql-hackers
>


Re: timestamp datatype cleanup

From
"Warren Turkal"
Date:
Is there anything I can do to help?

wt

On Tue, Mar 18, 2008 at 7:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Warren Turkal" <turkal@google.com> writes:
>  > Give the discussion on this. Is this small patch being considered for
>  > inclusion? If not, what do I need to change to make it acceptable?
>
>  It's in the to-do queue for the current commit fest.  The queue is kinda
>  long however :-(
>
>                         regards, tom lane
>


Re: timestamp datatype cleanup

From
Tom Lane
Date:
"Warren Turkal" <turkal@google.com> writes:
> Give the discussion on this. Is this small patch being considered for
> inclusion? If not, what do I need to change to make it acceptable?

It's in the to-do queue for the current commit fest.  The queue is kinda
long however :-(
        regards, tom lane


Re: timestamp datatype cleanup

From
Tom Lane
Date:
Warren Turkal <turkal@google.com> writes:
> Here's an initial bit of my attempt at cleaning up the the timestamp datatype.

I'm starting to work through this now.  Your two messages of 3/09 are
still the latest version correct?

> 2) Would it be reasonable to change timestamp.h into a file that
> includes other files that define the specific parts depending on
> HAVE_INT64_TIMESTAMP instead of testing for HAVE_INT64_TIMESTAMP many
> times throughout timestamp.h? I think this might more cleanly separate
> the logic for the different timestamp types.

I think this is probably a bad idea on maintainability grounds.  First,
you'd be duplicating the declarations that are the same, which would
leave you open to changing one copy and not the other.  Second, having
both versions of any given declaration adjacent to each other makes it
easier to compare them and keep them in sync.  Two separate files just
sounds like a recipe for code drift.
        regards, tom lane


Re: timestamp datatype cleanup

From
"Warren Turkal"
Date:
On Thu, Mar 20, 2008 at 5:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Warren Turkal <turkal@google.com> writes:
>
> > Here's an initial bit of my attempt at cleaning up the the timestamp datatype.
>
>  I'm starting to work through this now.  Your two messages of 3/09 are
>  still the latest version correct?

Yes, I can rebase it if neccessary.

>  > 2) Would it be reasonable to change timestamp.h into a file that
>  > includes other files that define the specific parts depending on
>  > HAVE_INT64_TIMESTAMP instead of testing for HAVE_INT64_TIMESTAMP many
>  > times throughout timestamp.h? I think this might more cleanly separate
>  > the logic for the different timestamp types.
>
>  I think this is probably a bad idea on maintainability grounds.  First,
>  you'd be duplicating the declarations that are the same, which would
>  leave you open to changing one copy and not the other.  Second, having
>  both versions of any given declaration adjacent to each other makes it
>  easier to compare them and keep them in sync.  Two separate files just
>  sounds like a recipe for code drift.

That was what I afraid of and why I posed the question.

I have to say, I am wondering more and more how real the need is for
the two representations of timestamps. Would it be better to deprecate
the float format or at least make the int64 format the default?

wt


Re: timestamp datatype cleanup

From
Tom Lane
Date:
"Warren Turkal" <turkal@google.com> writes:
> I have to say, I am wondering more and more how real the need is for
> the two representations of timestamps. Would it be better to deprecate
> the float format or at least make the int64 format the default?

This was gone over in great detail just recently ... didn't you see
that thread?
        regards, tom lane


Re: timestamp datatype cleanup

From
"Warren Turkal"
Date:
On Thu, Mar 20, 2008 at 6:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Warren Turkal" <turkal@google.com> writes:
>
> > I have to say, I am wondering more and more how real the need is for
>  > the two representations of timestamps. Would it be better to deprecate
>  > the float format or at least make the int64 format the default?
>
>  This was gone over in great detail just recently ... didn't you see
>  that thread?

I actually hadnn't seen the resolution until just now. Thanks for
pointing it out.

wt