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