Thread: RelationFlushRelation() or RelationClearRelation()
Hi, I've almost got the ALTER TABLE RENAME fixed so it doesn't break triggers referring to the renamed column. The final problem is that after the pg_trigger.tgargs is updated, that change is not visible in the current backend. I see that there are a couple of interesting functions to refresh the RelationCache: RelationClearRelation(Relation, bool); RelationFlushRelation(Relation); Which one of these should I use to have the new pg_trigger data visible in the current backend? Or is there a better way than either of these? I should be able to clean up this work and send a patch tomorrow evening if I get this kink worked out. 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:
> I've almost got the ALTER TABLE RENAME fixed so it doesn't break
> triggers referring to the renamed column. The final problem is that
> after the pg_trigger.tgargs is updated, that change is not visible
> in the current backend.
This should happen automatically as a consequence of the relcache flush
message. Doing a manual RelationClearRelation or whatever is NOT the
answer; if you find yourself doing that, it means that other backends
aren't hearing about the change either.
The usual way to force a relcache flush is to update the relation's
pg_class row. Now that I think about it, I'm not sure ALTER TABLE
RENAME COLUMN would have any direct reason to do that, so it may be
broken already in this regard. Does the relcache entry's column
data get updated with the new name?
regards, tom lane
On 06 Nov 2001 at 14:54 (-0500), Tom Lane wrote: | Brent Verner <brent@rcfile.org> writes: | > I've almost got the ALTER TABLE RENAME fixed so it doesn't break | > triggers referring to the renamed column. The final problem is that | > after the pg_trigger.tgargs is updated, that change is not visible | > in the current backend. | | This should happen automatically as a consequence of the relcache flush | message. Doing a manual RelationClearRelation or whatever is NOT the | answer; if you find yourself doing that, it means that other backends | aren't hearing about the change either. gotcha. I don't know what you mean by 'relcache flush message,' but I'll figure that out soon ;-) | The usual way to force a relcache flush is to update the relation's | pg_class row. Now that I think about it, I'm not sure ALTER TABLE | RENAME COLUMN would have any direct reason to do that, so it may be | broken already in this regard. Does the relcache entry's column | data get updated with the new name? The relation->triggerdesc still has the old tgargs after updating the pg_trigger table, so the triggered RI_ function is called with the old arguments. It is probably noteworthy that I am directly modifying the tgargs in the pg_trigger table, via simple_heap_update(). This modfication is made at the end of renameatt() in rename.c. Could making this change prior to the column rename cause the 'relcache flush message' to do the right thing? [I'm going to try this as soon as I'm off work.] Also, is directly updating the pg_trigger table advisable? I'll look further at trigger.c to see if I overlooked any utility to do this cleaner. 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 <brent@rcfile.org> writes:
> It is probably noteworthy that I am directly modifying the tgargs
> in the pg_trigger table, via simple_heap_update(). This modfication
> is made at the end of renameatt() in rename.
Seems reasonable.
Now that I think about it, the problem is almost certainly that the
relcache+sinval mechanism isn't recognizing this update as requiring
relcache-entry rebuild. If you update a pg_class row, it definitely
does recognize that event as forcing relcache rebuild, and I had thought
that updating a pg_attribute row associated with a relcache entry would
cause one too. But maybe not. We should either extend the sinval code
to make that happen, or tweak renameatt to force a relcache flush
explicitly.
Alternatively, maybe you're expecting too much? The relcache rebuild
doesn't (and isn't supposed to) happen until the next transaction commit
or CommandCounterIncrement. If you've structured the code in a way that
needs the relcache change to happen sooner, then I think we need to find
a way to avoid expecting that to happen.
> Also, is directly updating the pg_trigger table advisable?
simple_heap_update seems pretty direct to me :-) ... what did you have
in mind?
regards, tom lane
I said:
> Now that I think about it, the problem is almost certainly that the
> relcache+sinval mechanism isn't recognizing this update as requiring
> relcache-entry rebuild. If you update a pg_class row, it definitely
> does recognize that event as forcing relcache rebuild, and I had thought
> that updating a pg_attribute row associated with a relcache entry would
> cause one too. But maybe not.
It sure looks to me like the update of the pg_attribute row
should be sufficient to queue a relcache flush message (see
RelationInvalidateHeapTuple and subsidiary routines in
backend/utils/cache/inval.c). We could argue about whether
PrepareForTupleInvalidation needs to test for a wider variety of
relcache-invalidating updates, but nonetheless I don't see why
renameatt would be failing to trigger one. Are you sure it's
not working?
regards, tom lane
Brent Verner <brent@rcfile.org> writes:
> After running the above and without (re)starting a new backend yields
> the following error. After getting a new backend, the behavior is as
> desired.
> brent=# insert into child values(1);
> ERROR: constraint <unnamed>: table parent does not have an attribute id
I wonder whether you're looking in the right place. The RI trigger code
caches query plans --- could that caching be the source of the problem?
(See backend/utils/adt/ri_triggers.c)
regards, tom lane
On 06 Nov 2001 at 19:08 (-0500), Tom Lane wrote: | I said: | > Now that I think about it, the problem is almost certainly that the | > relcache+sinval mechanism isn't recognizing this update as requiring | > relcache-entry rebuild. If you update a pg_class row, it definitely | > does recognize that event as forcing relcache rebuild, and I had thought | > that updating a pg_attribute row associated with a relcache entry would | > cause one too. But maybe not. | | It sure looks to me like the update of the pg_attribute row | should be sufficient to queue a relcache flush message (see | RelationInvalidateHeapTuple and subsidiary routines in | backend/utils/cache/inval.c). We could argue about whether | PrepareForTupleInvalidation needs to test for a wider variety of | relcache-invalidating updates, but nonetheless I don't see why | renameatt would be failing to trigger one. Are you sure it's | not working? Pretty darned sure that I've accurately described my symptoms. Doing the tgargs update before or after the actual column update did not affect the behavior -- the cached relation->triggerdesc still contains incorrect tgargs. I'll clean up what I have and post a patch for review. To reply to your earlier email asking what do I expect. I'd like to be able to say... brent# create table parent (id int UNIQUE); brent# create table child (id int4 references parent(id) on update cascade);brent# alter table parent RENAME id to hello; brent# insert into parent values (1); brent# insert into child values(1); After running the above and without (re)starting a new backend yields the following error. After getting a new backend, the behavior is as desired. brent=# insert into child values(1); ERROR: constraint <unnamed>: table parent does not have an attribute id brent=# select tgargs from pg_trigger; tgargs ---------------------------------------------------------------- <unnamed>\000child\000parent\000UNSPECIFIED\000id\000hello\000 <unnamed>\000child\000parent\000UNSPECIFIED\000id\000hello\000 <unnamed>\000child\000parent\000UNSPECIFIED\000id\000hello\000 Your comments on this matter have been much appreciated. I'll next look (further) into the sinval mechanism for a way to forcibly invalidate the cached relation->triggerdesc. 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 06 Nov 2001 at 23:57 (-0500), Tom Lane wrote: | Brent Verner <brent@rcfile.org> writes: | > After running the above and without (re)starting a new backend yields | > the following error. After getting a new backend, the behavior is as | > desired. | | > brent=# insert into child values(1); | > ERROR: constraint <unnamed>: table parent does not have an attribute id | | I wonder whether you're looking in the right place. The RI trigger code | caches query plans --- could that caching be the source of the problem? | (See backend/utils/adt/ri_triggers.c) Tom, This code is now working as desired. I believe the problem I was seeing was due to my incorrect (and STUPID) approach to modifying the bytea pg_trigger->tgargs directly... I've since learned about heap_modifytuple(). Anyway, I'm cleaning this patch up, and will be sending it to -patches shortly. Thanks for your assistance with this, and hopefully the next time I decide to hack at PG, I'll choose something a bit more my speed :-P 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 <brent@rcfile.org> writes:
> Thanks for your assistance with this, and hopefully the next time I
> decide to hack at PG, I'll choose something a bit more my speed :-P
Sounds like "your speed" has advanced a couple notches. Hang in there
... how do you think the rest of us learned? ;-)
regards, tom lane
On 09 Nov 2001 at 23:41 (-0500), Tom Lane wrote: | Brent Verner <brent@rcfile.org> writes: | > Thanks for your assistance with this, and hopefully the next time I | > decide to hack at PG, I'll choose something a bit more my speed :-P | | Sounds like "your speed" has advanced a couple notches. Hang in there | ... how do you think the rest of us learned? ;-) I sure hope it didn't hurt anyone elses head as much as it hurts mine. My initial 'success' was sheer luck. In testing, I noticed certain length column names would do the wrong thing... Turns out I had to malloc the struct varlena* so it was padded to account for the size of the attached vl_dat data, to keep vl_dat from stepping all over the place... oh, /that's/ why I like perl ;-) 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 09 Nov 2001 at 23:41 (-0500), Tom Lane wrote: > | Brent Verner <brent@rcfile.org> writes: > | > Thanks for your assistance with this, and hopefully the next time I > | > decide to hack at PG, I'll choose something a bit more my speed :-P > | > | Sounds like "your speed" has advanced a couple notches. Hang in there > | ... how do you think the rest of us learned? ;-) > > I sure hope it didn't hurt anyone elses head as much as it hurts mine. When I see those long function names in subject lines, I hope Tom Lane takes the topic because I used to have to struggle though answering those type of questions. I makes everyone's head hurt, but it's good for you. :-) -- 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