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
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
Show quoted text
-----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 timeI 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?
"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
Show quoted text
-----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
Attachments:
command.capplication/octet-stream; name=command.cDownload
case CONSTR_PRIMARY:
{
char *iname = constr->name;
bool istemp = is_temp_rel_name(relationName);
Relation rel;
List *indexoidlist;
List *indexoidscan;
Form_pg_attribute *rel_attrs;
int num_keys = 0;
int keys_matched = 0;
bool index_found = false;
bool index_found_primary = false;
/* If the constraint name is not specified, generate a name */
if (iname == NULL) {
Oid indoid;
int pass = 0;
char *typename = palloc(NAMEDATALEN);
Ident *key;
/* Assume that the length of the attr list is already > 0 */
/* Get the first attribute so we can use its name */
key = (Ident *)lfirst(constr->keys);
/* Initialise typename to 'pkey' */
snprintf(typename, NAMEDATALEN, "pkey");
for (;;)
{
iname = makeObjectName(relationName, key->name, typename);
/* Check for a conflict */
indoid = RelnameFindRelid(iname);
/* If the oid was not found, then we have a safe name */
if ((!istemp && !OidIsValid(indoid)) ||
(istemp && !is_temp_rel_name(iname)))
break;
/* Found a conflict, so try a new name component */
pfree(iname);
snprintf(typename, NAMEDATALEN, "pkey%d", ++pass);
}
}
/* Need to check for primary key already on field(s) */
rel = heap_openr(relationName, AccessExclusiveLock);
/*
* First we check for limited correctness of the
* constraint
*/
rel_attrs = rel->rd_att->attrs;
/* Retrieve the oids of all indices on the relation */
indexoidlist = RelationGetIndexList(rel);
index_found = false;
index_found_primary = false;
/* Loop over all indices on the relation */
foreach(indexoidscan, indexoidlist)
{
Oid indexoid = lfirsti(indexoidscan);
HeapTuple indexTuple;
Form_pg_index indexStruct;
List *keyl;
int i;
indexTuple = SearchSysCache(INDEXRELID,
ObjectIdGetDatum(indexoid),
0, 0, 0);
if (!HeapTupleIsValid(indexTuple))
elog(ERROR, "ALTER TABLE/ADD CONSTRAINT: Index \"%u\" not found",
indexoid);
indexStruct = (Form_pg_index) GETSTRUCT(indexTuple);
index_found_primary = indexStruct->indisprimary;
/* @@ This check is always false @@ */
if (index_found_primary) {
ReleaseSysCache(indexTuple);
break;
}
/*
* If it's not a primary index, then check to see if it overlaps
* the new constraint.
* Make sure this index has the same number of
* keys as the constraint -- It obviously won't match otherwise.
*/
for (i = 0; i < INDEX_MAX_KEYS && indexStruct->indkey[i] != 0; i++);
num_keys = length(constr->keys);
keys_matched = 0;
if (i == num_keys)
{
/* Loop over each key in the constraint and check that there is a
corresponding key in the index. */
i = 0;
foreach(keyl, constr->keys)
{
Ident *key = lfirst(keyl);
/* 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 differentiate between (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++;
}
}
else elog(ERROR, "ALTER TABLE/ADD CONSTRAINT: Key \"%u[%u]\" not found", indexoid, i);
i++;
}
if (keys_matched == num_keys) {
index_found = true;
ReleaseSysCache(indexTuple);
break;
}
}
ReleaseSysCache(indexTuple);
}
freeList(indexoidlist);
if (index_found_primary)
elog(ERROR, "Primary key already defined on relation \"%s\"", relationName);
/* If everything is ok, create the new index (constraint) */
DefineIndex(
relationName,
iname,
"btree",
constr->keys,
true,
true,
NULL,
NIL);
/* Issue notice */
elog(NOTICE, "ALTER TABLE/ADD PRIMARY KEY will create implicit index '%s' for table '%s'",
iname, relationName);
if (index_found)
elog(NOTICE, "Primary Key constraint supercedes existing index on relation \"%s\". Drop the existing index to remove redundancy.", relationName);
pfree(iname);
/* Finally, close relation */
heap_close(rel, NoLock);
break;
}
"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 differentiate between (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