Thread: uuid patch 2.0 (8.3devel)
Folks, Hereby the version 2.0 of the uuid datatype patch with modifications commented by Neil. - the uuid.h has been cleaned. the declarations have been moved to uuid.c - the text_uuid() and varchar_uuid() have been refactored. - all uuid explicit functions are moved to uuid.c and made local. * this patch has been tested on 8.3devel, the snapshot of 25-Jan-07 * this patch uses 28 new oids. I have assigned the oids from 2950. If you need to change the oids, do not do this manually. I have a script that does that. Just provide me 28 unused oids and I will generate a new patch. Please provide comments. Regards, Gevik
Attachment
Gevik Babakhani wrote: > Hereby the version 2.0 of the uuid datatype patch with modifications If I may make some comments on style: Put your file at the end of the OBJS variable (or in some sort of sensible order). Put your file at the end of the tests (or in some sort of sensible order). Refrain from self-evident comments, such as + /* + * function handles input for the uuid datatype + */ + Datum uuid_in(PG_FUNCTION_ARGS) You can probably delete all comments in your patch by that criterion. This sort of super-verbose coding might be alright, but it gets tiring when done systematically for no reason: + result = DirectFunctionCall1(textin, uuid_str); + return result; The uuid.c file claims it is uuid.h. Move the stuff from builtins.h to uuid.h. Move the stuff from uuid.h that is not needed anywhere else to uuid.c. No // comments. Don't number the tests. We might want to insert something later and that would mess everything up. Capitalize the SQL test scripts as in other files. Remove gratuitous whitespace changes (there are many). Also remove the whitespace at the end of lines. Make some reasonable effort to align the catalog entries for readability. -- Peter Eisentraut http://developer.postgresql.org/~petere/
Folks, As commented by Peter, I have done some re-styling. Some additional tests and format checking have been added to this patch. > Put your file at the end of the OBJS variable (or in some sort of > sensible order). Done. > > Put your file at the end of the tests (or in some sort of sensible > order). Done. > Refrain from self-evident comments, such as > > + /* > + * function handles input for the uuid datatype > + */ > + Datum uuid_in(PG_FUNCTION_ARGS) > > You can probably delete all comments in your patch by that criterion. Some changed, but not all has been removed for readability reasons. > This sort of super-verbose coding might be alright, but it gets tiring > when done systematically for no reason: > > + result = DirectFunctionCall1(textin, uuid_str); > + return result; Some changed, but not all. > The uuid.c file claims it is uuid.h. Changed. > > Move the stuff from builtins.h to uuid.h. Not changed. please see: http://archives.postgresql.org/pgsql-patches/2007-01/msg00362.php > Move the stuff from uuid.h that is not needed anywhere else to uuid.c. Done. > No // comments. Done. > > Don't number the tests. We might want to insert something later and > that would mess everything up. Done. > > Capitalize the SQL test scripts as in other files. Done. > > Remove gratuitous whitespace changes (there are many). Done. AFAICS > > Also remove the whitespace at the end of lines. Done. AFAICS > > Make some reasonable effort to align the catalog entries for > readability. > Done. Any more comments? Regards, Gevik.
Attachment
On Fri, 2007-01-26 at 13:11 +0100, Gevik Babakhani wrote: > As commented by Peter, I have done some re-styling. > Some additional tests and format checking have been added to this patch. Barring any objections, I'll apply a revised version of this patch tomorrow. We need some more work on the uuid feature (e.g. generator functions and documentation), but that can be done shortly. -Neil
Neil Conway wrote: > On Fri, 2007-01-26 at 13:11 +0100, Gevik Babakhani wrote: > > As commented by Peter, I have done some re-styling. > > Some additional tests and format checking have been added to this patch. > > Barring any objections, I'll apply a revised version of this patch > tomorrow. We need some more work on the uuid feature (e.g. generator > functions and documentation), but that can be done shortly. Agreed. -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Neil Conway <neilc@samurai.com> writes:
> Barring any objections, I'll apply a revised version of this patch
> tomorrow. We need some more work on the uuid feature (e.g. generator
> functions and documentation), but that can be done shortly.
A few comments after a quick once-over, perhaps you caught all this
already...
uuid.c header is missing $PostgreSQL$ line, so is uuid.h,
copyright notice in the latter seems wrong too.
Generally I like to put "local var = PG_GETARG()" declarations
first in a function, not randomly mixed in with other declarations
of local variables. Think of them as part of the function header.
(Someday we might try to process them with some automatic script,
too... so the less random stylistic variation, the better.)
Please drop the conversions to/from varchar; text is sufficient.
Pay some attention to a logical ordering of the functions in
uuid.c, eg why is uuid_internal_cmp intermixed with unrelated
functions rather than with the ones that call it?
uuid.c contains some functions that are declared static and
then defined without, please clean this up, and make sure
it's not exporting anything it doesn't have to.
Don't put the uuid test at randomly inconsistent places in
parallel_schedule and serial_schedule
The regression test is probably expending a lot more cycles than are
justified, eg what exactly is the point of the last part that inserts
32K random-data records? If it ever did show a failure we'd be unable
to reproduce it, so please at least lose the use of now() and random().
> for(a = 0; a != fmtlen; a++)
OK, this is nitpicky, but there is not a single other C program in the
world that wouldn't have written that with "<" in place of "!=". This
coding is unusual and fragile.
Still haven't fixed all the // comments.
The patch still has some random whitespace changes... particularly
objectionable are the insertions of blank lines far away from
any intended change, eg at the head of various catalog header files.
Don't forget catversion bump, also double-check for duplicate_oids.
regards, tom lane
> uuid.c header is missing $PostgreSQL$ line, so is uuid.h, > copyright notice in the latter seems wrong too. I left this part because it is not clear to me what to put there. Is the following OK? .... * IDENTIFICATION * $PostgreSQL$ * *-------------------------------------------------------------- */ > > Generally I like to put "local var = PG_GETARG()" declarations > first in a function, not randomly mixed in with other declarations > of local variables. Think of them as part of the function header. > (Someday we might try to process them with some automatic script, > too... so the less random stylistic variation, the better.) Moved all possible var = PG_GETARG() to the first line in the functions > Please drop the conversions to/from varchar; text is sufficient. Do you also mean to also remove the casts to/from varchar? (also the catalog entries?) > Pay some attention to a logical ordering of the functions in > uuid.c, eg why is uuid_internal_cmp intermixed with unrelated > functions rather than with the ones that call it? I have relocated all the *helper* functions to not to intermix with *catalog* functions > > uuid.c contains some functions that are declared static and > then defined without, please clean this up, and make sure > it's not exporting anything it doesn't have to. Done. > > Don't put the uuid test at randomly inconsistent places in > parallel_schedule and serial_schedule > > The regression test is probably expending a lot more cycles than are > justified, eg what exactly is the point of the last part that inserts > 32K random-data records? If it ever did show a failure we'd be unable > to reproduce it, so please at least lose the use of now() and random(). I have removed this test because the validity test above already does the job. > > > for(a = 0; a != fmtlen; a++) > OK, this is nitpicky, but there is not a single other C program in the > world that wouldn't have written that with "<" in place of "!=". This > coding is unusual and fragile. Damn my old programming lessons :) (I probably have written crappy for-loops in the past decade) > > Still haven't fixed all the // comments. > Done. > The patch still has some random whitespace changes... particularly > objectionable are the insertions of blank lines far away from > any intended change, eg at the head of various catalog header files. This should never have happened, But it is fixed now > > Don't forget catversion bump, also double-check for duplicate_oids. catversion bump? please explain, Do you mean to change the catalog version? > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 1: if posting/reading through Usenet, please send an appropriate > subscribe-nomail command to majordomo@postgresql.org so that your > message can get through to the mailing list cleanly >
On Sat, 2007-01-27 at 23:53 -0500, Tom Lane wrote: > A few comments after a quick once-over, perhaps you caught all this > already... Much of it :) But thanks for the review. > uuid.c header is missing $PostgreSQL$ line, so is uuid.h, > copyright notice in the latter seems wrong too. I think the copyright header on completely new code should just include the current year, shouldn't it? I've just marked both files as "Copyright (c) 2007 [PGDG]". > Generally I like to put "local var = PG_GETARG()" declarations > first in a function, not randomly mixed in with other declarations > of local variables. Think of them as part of the function header. > (Someday we might try to process them with some automatic script, > too... so the less random stylistic variation, the better.) Agreed on both points; of course, PG_GETARG_XXX(n) should also be ordered in ascending order of "n". > uuid.c contains some functions that are declared static and > then defined without, please clean this up, and make sure > it's not exporting anything it doesn't have to. Notably, the uuid_t struct doesn't need to be exported. > The regression test is probably expending a lot more cycles than are > justified, eg what exactly is the point of the last part that inserts > 32K random-data records? If it ever did show a failure we'd be unable > to reproduce it, so please at least lose the use of now() and random(). I just removed the test, as it seemed unlikely to be helpful. Patch applied with various stylistic changes (including all of Tom's suggestions), catversion bumped. -Neil
Neil Conway <neilc@samurai.com> writes:
> I think the copyright header on completely new code should just include
> the current year, shouldn't it? I've just marked both files as
> "Copyright (c) 2007 [PGDG]".
I've always made a habit of using the full copyright notice including
the UC Berkeley line, because hardly any new files that go into our tree
are really written totally from scratch --- there's usually a
significant amount of copy-and-paste of older code to begin with, and so
I think it's appropriate to acknowledge that the roots go way back.
Also, random variations in the format of the notice tend to fool Bruce's
copyright-year-updating script.
regards, tom lane
Gevik Babakhani <pgdev@xs4all.nl> writes:
> Is the following OK?
> ....
> * IDENTIFICATION
> * $PostgreSQL$
Yeah, that's the appropriate starting point --- CVS will take it from there.
>> Please drop the conversions to/from varchar; text is sufficient.
> Do you also mean to also remove the casts to/from varchar? (also the
> catalog entries?)
I'm sorry, I wasn't clear enough. The pg_cast entries are fine, I was
just objecting to the redundant underlying functions. If you look at
the other datatypes they use the same functions for casts to text and
varchar.
regards, tom lane
On Sat, 27 Jan 2007, Neil Conway wrote: > On Fri, 2007-01-26 at 13:11 +0100, Gevik Babakhani wrote: >> As commented by Peter, I have done some re-styling. >> Some additional tests and format checking have been added to this patch. > > Barring any objections, I'll apply a revised version of this patch > tomorrow. We need some more work on the uuid feature (e.g. generator > functions and documentation), but that can be done shortly. > This fails on Solaris 9 buildfarm members kudu and dragonfly because they do not support the "hh" scanf modifier using in UUID_FMTx. Kris Jurka
On Sun, 2007-01-28 at 13:08 -0500, Kris Jurka wrote:
> This fails on Solaris 9 buildfarm members kudu and dragonfly because they
> do not support the "hh" scanf modifier using in UUID_FMTx.
Hmmm. I suppose the easiest thing to do would be to rewrite the uuid
parsing function to not depend on sscanf(). On thinking about it more,
perhaps we should be using the in-memory layout described by RFC 4122:
struct pg_uuid_t
{
uint32 time_low;
uint16 time_mid;
uint16 time_hi_and_version;
uint8 clock_seq_hi_and_reserved;
uint8 clock_seq_low;
char[6] node;
};
But that can wait for later. Working on fixing the parsing code, I'll
post a patch shortly.
-Neil
Neil Conway <neilc@samurai.com> writes:
> On Sun, 2007-01-28 at 13:08 -0500, Kris Jurka wrote:
>> This fails on Solaris 9 buildfarm members kudu and dragonfly because they
>> do not support the "hh" scanf modifier using in UUID_FMTx.
> Hmmm. I suppose the easiest thing to do would be to rewrite the uuid
> parsing function to not depend on sscanf().
Is anything happening with that? This code fails regression on my
primary development machine, and my annoyance level is rising rapidly.
I'll probably remove the uuid test from the schedule files as a
band-aid, if no fix is forthcoming soon.
regards, tom lane
On Wed, 2007-01-31 at 12:14 -0500, Tom Lane wrote: > Is anything happening with that? This code fails regression on my > primary development machine, and my annoyance level is rising rapidly. > I'll probably remove the uuid test from the schedule files as a > band-aid, if no fix is forthcoming soon. Sorry for the delay -- I should have a patch ready in an hour or two. -Neil