Обсуждение: 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@jabber.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@jabber.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@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
>>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@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 >
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 >
"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
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
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
"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
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