Re: uuid patch 3.0 (8.3devel) - Mailing list pgsql-patches

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