Thread: Beta time
I want to mention that the number of patches submitted has dropped off dramatically. Seems people are prepared for beta and we should start beta as soon as we can. I think the current plan is Friday. Also, I will be on vacation this week. Tom will apply any patches that look good. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> I want to mention that the number of patches submitted has dropped off
> dramatically. Seems people are prepared for beta and we should start
> beta as soon as we can. I think the current plan is Friday.
I'm doing a substantial amount of work on the date/time types. Not
certain it will be ready for Friday. Will know more by then, of course
;)
- Thomas
I spent an hour or two trying to get my ADD PRIMARY KEY patch to work but I'm beginning to think my code is suffering from bit rot. Basically, during the iteration over the indices on the table, looking for other primary indices, none are found. I am checking the indexStruct->indisprimary field, but it always resolves to false. indisunique works fine. It is a trivial change to the ADD UNIQUE code, but it doesn't work. Viewing the system catalogs and '\d' both show the indices as primary, but the SearchSysCache funtion believes that they are not. Is DefineIndex for primary indices broken or something? I have tried putting a CommandCounterIncrement() in there out of desperation, but it does nothing. Does anyone have any ideas? Might have to leave it for 7.3 I guess. Chris > -----Original Message----- > From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org]On Behalf Of Bruce Momjian > Sent: Tuesday, 18 September 2001 12:00 AM > To: PostgreSQL-development > Subject: [HACKERS] Beta time > > > I want to mention that the number of patches submitted has dropped off > dramatically. Seems people are prepared for beta and we should start > beta as soon as we can. I think the current plan is Friday. > > Also, I will be on vacation this week. Tom will apply any patches that > look good. > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (610) 853-3000 > + If your life is a hard drive, | 830 Blythe Avenue > + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 > > ---------------------------(end of broadcast)--------------------------- > TIP 5: Have you checked our extensive FAQ? > > http://www.postgresql.org/users-lounge/docs/faq.html >
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
> I am checking the indexStruct->indisprimary field, but it always resolves to
> false. indisunique works fine. It is a trivial change to the ADD UNIQUE
> code, but it doesn't work. Viewing the system catalogs and '\d' both show
> the indices as primary, but the SearchSysCache funtion believes that they
> are not.
Doesn't make any sense to me either. Want to post your code?
regards, tom lane
Hi has anyone tried Intel's compiler yet? http://developer.intel.com/software/products/eval/ Just wondering what would happen. Cheerio, Link.
Attached is the CONSTR_PRIMARY switch block from command.c. I've marked the problem test with '@@'. Basically the patch all seems to work properly, except that it doesn't recognise existing primarty keys. ie. You can go ALTER TABLE test ADD PRIMARY KEY(a) multiple times and it will keep adding a primary key. My ADD UNIQUE patch that has been committed is virtually identical, and has no such problem. Chris > -----Original Message----- > From: Tom Lane [mailto:tgl@sss.pgh.pa.us] > Sent: Tuesday, 18 September 2001 12:41 PM > To: Christopher Kings-Lynne > Cc: Bruce Momjian; PostgreSQL-development > Subject: Re: [HACKERS] Beta time > > > "Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes: > > I am checking the indexStruct->indisprimary field, but it > always resolves to > > false. indisunique works fine. It is a trivial change to the > ADD UNIQUE > > code, but it doesn't work. Viewing the system catalogs and > '\d' both show > > the indices as primary, but the SearchSysCache funtion believes > that they > > are not. > > Doesn't make any sense to me either. Want to post your code? > > regards, tom lane >
Attachment
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
> Attached is the CONSTR_PRIMARY switch block from command.c. I've marked the
> problem test with '@@'.
Hmmm .... this code has got a number of problems, but I don't see why
*that* would fail. Anyone?
What I do see:
1. Should not "break" out of loop over indexes after detecting a
matching non-primary-key index. This allows detection of the NOTICE
condition to distract you from detecting the ERROR condition on a
later index. I'd suggest issuing the NOTICE inside the loop, actually,
and not breaking at all. (See also #4)
2. What's with the "if (keyno > 0)"? That breaks detection of
everything on indexes on system columns, eg OID. (Of course, the
"rel_attrs[keyno - 1]" reference doesn't work for system columns,
but sticking your head in the sand is no answer.)
3. pfree'ing iname at the bottom doesn't strike me as a good
idea; isn't that possibly part of your input querytree?
4. If you're going to be so pedantic as to issue a warning notice about
a duplicate non-primary index, it'd be polite to give the name of that
index. Else how shall I know which index you think I should drop?
regards, tom lane
> 1. Should not "break" out of loop over indexes after detecting a > matching non-primary-key index. This allows detection of the NOTICE > condition to distract you from detecting the ERROR condition on a > later index. I'd suggest issuing the NOTICE inside the loop, actually, > and not breaking at all. (See also #4) OK. > 2. What's with the "if (keyno > 0)"? That breaks detection of > everything on indexes on system columns, eg OID. (Of course, the > "rel_attrs[keyno - 1]" reference doesn't work for system columns, > but sticking your head in the sand is no answer.) This is code that I've borrowed from elsewhere. I'll have a good look at it tho. > 3. pfree'ing iname at the bottom doesn't strike me as a good > idea; isn't that possibly part of your input querytree? Hmmm. OK. What about in the case where iname is null and I give it a makeObjectName? > 4. If you're going to be so pedantic as to issue a warning notice about > a duplicate non-primary index, it'd be polite to give the name of that > index. Else how shall I know which index you think I should drop? I'll improve the messages. As for me being pedantic - that's a result of what you specified as the best behaviour should be when I posted on the list! You may also want to look at the CONSTR_UNIQUE block that's already been committed, as it may also have similar issues. Any fixes I make to PRIMARY, I will also fix in UNIQUE... Cheers, Chris
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
>> 3. pfree'ing iname at the bottom doesn't strike me as a good
>> idea; isn't that possibly part of your input querytree?
> Hmmm. OK. What about in the case where iname is null and I give it a
> makeObjectName?
Don't worry about it. palloc'd space will be recovered anyway at end of
statement. It's really not worth the code space to pfree every little
bit of space you might use, except in routines that could be executed
many times in a single query.
regards, tom lane
> 1. Should not "break" out of loop over indexes after detecting a
> matching non-primary-key index. This allows detection of the NOTICE
> condition to distract you from detecting the ERROR condition on a
> later index. I'd suggest issuing the NOTICE inside the loop, actually,
> and not breaking at all. (See also #4)
I don't quite understand what you mean here?
> 2. What's with the "if (keyno > 0)"? That breaks detection of
> everything on indexes on system columns, eg OID. (Of course, the
> "rel_attrs[keyno - 1]" reference doesn't work for system columns,
> but sticking your head in the sand is no answer.)
That is that part of the code that I least understand, so I would
appreciate it if someone took 1 minute and told me how this _should_ be
written. Note that I used this code from the ADD FOREIGN KEY stuff.
/* Look at key[i] in the index and check that it is over the same column as key[i] in the constraint. This is to
differentiatebetween (a,b) and (b,a) */if (i < INDEX_MAX_KEYS && indexStruct->indkey[i] != 0){ int keyno =
indexStruct->indkey[i];
if (keyno > 0) { char *name = NameStr(rel_attrs[keyno - 1]->attname); if (strcmp(name, key->name) == 0)
keys_matched++; }}
I admit I was confused as to why it's keyno - 1??
> 3. pfree'ing iname at the bottom doesn't strike me as a good
> idea; isn't that possibly part of your input querytree?
OK, gone.
> 4. If you're going to be so pedantic as to issue a warning notice about
> a duplicate non-primary index, it'd be polite to give the name of that
> index. Else how shall I know which index you think I should drop?
I was going to do this, but then realised all I had access to in the
indexStruct was the oid of the index relation? What's the easiest way of
retrieving the name of an index given it's oid, or the oid of it's
relation?
Once I've figured these probs out, I'll fix the ADD UNIQUE code as well.
Chris
Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
>> I'd suggest issuing the NOTICE inside the loop, actually,
>> and not breaking at all. (See also #4)
> I don't quite understand what you mean here?
Just do elog(NOTICE) inside the loop over indexes, rather than setting a
flag to do it later. For that matter, I see no reason not to raise the
elog(ERROR) condition inside the loop, rather than setting a flag to
do it later.
> I admit I was confused as to why it's keyno - 1??
To make a zero-based C array index from the one-based attribute number.
But the problem with this code is it doesn't handle indexes on system
attributes such as OID, which have negative attribute numbers and are
not shown in rel->rd_att->attrs. I'd be inclined to use get_attname,
and not bother with looking into the relcache rd_att structure at all.
> I was going to do this, but then realised all I had access to in the
> indexStruct was the oid of the index relation? What's the easiest way of
> retrieving the name of an index given it's oid, or the oid of it's
> relation?
get_rel_name
regards, tom lane