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

> As far as I can tell, it doesn't --- drop table seems to work just fine
> without it.
>
> I have been thinking some more about this, and have come to the
> conclusion that it is only safe to call CommandCounterIncrement
> at points where you have a self-consistent catalog tuple state.
> In particular it must be possible to build valid relcache entries
> from whatever tuples you are making visible with the increment.

This is a good analysis.

>
> For example, it's OK for heap_create to call C.C.I. after creating the
> relation's pg_class, pg_type, and pg_attribute entries; the relation
> is not finished, since it lacks default and constraint info, but
> relcache.c won't have a problem with that. (Note that heap_create
> explicitly forces a rebuild of the relcache entry after it's added
> that extra stuff!)
>
> It is *not* OK for heap_drop to call C.C.I. where it was doing it,
> because it had already deleted the pg_attribute tuples, but was still
> holding a refcount lock on the relcache entry for the target relation.
> (If the refcount were zero, then relcache.c would have just dropped
> the entry instead of trying to rebuild it...)
>
> The heap_drop code was risky even in its original form of
> setheapoverride, since had a relcache rebuild been caused during
> DeleteTypeTuple(), it would have failed. (This could happen,
> in the current state of the code, if an SI Reset message arrives
> and gets processed while DeleteTypeTuple is trying to open pg_type.)
> Switching to CommandCounterIncrement just exposed the latent bug
> by forcing the rebuild attempt to occur.

This is an excellent point. We know we have some instability in
creating/droping tables in separate sessions at the same time. This may
not fix that, but it is clearly an issue that an SI message could arrive
at that time.

>
> In short, I have convinced myself that this is all fine. I will
> finish ripping out setheapoverride and commit the changes tonight.
> Should be able to simplify tqual.c a little bit now that we don't
> need the override code.

I know I am responsible for at least one of those function calls. I
remember asking about it in the past. I have added my first two emails
about this below. I may have added it whenever I did heap_update
because I never knew what it did, and the name was confusing to me.

---------------------------------------------------------------------------

From maillist Fri Aug 14 22:22:06 1998
Date: Fri, 14 Aug 1998 22:22:06 -0400 (EDT)
X-Mailer: ELM [version 2.4ME+ PL43 (25)]
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
XFMstatus: 0000
To: (PostgreSQL-development) <hackers(at)postgreSQL(dot)org>
Subject: setheapoverride
Content-Length: 346
Status: RO

Can someone tell me what setheapoverride() does? I see it around
heap_replace a lot.

---------------------------------------------------------------------------

Date: Sat, 18 Sep 1999 17:25:43 -0400 (EDT)
X-Mailer: ELM [version 2.4ME+ PL56 (25)]
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
XFMstatus: 0000
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: [HACKERS] setheapoverride() considered harmful
Cc: pgsql-hackers(at)postgreSQL(dot)org
Content-Length: 628
Status: RO

> I think we need to get rid of setheapoverride().

I have always wondered what it did. It is in my personal TODO with a
questionmark. Never figured out its purpose.

> since this way the tuples still look valid if we look at them again
> later in the same command.
>
> Comments? Anyone know a reason not to get rid of setheapoverride?

Yes, please remove it.

--
Bruce Momjian | http://www.op.net/~candle
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026