Re: Misc. consequences of backend memory management changes

> I'm about halfway done with revising backend memory management per
> the discussions on pghackers around the end of April (see the
> just-committed src/backend/utils/mmgr/README for latest version
> of the proposal). There are several things that weren't previously
> discussed that are now looking like good ideas. I don't think
> these'll be too controversial, but I wanted to mention them in case
> anyone gets unhappy:
>
> 1. src/backend/nodes/freefuncs.c is currently dead code; it's not
> called from anywhere. I had proposed reviving it to use for freeing
> rule trees that are stored in relcache entries (currently, a relcache
> flush leaks storage for any rules associated with flushed entries)
> but Hiroshi and others objected that freeObject() is inherently unsafe
> since it can't cope with nodetrees having multiple links to the same
> node --- and we do create such trees in places. We now have a much
> better, safer, and faster mechanism for getting rid of arbitrary node
> trees: construct them in a private memory context that you can delete
> (or just reset) when you don't want the tree anymore. Therefore I'm
> now persuaded that freefuncs.c never will be anything but dead code.
> I propose removing it entirely, so that people won't feel they have
> to maintain it when adding/revising/deleting node types. Any
> objections?

Sure.

>
> 2. Currently there is some code in aset.c that arranges to wipe
> freed space immediately upon its being freed, as an aid in detecting
> attempted uses of already-freed objects. It's conditionally compiled
> based on "#ifdef CLOBBER_FREED_MEMORY", which ought to be mentioned
> in config.h.in (but isn't yet). This slows things down a little bit
> so I wouldn't recommend having it turned on for production use, but
> I think it would be a good idea to keep it turned on during the
> development and early beta phases of 7.1. With the memory
> management changes I think we will be at increased risk of having
> use-of-freed-memory bugs, so I'd like to get as much testing as
> possible done with this code enabled. Will anyone object if I make
> CLOBBER_FREED_MEMORY defined by default until 7.1 release is near?

Why not have it enabled when cassert is enabled?

>
> 3. The portal ("DECLARE CURSOR") code is now critically dependent on
> being able to copy parse and plan trees with copyObject(). Formerly it
> didn't copy them, instead using a hack that involved relabeling the
> working "blank" portal as a permanent portal so that its contents
> wouldn't go away at end of statement. That won't work anymore (blank
> portals are history) so the constructed trees have to be explicitly
> copied into the memory context that's created for the cursor portal.
> The reason I bring this up is that we've had bugs in the past with some
> node types or tree structures not being understood or properly copied by
> copyObject(). Such a bug would now manifest itself as a failure
> occurring only when some particular feature is used in a DECLARE CURSOR.
> To help catch such problems sooner, it'd be a good idea to have
> conditionally-compiled test code that forces *every* generated parse and
> plan tree to be passed through copyObject(). Again, I propose setting
> up config.h symbols for this and turning them on by default during the
> 7.1 development phase.

Same, why not use cassert, or maybe change cassert to debug and all
developers will enable 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