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

Tue, 6 Nov 2001 04:38:45 -0500
Brent Verner <brent@rcfile.org>
RelationFlushRelation() or RelationClearRelation()
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

Tue, 06 Nov 2001 14:54:39 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: RelationFlushRelation() or RelationClearRelation() 
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

Tue, 6 Nov 2001 18:07:26 -0500
Brent Verner <brent@rcfile.org>
Re: RelationFlushRelation() or RelationClearRelation()
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

Tue, 06 Nov 2001 18:50:08 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: RelationFlushRelation() or RelationClearRelation() 
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

Tue, 06 Nov 2001 19:08:28 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: RelationFlushRelation() or RelationClearRelation() 
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

Tue, 6 Nov 2001 23:38:44 -0500
Brent Verner <brent@rcfile.org>
Re: RelationFlushRelation() or RelationClearRelation()
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

Tue, 06 Nov 2001 23:57:18 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: RelationFlushRelation() or RelationClearRelation() 
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

Fri, 9 Nov 2001 23:32:39 -0500
Brent Verner <brent@rcfile.org>
Re: RelationFlushRelation() or RelationClearRelation()
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

Fri, 09 Nov 2001 23:41:39 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: RelationFlushRelation() or RelationClearRelation() 
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

Sat, 10 Nov 2001 06:07:50 -0500
Brent Verner <brent@rcfile.org>
Re: RelationFlushRelation() or RelationClearRelation()
> 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, Pennsylvania 19026

Sat, 10 Nov 2001 11:11:16 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: RelationFlushRelation() or RelationClearRelation()