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

In response to

Responses

Browse pgsql-patches by date

  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