Re: [pgsql-patches] uuid patch 3.0 (8.3devel)
On Wed, 2007-01-31 at 12:14 -0500, Tom Lane wrote: Is anything happening with that? This code fails regression on my primary development machine, and my annoyance level is rising rapidly. I'll probably remove the uuid test from the schedule files as a band-aid, if no fix is forthcoming soon. Sorry for the delay -- I should have a patch ready in an hour or two. -Neil ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [pgsql-patches] uuid patch 3.0 (8.3devel)
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 [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [pgsql-patches] uuid patch 3.0 (8.3devel)
On Sat, 2007-01-27 at 23:53 -0500, Tom Lane wrote: A few comments after a quick once-over, perhaps you caught all this already... Much of it :) But thanks for the review. uuid.c header is missing $PostgreSQL$ line, so is uuid.h, copyright notice in the latter seems wrong too. I think the copyright header on completely new code should just include the current year, shouldn't it? I've just marked both files as Copyright (c) 2007 [PGDG]. 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.) Agreed on both points; of course, PG_GETARG_XXX(n) should also be ordered in ascending order of n. 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. Notably, the uuid_t struct doesn't need to be exported. 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 just removed the test, as it seemed unlikely to be helpful. Patch applied with various stylistic changes (including all of Tom's suggestions), catversion bumped. -Neil ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [pgsql-patches] uuid patch 3.0 (8.3devel)
Neil Conway [EMAIL PROTECTED] writes: I think the copyright header on completely new code should just include the current year, shouldn't it? I've just marked both files as Copyright (c) 2007 [PGDG]. I've always made a habit of using the full copyright notice including the UC Berkeley line, because hardly any new files that go into our tree are really written totally from scratch --- there's usually a significant amount of copy-and-paste of older code to begin with, and so I think it's appropriate to acknowledge that the roots go way back. Also, random variations in the format of the notice tend to fool Bruce's copyright-year-updating script. regards, tom lane ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [pgsql-patches] uuid patch 3.0 (8.3devel)
Gevik Babakhani [EMAIL PROTECTED] writes: Is the following OK? * IDENTIFICATION * $PostgreSQL$ Yeah, that's the appropriate starting point --- CVS will take it from there. 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?) I'm sorry, I wasn't clear enough. The pg_cast entries are fine, I was just objecting to the redundant underlying functions. If you look at the other datatypes they use the same functions for casts to text and varchar. regards, tom lane ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [pgsql-patches] uuid patch 3.0 (8.3devel)
On Sat, 27 Jan 2007, Neil Conway wrote: On Fri, 2007-01-26 at 13:11 +0100, Gevik Babakhani wrote: As commented by Peter, I have done some re-styling. Some additional tests and format checking have been added to this patch. 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. This fails on Solaris 9 buildfarm members kudu and dragonfly because they do not support the hh scanf modifier using in UUID_FMTx. Kris Jurka ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [pgsql-patches] uuid patch 3.0 (8.3devel)
On Sun, 2007-01-28 at 13:08 -0500, Kris Jurka wrote:
This fails on Solaris 9 buildfarm members kudu and dragonfly because they
do not support the hh scanf modifier using in UUID_FMTx.
Hmmm. I suppose the easiest thing to do would be to rewrite the uuid
parsing function to not depend on sscanf(). On thinking about it more,
perhaps we should be using the in-memory layout described by RFC 4122:
struct pg_uuid_t
{
uint32 time_low;
uint16 time_mid;
uint16 time_hi_and_version;
uint8clock_seq_hi_and_reserved;
uint8clock_seq_low;
char[6] node;
};
But that can wait for later. Working on fixing the parsing code, I'll
post a patch shortly.
-Neil
---(end of broadcast)---
TIP 4: Have you searched our list archives?
http://archives.postgresql.org
Re: [pgsql-patches] uuid patch 3.0 (8.3devel)
Neil Conway wrote: On Fri, 2007-01-26 at 13:11 +0100, Gevik Babakhani wrote: As commented by Peter, I have done some re-styling. Some additional tests and format checking have been added to this patch. 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. Agreed. -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
