Thread: ALTER TABLE RENAME fix
These patches fix the problem where an ALTER TABLE <table> RENAME <oldcolumn> TO <newcolumn> did not update the RI_ triggers if the oldcolumn was referenced in a RI constraint. The net effect of these patches is that the following sql now works. create table parent (id int UNIQUE); create table child (id int references parent(id) on update cascade); alter table parent rename id to hello; insert into parent values(1); insert into child values(1); The first patch (pgsql-1.diff) moves the defines for the RI_ function args to an include, instead of just being local definitions. It also changes the name of one of the defines and adds another define so that all function args in the pg_trigger.tgargs bytea are #defined. The second patch (pgsql-2.diff) adds a function update_trigger_tgargs(). See comments in the file for explanation. This code could /really/ use a sanity check by someone with more C-foo than I have, to make sure I'm not doing anything blatantly wrong. Also, I'm directly creating a varlena type, which I'm a bit uncomfortable with, so if there is a utility to create these (without creating a char* for use with byteain()), I'd appreciate you pointing me towards that, and I'll resubmit a better patch. I have not exhaustively tested this code, but everything I've thrown at it does the right thing, and no regression failures are caused. I'll work on regression tests after some sleep ;-) cheers. brent -- "Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing." -- Duane Allman
Attachment
Brent Verner <brent@rcfile.org> writes:
> These patches fix the problem where an
> ALTER TABLE <table> RENAME <oldcolumn> TO <newcolumn>
> did not update the RI_ triggers if the oldcolumn was referenced in
> a RI constraint.
This seems wrong on its face: the code unconditionally updates the
attname part of a trigger's tgargs without checking whether the trigger
actually refers to the attribute being renamed. Don't you need to have
a comparison against the old attname in there somewhere?
Another problem is that you're assuming that *all* triggers are RI
triggers. You need to check the trigger function before assuming that
the tgargs are of a form you can parse. There's at least one place in
the existing code that makes such a check already, but I forget where.
Also, I would think that you need to look not only at triggers on the
relation containing the att being renamed, but also at triggers on the
rels that are the other side of the FK relationships. Those triggers
also contain references to the attname, no?
A performance issue here is that scanning all of pg_trigger could be
kind of slow in a database with lots and lots of triggers. You should
be doing an indexscan using pg_trigger_tgrelid_index. Actually, given
the previous comment, probably more than one indexscan. My inclination
would be to accumulate the distinct tgconstrrelid values seen on
triggers that are found to refer to the renamed att during the first
indexscan on the original rel's relid, and then do additional indexscans
looking at those relids (probably there won't be many).
One stylistic quibble: the direct references to CurrentMemoryContext
are verbose and unnecessary. Use palloc() and pstrdup().
regards, tom lane
On 10 Nov 2001 at 14:21 (-0500), Tom Lane wrote: | Brent Verner <brent@rcfile.org> writes: | > These patches fix the problem where an | > ALTER TABLE <table> RENAME <oldcolumn> TO <newcolumn> | > did not update the RI_ triggers if the oldcolumn was referenced in | > a RI constraint. | | This seems wrong on its face: the code unconditionally updates the | attname part of a trigger's tgargs without checking whether the trigger | actually refers to the attribute being renamed. Don't you need to have | a comparison against the old attname in there somewhere? agreed. | Another problem is that you're assuming that *all* triggers are RI | triggers. You need to check the trigger function before assuming that | the tgargs are of a form you can parse. There's at least one place in | the existing code that makes such a check already, but I forget where. Yes, I thought about this as I was going to sleep. I believe I can just check that tgisconstraint='t' in the tuple. I'll make sure this test is sane. | Also, I would think that you need to look not only at triggers on the | relation containing the att being renamed, but also at triggers on the | rels that are the other side of the FK relationships. Those triggers | also contain references to the attname, no? Either this is already being done, or I do not understand what you mean. The code currently will inspect all tuples where tgrelid or tgconstrrelid is the altered relname's oid. If relname appears as tgargs[1] or tgargs[2], the attname tgargs[4] or tgargs[5] is updated appropriately. | A performance issue here is that scanning all of pg_trigger could be | kind of slow in a database with lots and lots of triggers. You should | be doing an indexscan using pg_trigger_tgrelid_index. Actually, given | the previous comment, probably more than one indexscan. My inclination | would be to accumulate the distinct tgconstrrelid values seen on | triggers that are found to refer to the renamed att during the first | indexscan on the original rel's relid, and then do additional indexscans | looking at those relids (probably there won't be many). I'll look into this after I get the rest of the code working correctly. | One stylistic quibble: the direct references to CurrentMemoryContext | are verbose and unnecessary. Use palloc() and pstrdup(). gotcha. Thank you, Brent -- "Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing." -- Duane Allman
Brent Verner <brent@rcfile.org> writes:
> | Another problem is that you're assuming that *all* triggers are RI
> | triggers. You need to check the trigger function before assuming that
> | the tgargs are of a form you can parse. There's at least one place in
> | the existing code that makes such a check already, but I forget where.
> Yes, I thought about this as I was going to sleep. I believe I can just
> check that tgisconstraint='t' in the tuple. I'll make sure this test
> is sane.
The code I was recalling is in trigger.c, and what it does is to test
the tgfoid to see if it's one of the RI_FKEY functions:
/*
* We are interested in RI_FKEY triggers only.
*/
switch (trigger->tgfoid)
{
case F_RI_FKEY_NOACTION_UPD:
case F_RI_FKEY_CASCADE_UPD:
case F_RI_FKEY_RESTRICT_UPD:
case F_RI_FKEY_SETNULL_UPD:
case F_RI_FKEY_SETDEFAULT_UPD:
is_ri_trigger = true;
break;
default:
is_ri_trigger = false;
break;
}
I'd suggest sticking with that approach. (It'd probably be a good idea
to break this chunk of knowledge out into a function, along the lines of
bool is_ri_trigger_function(func_oid), rather than duplicating it in two
different modules.)
> | Also, I would think that you need to look not only at triggers on the
> | relation containing the att being renamed, but also at triggers on the
> | rels that are the other side of the FK relationships. Those triggers
> | also contain references to the attname, no?
> Either this is already being done, or I do not understand what you mean.
> The code currently will inspect all tuples where tgrelid or tgconstrrelid
> is the altered relname's oid. If relname appears as tgargs[1] or tgargs[2],
> the attname tgargs[4] or tgargs[5] is updated appropriately.
But your patch has
> tgrel = heap_openr(TriggerRelationName, AccessShareLock);
> ScanKeyEntryInitialize(&skey, 0, Anum_pg_trigger_tgrelid,
> F_OIDEQ, RelationGetRelid(rel));
> tgscan = heap_beginscan(tgrel, 0, SnapshotNow, 1, &skey);
The scankey tells the scan subroutines to only return tuples matching
the scan key, ie, only tuples with tgrelid = OID of the relation.
So even though you're plowing through all the tuples of pg_trigger,
only those attached to the target rel will be examined.
Oh, one other thing: patches should be submitted in "diff -c" format.
Plain diff format is considered too risky to apply, since it fails
silently if the line numbers are at all different between what you've
been working with and the file the committer is trying to apply the
patch to. A context diff provides a little bit of protection.
regards, tom lane
On 10 Nov 2001 at 19:08 (-0500), Tom Lane wrote: | Brent Verner <brent@rcfile.org> writes: | > | Another problem is that you're assuming that *all* triggers are RI | > | triggers. You need to check the trigger function before assuming that | > | the tgargs are of a form you can parse. There's at least one place in | > | the existing code that makes such a check already, but I forget where. | | > Yes, I thought about this as I was going to sleep. I believe I can just | > check that tgisconstraint='t' in the tuple. I'll make sure this test | > is sane. | | The code I was recalling is in trigger.c, and what it does is to test | the tgfoid to see if it's one of the RI_FKEY functions: [snip] | I'd suggest sticking with that approach. (It'd probably be a good idea | to break this chunk of knowledge out into a function, along the lines of | bool is_ri_trigger_function(func_oid), rather than duplicating it in two | different modules.) Thanks for this tip. I'll make that code a function. | > | Also, I would think that you need to look not only at triggers on the | > | relation containing the att being renamed, but also at triggers on the | > | rels that are the other side of the FK relationships. Those triggers | > | also contain references to the attname, no? | | > Either this is already being done, or I do not understand what you mean. | > The code currently will inspect all tuples where tgrelid or tgconstrrelid | > is the altered relname's oid. If relname appears as tgargs[1] or tgargs[2], | > the attname tgargs[4] or tgargs[5] is updated appropriately. | | But your patch has | | > tgrel = heap_openr(TriggerRelationName, AccessShareLock); | > ScanKeyEntryInitialize(&skey, 0, Anum_pg_trigger_tgrelid, | > F_OIDEQ, RelationGetRelid(rel)); | > tgscan = heap_beginscan(tgrel, 0, SnapshotNow, 1, &skey); | | The scankey tells the scan subroutines to only return tuples matching | the scan key, ie, only tuples with tgrelid = OID of the relation. | So even though you're plowing through all the tuples of pg_trigger, | only those attached to the target rel will be examined. Yes, but I'm doing two separate scans for relname's OID; tgrelid and tgconstrrelid. The second scan returns tuples where tgrelid is the OID of the other relname in the constraint. Is there a way to do this in one scan? | Oh, one other thing: patches should be submitted in "diff -c" format. gotcha. Thanks, Brent -- "Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing." -- Duane Allman
Brent Verner <brent@rcfile.org> writes:
> Yes, but I'm doing two separate scans for relname's OID; tgrelid and
> tgconstrrelid.
Oh! Duh ... I managed to miss that completely. You're right, the
scan for tgconstrrelid should catch the "other half" triggers that
I was worried you'd miss. The code is kinda misleadingly structured,
since it looks at both RI_FK_RELNAME_ARGNO and RI_PK_RELNAME_ARGNO in
each loop, which is really wrong: ISTM you should be examining only one
of those, depending on whether you found the trigger by tgrelid or
tgconstrrelid, and it shouldn't be necessary to look at the stored
relname at all should it?
> The second scan returns tuples where tgrelid is the
> OID of the other relname in the constraint. Is there a way to do
> this in one scan?
I don't think so, but it seems like you could avoid the duplicated code
by having a subroutine that's invoked twice with two different parameter
sets.
Each of the scans could be indexscans (but using two different indexes,
viz pg_trigger_tgconstrrelid_index and pg_trigger_tgrelid_index), so
it should still be plenty fast enough.
regards, tom lane
On 10 Nov 2001 at 19:35 (-0500), Tom Lane wrote:
| Brent Verner <brent@rcfile.org> writes:
| > Yes, but I'm doing two separate scans for relname's OID; tgrelid and
| > tgconstrrelid.
|
| Oh! Duh ... I managed to miss that completely. You're right, the
| scan for tgconstrrelid should catch the "other half" triggers that
| I was worried you'd miss. The code is kinda misleadingly structured,
| since it looks at both RI_FK_RELNAME_ARGNO and RI_PK_RELNAME_ARGNO in
| each loop, which is really wrong: ISTM you should be examining only one
| of those, depending on whether you found the trigger by tgrelid or
| tgconstrrelid, and it shouldn't be necessary to look at the stored
| relname at all should it?
<light bulb over head>
no doubt, very badly/misleadingly written. There is no reason for
me to reconstruct the new tgargs for each tuple at all. I just need
to do the check against PK_RELNAME and FK_RELNAME for the first tuple
that needs updating, since the tgargs is the /same/ for all calls to
this trigger... but we have to check both PK_RELNAME and FK_RELNAME,
since we don't know which side of the constraint this relname is on.
The following pseudo-code describes all I need to do.
newtgargs = NULL;
while(tuple = scan(tgrelid==relname.oid)){
if( is_ri_trigger(tuple) ){
if( newtgargs == NULL ){
get_newtgargs(tuple,newtgargs);
}
update_tgargs(tuple,newtgargs);
}
}
while(tuple = scan(tgconstrrelid==relname.oid)){
if( is_ri_trigger(tuple) ){
update_tgargs(tuple,newtgargs);
}
}
| > The second scan returns tuples where tgrelid is the
| > OID of the other relname in the constraint. Is there a way to do
| > this in one scan?
|
| I don't think so, but it seems like you could avoid the duplicated code
| by having a subroutine that's invoked twice with two different parameter
| sets.
Yeah, I started to do this anyway, since making the same change in two
places was just painful ;-)
| Each of the scans could be indexscans (but using two different indexes,
| viz pg_trigger_tgconstrrelid_index and pg_trigger_tgrelid_index), so
| it should still be plenty fast enough.
I'll make it so.
Thanks,
Brent
--
"Develop your talent, man, and leave the world something. Records are
really gifts from people. To think that an artist would love you enough
to share his music with anyone is a beautiful thing." -- Duane Allman
Brent Verner <brent@rcfile.org> writes:
> ... but we have to check both PK_RELNAME and FK_RELNAME,
> since we don't know which side of the constraint this relname is on.
But we do, because we know whether we're scanning by tgrelid or
tgconstrrelid. This is not merely an efficiency hack, I believe it's
*necessary* to ensure correct functioning. Think about (a) FK
constraints against a different rel that happens to have an att of the
same name as the one being renamed; (b) an FK constraint against the
*same* rel (quite legal).
Another thing in the back of my mind is that I think ALTER RENAME TABLE
has the same problem as ALTER RENAME COLUMN: the relnames in RI triggers
need to be fixed, and aren't getting fixed. It seems to me that this
scan subroutine could be the workhorse for solving both problems, if
it's defined to do string substitution in a particular field of the
RI trigger args. The scan subr should be something like
update_ri_trigger_args (Oid relid,
bool scan_by_constrrelid,
int tgargs_item_num,
const char *oldname,
const char *newname)
with the semantics that it examines all RI triggers with tgrelid=relid
if scan_by_constrrelid is FALSE, or all RI triggers with tgconstrrelid
= relid if scan_by_constrrelid is TRUE. If the field numbered
tgargs_item_num contains the string oldname, replace it with newname;
else do nothing to that trigger.
Now, ALTER RENAME COLUMN does this:
update_ri_trigger_args(relid, false, RI_PK_ATTNAME_ARGNO,
oldattname, newattname);
update_ri_trigger_args(relid, true, RI_FK_ATTNAME_ARGNO,
oldattname, newattname);
and ALTER RENAME TABLE does this:
update_ri_trigger_args(relid, false, RI_PK_RELNAME_ARGNO,
oldrelname, newrelname);
update_ri_trigger_args(relid, true, RI_FK_RELNAME_ARGNO,
oldrelname, newrelname);
I might have the FK and PK roles backwards, but it seems like this
should work.
BTW, you probably need a CommandCounterIncrement between the two scans
in each case. Otherwise the second scan doesn't see the tuples already
updated by the first, which is deadly if there are any self-referential
triggers.
regards, tom lane
On 10 Nov 2001 at 20:26 (-0500), Brent Verner wrote: | <light bulb over head> <sound of light bulb busting> | no doubt, very badly/misleadingly written. There is no reason for | me to reconstruct the new tgargs for each tuple at all. I just need | to do the check against PK_RELNAME and FK_RELNAME for the first tuple | that needs updating, since the tgargs is the /same/ for all calls to | this trigger... but we have to check both PK_RELNAME and FK_RELNAME, | since we don't know which side of the constraint this relname is on. | The following pseudo-code describes all I need to do. disregard this idea. It will not do the right thing with multiple constraints. brent -- "Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing." -- Duane Allman
On 10 Nov 2001 at 20:41 (-0500), Tom Lane wrote: | Brent Verner <brent@rcfile.org> writes: | > ... but we have to check both PK_RELNAME and FK_RELNAME, | > since we don't know which side of the constraint this relname is on. | | But we do, because we know whether we're scanning by tgrelid or | tgconstrrelid. My brain is not stretching far enough to see this. Yes, we know what we are scanning on, but that stilll doesn't tell us which side of the FK/PK relation this relname is on. CREATE TABLE p_rel (id int UNIQUE); CREATE TABLE f_rel (id int REFERENCES p_rel(id) ON UPDATE CASCADE); p_rel.id is our PK f_rel.id is our FK scan on tgrelid==p_rel.oid : 2 tuples. scan on tgconstrrelid==p_rel.oid : 1 tuple. scan on tgrelid==f_rel.oid : 1 tuple. scan on tgconstrrelid==f_rel.oid : 2 tuples. AFAICS, given a relname, the only way we can know if it is the PK or FK is the number of tuples a known scan returns. What am I missing here? | Another thing in the back of my mind is that I think ALTER RENAME TABLE | has the same problem as ALTER RENAME COLUMN: the relnames in RI triggers | need to be fixed, and aren't getting fixed. It seems to me that this | scan subroutine could be the workhorse for solving both problems, if | it's defined to do string substitution in a particular field of the | RI trigger args. The scan subr should be something like | | update_ri_trigger_args (Oid relid, | bool scan_by_constrrelid, | int tgargs_item_num, | const char *oldname, | const char *newname) I wasn't aware of the related ALTER RENAME TABLE problem. This is certainly an appropriate solution. I'll work to do what you suggest. | BTW, you probably need a CommandCounterIncrement between the two scans | in each case. Otherwise the second scan doesn't see the tuples already | updated by the first, which is deadly if there are any self-referential | triggers. cool. Thanks for alerting me of this danger -- I had no idea. thanks. b -- "Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing." -- Duane Allman
Brent Verner <brent@rcfile.org> writes:
> | But we do, because we know whether we're scanning by tgrelid or
> | tgconstrrelid.
> My brain is not stretching far enough to see this. Yes, we know
> what we are scanning on, but that stilll doesn't tell us which
> side of the FK/PK relation this relname is on.
Doesn't it? Maybe I'm the one who's confused. The RI trigger functions
certainly know which field is which without any searching.
I did:
regression=# CREATE TABLE p_rel (p_id int UNIQUE);
NOTICE: CREATE TABLE / UNIQUE will create implicit index 'p_rel_p_id_key' for table 'p_rel'
CREATE
regression=# CREATE TABLE f_rel (f_id int REFERENCES p_rel(p_id) ON UPDATE CASCADE);
NOTICE: CREATE TABLE will create implicit trigger(s) for FOREIGN KEY check(s)
CREATE
regression=# select tgfoid::regproc,tgargs from pg_trigger where tgrelid =
regression-# (select oid from pg_class where relname = 'p_rel');
tgfoid | tgargs
----------------------+----------------------------------------------------------------
RI_FKey_noaction_del | <unnamed>\000f_rel\000p_rel\000UNSPECIFIED\000f_id\000p_id\000
RI_FKey_cascade_upd | <unnamed>\000f_rel\000p_rel\000UNSPECIFIED\000f_id\000p_id\000
(2 rows)
regression=# select tgfoid::regproc,tgargs from pg_trigger where tgconstrrelid =
regression-# (select oid from pg_class where relname = 'p_rel');
tgfoid | tgargs
-------------------+----------------------------------------------------------------
RI_FKey_check_ins | <unnamed>\000f_rel\000p_rel\000UNSPECIFIED\000f_id\000p_id\000
(1 row)
regression=# select tgfoid::regproc,tgargs from pg_trigger where tgrelid =
regression-# (select oid from pg_class where relname = 'f_rel');
tgfoid | tgargs
-------------------+----------------------------------------------------------------
RI_FKey_check_ins | <unnamed>\000f_rel\000p_rel\000UNSPECIFIED\000f_id\000p_id\000
(1 row)
regression=# select tgfoid::regproc,tgargs from pg_trigger where tgconstrrelid =
regression-# (select oid from pg_class where relname = 'f_rel');
tgfoid | tgargs
----------------------+----------------------------------------------------------------
RI_FKey_noaction_del | <unnamed>\000f_rel\000p_rel\000UNSPECIFIED\000f_id\000p_id\000
RI_FKey_cascade_upd | <unnamed>\000f_rel\000p_rel\000UNSPECIFIED\000f_id\000p_id\000
(2 rows)
Hmm, so the same tgargs vector is passed to all the triggers regardless
of which side of the relationship they're on, and it's up to the trigger
itself to know which relname/attname applies to its own table.
I think what this means is that a boolean "is_ri_trigger" classification
isn't good enough. You need to make the knowledge function have a
three-way return: not an RI trigger, RI trigger for PK relation, or
RI trigger for FK relation. Then the scan subroutine has to take two
item-number parameters, one telling it which tgarg item to look at for a
PK relation and a different one to look at for an FK relation.
Or maybe some slightly different structure would be cleaner. But the
point is that you have to be able to decide which item to look at
by some means that doesn't involve checking the relname item. Else it
doesn't work for self-referential RI constraints (same relname on
both sides).
> AFAICS, given a relname, the only way we can know if it is the PK or
> FK is the number of tuples a known scan returns.
The number of tuples is an implementation artifact and might change;
I don't think we should trust that. (For example, RI_FKey_noaction_del
and RI_FKey_cascade_upd could theoretically be merged into a single
trigger that pays attention to the event it was called for.)
regards, tom lane
Brent Verner <brent@rcfile.org> writes:
> exactly. which is why we've gotta inspecy the relname to determine
> which of the attnames to modify.
My point is that inspecting the tgfoid is safer, because it
unambiguously tells you what you are looking at. The relname is
inherently ambiguous because it could be the same on both sides.
Also, depending on the relname is highly fragile if you're trying
to handle the RENAME TABLE case --- did you already update (one
side of) this trigger, or not?
> right. I was not proposing that as any bit of info to use, just an
> example that there is no useful way (in a pg_trigger scan) to know
> if the relname is a PK or FK rel.
There is: the tgfoid, which is indirectly the same way that the trigger
functions themselves know what to do.
regards, tom lane
On 10 Nov 2001 at 21:41 (-0500), Tom Lane wrote: | Brent Verner <brent@rcfile.org> writes: | > | But we do, because we know whether we're scanning by tgrelid or | > | tgconstrrelid. | | > My brain is not stretching far enough to see this. Yes, we know | > what we are scanning on, but that stilll doesn't tell us which | > side of the FK/PK relation this relname is on. | | Doesn't it? Maybe I'm the one who's confused. The RI trigger functions | certainly know which field is which without any searching. [snip] | Hmm, so the same tgargs vector is passed to all the triggers regardless | of which side of the relationship they're on, and it's up to the trigger | itself to know which relname/attname applies to its own table. exactly. which is why we've gotta inspecy the relname to determine which of the attnames to modify. | I think what this means is that a boolean "is_ri_trigger" classification | isn't good enough. You need to make the knowledge function have a | three-way return: not an RI trigger, RI trigger for PK relation, or | RI trigger for FK relation. Then the scan subroutine has to take two | item-number parameters, one telling it which tgarg item to look at for a | PK relation and a different one to look at for an FK relation. I'm not sure why this is necessary. A scan on tgrelid==relname.oid returns an RI trigger. A scan on tgconstrrelid==relname.oid returns an RI trigger. We must update these tuples. We can't know if the relname being modified is the PK or FK, and it really doesn't matter, because its name or attname must be updated to refer to the new name. This is why I'm checking relname in either the PK or FK position for each tuple. Is there a case where updating these tuples tgargs to contain the new relname|attname would break things? If not, I believe my 'update all references to the oldname' behavior is acceptable, and even correct :) | Or maybe some slightly different structure would be cleaner. But the | point is that you have to be able to decide which item to look at | by some means that doesn't involve checking the relname item. Else it | doesn't work for self-referential RI constraints (same relname on | both sides). do you have a bit of sql in mind so that I might test this self-referential RI constraint? | > AFAICS, given a relname, the only way we can know if it is the PK or | > FK is the number of tuples a known scan returns. | | The number of tuples is an implementation artifact and might change; | I don't think we should trust that. (For example, RI_FKey_noaction_del | and RI_FKey_cascade_upd could theoretically be merged into a single | trigger that pays attention to the event it was called for.) right. I was not proposing that as any bit of info to use, just an example that there is no useful way (in a pg_trigger scan) to know if the relname is a PK or FK rel. cheers. brent -- "Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing." -- Duane Allman
On 10 Nov 2001 at 22:42 (-0500), Tom Lane wrote: | Brent Verner <brent@rcfile.org> writes: | > exactly. which is why we've gotta inspecy the relname to determine | > which of the attnames to modify. | | My point is that inspecting the tgfoid is safer, because it | unambiguously tells you what you are looking at. The relname is | inherently ambiguous because it could be the same on both sides. gotcha! I'm digging further into how the F_RI_FKEY_* defines relate to this case. thanks. brent -- "Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing." -- Duane Allman
On 10 Nov 2001 at 23:25 (-0500), Brent Verner wrote: | On 10 Nov 2001 at 22:42 (-0500), Tom Lane wrote: | | Brent Verner <brent@rcfile.org> writes: | | > exactly. which is why we've gotta inspecy the relname to determine | | > which of the attnames to modify. | | | | My point is that inspecting the tgfoid is safer, because it | | unambiguously tells you what you are looking at. The relname is | | inherently ambiguous because it could be the same on both sides. | | gotcha! I'm digging further into how the F_RI_FKEY_* defines | relate to this case. Ok, I'm sending an updated patch that does not use relname at all, and uses an indexscan (getting the Buffer Leak message was fun ;-). cheers. brent -- "Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing." -- Duane Allman
Brent Verner writes: > These patches fix the problem where an > > ALTER TABLE <table> RENAME <oldcolumn> TO <newcolumn> > > did not update the RI_ triggers if the oldcolumn was referenced in > a RI constraint. Instead of trying to fix this, how about making the RI triggers not use the column names in the first place. (Instead they should use the oid of the table and the attnums.) -- Peter Eisentraut peter_e@gmx.net
Peter Eisentraut <peter_e@gmx.net> writes:
> Instead of trying to fix this, how about making the RI triggers not use
> the column names in the first place. (Instead they should use the oid of
> the table and the attnums.)
That would probably be a better answer in the long run, but since it
implies initdb for everyone with an RI trigger, I'm disinclined to
attack it that way during beta.
regards, tom lane
> Peter Eisentraut <peter_e@gmx.net> writes: > > Instead of trying to fix this, how about making the RI triggers not use > > the column names in the first place. (Instead they should use the oid of > > the table and the attnums.) > > That would probably be a better answer in the long run, but since it > implies initdb for everyone with an RI trigger, I'm disinclined to > attack it that way during beta. If that is the proper answer, maybe we should just leave it and fix it properly in 7.3. -- 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
On 12 Nov 2001 at 18:28 (+0100), Peter Eisentraut wrote:
| Brent Verner writes:
|
| > These patches fix the problem where an
| >
| > ALTER TABLE <table> RENAME <oldcolumn> TO <newcolumn>
| >
| > did not update the RI_ triggers if the oldcolumn was referenced in
| > a RI constraint.
|
| Instead of trying to fix this, how about making the RI triggers not use
| the column names in the first place. (Instead they should use the oid of
| the table and the attnums.)
I agree with you. Two things led me toward the current approach
(modifying tgargs)
1) it would require fewer changes to get correct behavior, which
was important as late in beta as I started and
2) I had /some/ idea of how to get it done ;-)
I'll begin to look at what would be required to do it the right
way for 7.3.
cheers.
brent
--
"Develop your talent, man, and leave the world something. Records are
really gifts from people. To think that an artist would love you enough
to share his music with anyone is a beautiful thing." -- Duane Allman
> On 12 Nov 2001 at 18:28 (+0100), Peter Eisentraut wrote:
> | Brent Verner writes:
> |
> | > These patches fix the problem where an
> | >
> | > ALTER TABLE <table> RENAME <oldcolumn> TO <newcolumn>
> | >
> | > did not update the RI_ triggers if the oldcolumn was referenced in
> | > a RI constraint.
> |
> | Instead of trying to fix this, how about making the RI triggers not use
> | the column names in the first place. (Instead they should use the oid of
> | the table and the attnums.)
>
> I agree with you. Two things led me toward the current approach
> (modifying tgargs)
> 1) it would require fewer changes to get correct behavior, which
> was important as late in beta as I started and
> 2) I had /some/ idea of how to get it done ;-)
>
> I'll begin to look at what would be required to do it the right
> way for 7.3.
Added to TODO:
* Make triggers refer to columns by oid, not name
--
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
On Mon, 12 Nov 2001, Brent Verner wrote: > On 12 Nov 2001 at 18:28 (+0100), Peter Eisentraut wrote: > | Brent Verner writes: > | > | > These patches fix the problem where an > | > > | > ALTER TABLE <table> RENAME <oldcolumn> TO <newcolumn> > | > > | > did not update the RI_ triggers if the oldcolumn was referenced in > | > a RI constraint. > | > | Instead of trying to fix this, how about making the RI triggers not use > | the column names in the first place. (Instead they should use the oid of > | the table and the attnums.) > > I agree with you. Two things led me toward the current approach > (modifying tgargs) > 1) it would require fewer changes to get correct behavior, which > was important as late in beta as I started and > 2) I had /some/ idea of how to get it done ;-) > > I'll begin to look at what would be required to do it the right > way for 7.3. The biggest pain involved is deal with old dumps that have a create constraint trigger with the names which would either mean we'd need to support both for a version or have it massage the data on the create constraint trigger if the function being specified is one of the ri functions since I don't think its preferable to force one to use the new dump to upgrade.
Bruce Momjian wrote: > > > On 12 Nov 2001 at 18:28 (+0100), Peter Eisentraut wrote: > > | Brent Verner writes: > > | > > | > These patches fix the problem where an > > | > > > | > ALTER TABLE <table> RENAME <oldcolumn> TO <newcolumn> > > | > > > | > did not update the RI_ triggers if the oldcolumn was referenced in > > | > a RI constraint. > > | > > | Instead of trying to fix this, how about making the RI triggers not use > > | the column names in the first place. (Instead they should use the oid of > > | the table and the attnums.) > > > > I agree with you. Two things led me toward the current approach > > (modifying tgargs) > > 1) it would require fewer changes to get correct behavior, which > > was important as late in beta as I started and > > 2) I had /some/ idea of how to get it done ;-) > > > > I'll begin to look at what would be required to do it the right > > way for 7.3. > > Added to TODO: > > * Make triggers refer to columns by oid, not name What OIDs could we use to point columns ? Is it really a good idea to use IDs or numbers in the first place ? We know now reference by IDs aren't necessarily good, don't we ? It's not preferable to optimize only *rename*. IMHO this should be considered as a part of total object-dependency mechanism. regards, Hiroshi Inoue
> What OIDs could we use to point columns ? > > Is it really a good idea to use IDs or numbers in the first place ? > We know now reference by IDs aren't necessarily good, don't we ? > It's not preferable to optimize only *rename*. IMHO this should be > considered as a part of total object-dependency mechanism. Sorry, I should have said attribute numbers, not oid, since _Tom_ removed OID's from pg_attribute. :-) Good question. Several people thought attno was the way to go and it seemed more natural because almost everything else goes by attno and not name. -- 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
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Good question. Several people thought attno was the way to go and it
> seemed more natural because almost everything else goes by attno and not
> name.
Since the primary key for pg_attribute is reloid+attnum, it would seem
that that's an appropriate representation for RI trigger links too.
But Hiroshi's got a point: we have a whole set of issues with tracking
object dependencies, and it probably makes sense to think about how we
are going to deal with those issues before we fool around with changing
the representation of RI triggers.
regards, tom lane