Re: timestamp datatype cleanup

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

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

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/

> >> 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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

>>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/

Michael Meskes <meskes(at)postgresql(dot)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

>>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/

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(at)google(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

"Warren Turkal" <turkal(at)google(dot)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

Is there anything I can do to help?

wt

On Tue, Mar 18, 2008 at 7:49 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Warren Turkal" <turkal(at)google(dot)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
>

Warren Turkal <turkal(at)google(dot)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

On Thu, Mar 20, 2008 at 5:22 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Warren Turkal <turkal(at)google(dot)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

"Warren Turkal" <turkal(at)google(dot)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

On Thu, Mar 20, 2008 at 6:48 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Warren Turkal" <turkal(at)google(dot)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