From: | Gevik Babakhani <pgdev(at)xs4all(dot)nl> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Neil Conway <neilc(at)samurai(dot)com>, pgsql-patches <pgsql-patches(at)postgresql(dot)org> |
Subject: | Re: uuid patch 3.0 (8.3devel) |
Date: | 2007-01-28 15:37:37 |
Message-ID: | 1169998657.4814.24.camel@voyager.truesoftware.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly
>
From | Date | Subject | |
---|---|---|---|
Next Message | Neil Conway | 2007-01-28 16:20:09 | Re: uuid patch 3.0 (8.3devel) |
Previous Message | Tom Lane | 2007-01-28 15:00:08 | Re: [ADMIN] server process (PID xxx) was terminated by signal |