Re: [HACKERS] RE: Getting rid of setheapoverride (was Re: [COMMITTERS] heap.c)

>>>> Oh,I was just looking at heapoverride stuff quite accidentally. 
>>>> Yes, this call is ugly and should be replaced by CommandCounterIncrement()
>> 
>> OK, I'm running a build now with setheapoverride calls removed.
>> Will see what happens.

Well, it seems to work, but...

>> About half of the setheapoverride calls surrounded heap_update()
>> (formerly called heap_replace()) calls.  AFAICS there is no need
>> for these calls unless heap_update itself needs them --- but there
>> are many calls to heap_update that do not have setheapoverride.

I figured out that the cases where setheapoverride (or, now,
CommandCounterIncrement) were needed were the cases where the
heap_update might be updating a tuple created earlier in the
same command.  pg_operator.c has some cases like that, but many of
the other uses of setheapoverride seem to be unnecessary.

However, I'm a little uncomfortable with committing this change,
because my first try at it worked OK at creating things but fell
right over on DROP TABLE.  I had replaced the setheapoverride(true)
in heap_drop_with_catalog() (backend/catalog/heap.c) with a
CommandCounterIncrement, and it failed with this backtrace:

(gdb) bt
#0  elog (lev=-1, fmt=0x6b160 "cannot find attribute %d of relation %s")   at elog.c:112
#1  0x1767d8 in build_tupdesc_ind (buildinfo={infotype = 1, i = {       info_id = 19040,       info_name = 0x4a60}},
relation=0x4006ef00,natts=1074198864) at relcache.c:527
 
#2  0x176554 in RelationBuildTupleDesc (buildinfo={infotype = 1, i = {       info_id = 19040,       info_name = 0x4a60
}},  relation=0x1, natts=1074198864) at relcache.c:437
 
#3  0x177230 in RelationBuildDesc (buildinfo={infotype = 1, i = {       info_id = 19040,       info_name = 0x4a60 }},
oldrelation=0x4006ef00)at relcache.c:808
 
#4  0x177b28 in RelationClearRelation (relation=0x4006ef00, rebuildIt=0 '\000')   at relcache.c:1279
#5  0x177bbc in RelationFlushRelation (relationPtr=0xffffffff,   onlyFlushReferenceCountZero=96 '`') at
relcache.c:1320
#6  0x177e10 in RelationIdInvalidateRelationCacheByRelationId (   relationId=19040) at relcache.c:1415
#7  0x175968 in CacheIdInvalidate (cacheId=4294967295, hashIndex=438624,   pointer=0x1) at inval.c:544
#8  0x175ae8 in InvalidationMessageCacheInvalidate (message=0x4007cce4)   at inval.c:657
#9  0x175490 in LocalInvalidInvalidate (invalid=0x4007cce4 "r",   function=0x4000c3ca <DINFINITY+9226>, freemember=1
'\001')at inval.c:173
 
#10 0x175ca4 in ImmediateLocalInvalidation (send=-1 '�') at inval.c:806
#11 0x9d0b0 in AtCommit_LocalCache () at xact.c:687
#12 0x9cf70 in CommandCounterIncrement () at xact.c:520
#13 0xa7a08 in heap_drop_with_catalog (relname=0x4006ef00 "����")   at heap.c:1528
#14 0xb16ac in RemoveRelation (name=0x40083328 "ff1") at creatinh.c:217
#15 0x13ce84 in ProcessUtility (parsetree=0x40083370, dest=Remote)   at utility.c:201
#16 0x13add0 in pg_exec_query_dest (query_string=0x40024270 "drop table ff1",   dest=Remote, aclOverride=1 '\001') at
postgres.c:721

Apparently, if I call CommandCounterIncrement while partway through
dropping a relation, it will try to rebuild the relcache entry for
the relation --- and of course fail.  I'm too tired to figure out
whether this is just a small coding error in the new cache invalidation
code or whether it's a serious limitation in the whole design.  Hiroshi,
what do you think?

I was able to get around this by simply removing CommandCounterIncrement
from heap_drop_with_catalog entirely --- dropping tables seems to work
fine that way ... but I don't trust it ...
        regards, tom lane