Skip site navigation (1) Skip section navigation (2)

Re: uuid patch 3.0 (8.3devel)

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 (view raw or flat)
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
> 


In response to

Responses

pgsql-patches by date

Next:From: Neil ConwayDate: 2007-01-28 16:20:09
Subject: Re: uuid patch 3.0 (8.3devel)
Previous:From: Tom LaneDate: 2007-01-28 15:00:08
Subject: Re: [ADMIN] server process (PID xxx) was terminated by signal

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group