uuid patch 2.0 (8.3devel)

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.

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?

Show quoted text

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