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

> >> 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.

I thought about that this morning and suspected this may be the case,
though I thought tuples would be visible to the same transaction
automatically.  Hard to imagine why we would not want such visibility in
all cases.


> 
> 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 ...

With buggy code like that, it seems we could just try removing them all,
and adding them back in as part of beta testing as people find problems.

--  Bruce Momjian                        |  http://www.op.net/~candle 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