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
