Horton

Thread

  1. Misc. consequences of backend memory management changes

    Tom Lane <tgl@sss.pgh.pa.us> — 2000-06-28T17:04:27Z

    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?

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?

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.

Comments, objections?

			regards, tom lane

  2. Re: Misc. consequences of backend memory management changes

    Bruce Momjian <pgman@candle.pha.pa.us> — 2000-06-28T17:27:35Z

    > 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@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

  3. Re: Misc. consequences of backend memory management changes

    Karel Zak <zakkr@zf.jcu.cz> — 2000-06-28T18:26:59Z

    On Wed, 28 Jun 2000, Tom Lane wrote:

> I'm about halfway done with revising backend memory management per

 haha, I just prepare first query cache snapshot and what I see, 
Tom already rewrite aset/mcxt and my routines are out-of-date. 
But it is right, with your changes it will query cache better :-)  

 Well, I a little surprise that you remove all aset definiton from
header files to aset.c. If anyone will write new context type, he
can't uses some already exist definition.

 IMHO not will bad if all context types (now exists aset type only)
will more transparently and will own header files and at first sight will
visible what is common memory routines and what specific.

 I believe that current memory managemet changes create more _modular_
mem code. 


About context tree --- what will happen if in PG will exist some context 
that not will in context tree? Yes, it is curios question...explication: 
 I have in the query cache contexts (for each plan) that are persisten (in 
shmem) across connection (and across TopMemoryContext live-time). How 
integrate this context to the contect tree? Or skip for this specific 
variant context type independent MemoryContextCreate and init this common 
part itself? - (I vote for this)

 New pfree(), repalloc() are nice. 

 Plan you some changes in SPI? I have new code for SPI save plan
(per-context and via query cache). 

 And last question, is current mcxt.c API final? I want port my query cache
to compatible with current interface.  

					Karel

  4. Re: Misc. consequences of backend memory management changes

    Karel Zak <zakkr@zf.jcu.cz> — 2000-06-28T19:40:35Z

    On Wed, 28 Jun 2000, Karel Zak wrote:

> 
> About context tree --- what will happen if in PG will exist some context 
> that not will in context tree? Yes, it is curios question...explication: 
>  I have in the query cache contexts (for each plan) that are persisten (in 
> shmem) across connection (and across TopMemoryContext live-time). How 
> integrate this context to the contect tree? Or skip for this specific 
> variant context type independent MemoryContextCreate and init this common 
> part itself? - (I vote for this)

 I a little speculate about it and I mean that query cache contexts can be
in separate contest tree based on master query cache context that is create
during global shmem initialization. Agree?

						Karel

  5. Re: Misc. consequences of backend memory management changes

    Tom Lane <tgl@sss.pgh.pa.us> — 2000-06-29T01:37:35Z

    Karel Zak <zakkr@zf.jcu.cz> writes:
>  Well, I a little surprise that you remove all aset definiton from
> header files to aset.c. If anyone will write new context type, he
> can't uses some already exist definition.

>  IMHO not will bad if all context types (now exists aset type only)
> will more transparently and will own header files and at first sight will
> visible what is common memory routines and what specific.

I left AllocSetContext in memnodes.h temporarily, but I think in the
long run it should only be defined inside aset.c.  Anyone else have
an opinion on that?

> About context tree --- what will happen if in PG will exist some context 
> that not will in context tree?

You can certainly have top-level contexts that aren't children of
anything.  There's no real functional distinction between a context
created like that and one that is a child of TopMemoryContext, because
we never reset/delete children of TopMemoryContext anyway.  I was
careful to make all the other contexts descendants of TopMemoryContext
because I wanted to be able to call MemoryContextStats(TopMemoryContext)
to get a picture of all the memory usage.  Other than that
consideration, CacheMemoryContext, ErrorContext, and so forth could
perfectly well have been top-level contexts with no parent.

A context representing shared memory probably should be a top-level
context, though.  That would make sense if you think of TopMemoryContext
as the top-level of all the backend's *local* memory.

> Or skip for this specific variant context type independent
> MemoryContextCreate and init this common part itself? - (I vote for
> this)

No, certainly not.  Just pass a NULL for parent if you don't want to
connect it up to the local context tree.

>  Plan you some changes in SPI? I have new code for SPI save plan
> (per-context and via query cache). 

I have been wondering about that.  I don't like SPI saving plans in
TopMemoryContext because I don't think it knows how to get rid of them;
a plan saved there is effectively a permanent memory leak.  Not sure
what to do about it though.

> And last question, is current mcxt.c API final? I want port my query cache
> to compatible with current interface.  

I think it is done, though I reserve the right to change it if I find out
something needs to be done differently ;-).  But what I am expecting to
do next is just modify the planner and executor to make better use of
the facilities that are there now.  I won't need to change mcxt.c any
more unless I find it's too hard to use or doesn't do quite the right
things...

			regards, tom lane

  6. Re: Misc. consequences of backend memory management changes

    Karel Zak <zakkr@zf.jcu.cz> — 2000-06-29T09:39:07Z

    On Wed, 28 Jun 2000, Tom Lane wrote:

> Karel Zak <zakkr@zf.jcu.cz> writes:
> >  Well, I a little surprise that you remove all aset definiton from
> > header files to aset.c. If anyone will write new context type, he
> > can't uses some already exist definition.
> 
> >  IMHO not will bad if all context types (now exists aset type only)
> > will more transparently and will own header files and at first sight will
> > visible what is common memory routines and what specific.
> 
> I left AllocSetContext in memnodes.h temporarily, but I think in the
> long run it should only be defined inside aset.c.  Anyone else have
> an opinion on that?

 For example I use AllocSet's block definition.

> 
> > About context tree --- what will happen if in PG will exist some context 
> > that not will in context tree?
> 
> You can certainly have top-level contexts that aren't children of
> anything.  There's no real functional distinction between a context
> created like that and one that is a child of TopMemoryContext, because
> we never reset/delete children of TopMemoryContext anyway.  I was
> careful to make all the other contexts descendants of TopMemoryContext
> because I wanted to be able to call MemoryContextStats(TopMemoryContext)
> to get a picture of all the memory usage.  Other than that

 Yes, I see, it is good idea.

> consideration, CacheMemoryContext, ErrorContext, and so forth could
> perfectly well have been top-level contexts with no parent.
> 
> A context representing shared memory probably should be a top-level
> context, though.  That would make sense if you think of TopMemoryContext
> as the top-level of all the backend's *local* memory.
> 
> > Or skip for this specific variant context type independent
> > MemoryContextCreate and init this common part itself? - (I vote for
> > this)
> 
> No, certainly not.  Just pass a NULL for parent if you don't want to
> connect it up to the local context tree.

 Well, the query cache shared memory pool and qCache_TopContext (parent of
all cache entry) are init during postmarter start up. In this time not exist
TopMemoryContext and 'node' for this context will alloc via malloc(). It is 
right and I understand here. This qCache_TopContext is used for internal 
query cache data and all data is in shmem.
 
*But* if I add entry to query cache I create for each entry separate memory 
context and it must be in shared memory, but in shared memory must be not
only data allocated via this context, in shmem must be _all_ for context
relevant data --- so, context independent data too (bacause context must
overlive backend end). A problem is that common MemoryContextCreate() expects 
that context 'node' is allocated in the TopMemoryContext or by force of 
malloc(). Not is possible allocate context node in a parent context.

I read your comment "since the node must survive resets of its parent
context", but how resolve this?
  
  
 I have two method for save cached planns. First in the hash table in shared 
memory (about it we talking) and second in hash table in local memory. For
'local part' context I can set as parent CacheMemoryContext or will better
use TopMemoryConetxt.

> >  Plan you some changes in SPI? I have new code for SPI save plan
> > (per-context and via query cache). 
> 
> I have been wondering about that.  I don't like SPI saving plans in
> TopMemoryContext because I don't think it knows how to get rid of them;
> a plan saved there is effectively a permanent memory leak.  Not sure
> what to do about it though.

 We already told about it with Hiroshi. I rewrite SPI_sevaplan(), now it
save each plan to separate context and is possible use SPI_freeplan().
Also I add 'by_key' interface to SPI that allows save planns to the query 
cache under some key (string or binary key). My SPI *not* use
TopMemoryContext.


						Karel

  7. Re: Misc. consequences of backend memory management changes

    Tom Lane <tgl@sss.pgh.pa.us> — 2000-06-29T14:35:01Z

    Karel Zak <zakkr@zf.jcu.cz> writes:
>>>> Or skip for this specific variant context type independent
>>>> MemoryContextCreate and init this common part itself? - (I vote for
>>>> this)
>> 
>> No, certainly not.  Just pass a NULL for parent if you don't want to
>> connect it up to the local context tree.

>  Well, the query cache shared memory pool and qCache_TopContext (parent of
> all cache entry) are init during postmarter start up. In this time not exist
> TopMemoryContext and 'node' for this context will alloc via malloc().

Look again ;-).  TopMemoryContext is created as the first or second step
in postmaster startup, long before we attach to shared mem.

> *But* if I add entry to query cache I create for each entry separate memory 
> context and it must be in shared memory, but in shared memory must be not
> only data allocated via this context, in shmem must be _all_ for context
> relevant data --- so, context independent data too (bacause context must
> overlive backend end). A problem is that common MemoryContextCreate() expects 
> that context 'node' is allocated in the TopMemoryContext or by force of 
> malloc(). Not is possible allocate context node in a parent context.

Good point.

> I read your comment "since the node must survive resets of its parent
> context", but how resolve this?

Well, there's a fundamental problem with keeping memory contexts that
meet this API in shared memory anyway: the contexts have to contain
parent/child pointers pointing at other contexts, and the contexts
have to contain pointers to context-management function pointer structs.
Neither of these are very safe for shared memory.

You've probably noticed that we keep cross-pointers in shared memory in
the form of offsets from the start of the shmem block, not as absolute
pointers.  This is to support the possibility that the shmem block is
mapped at different addresses in different backends.  (It's possible
that we could give up that flexibility now that all the backends are
forked from a postmaster that's already attached the shmem block.
But I'd rather not wire such an assumption into the contents of shmem.)

Similarly, it would be risky to assume that the functions all appear
at the same address in every backend connected to the shmem block.

What I think you probably want to do is create "placeholder" context
nodes in backend local memory to represent the shared-memory contexts.
The placeholder would contain the MemoryContext fields and a shmem
offset to a struct in shared memory that is the "real" control info
for the context.

If every backend has a placeholder node for every context in shmem
then you have a bit of a problem deleting shmem contexts, but I
think you probably don't need to do that.  There should be a permanent
placeholder for the top-level shmem context (create this during
backend startup), but placeholder context nodes for per-plan contexts
only need to live for as long as you are copying nodetrees into the
context.  They don't have to stick around longer than that.


A few minutes later:

Now that I think about it, this whole notion of copying nodetrees
into shared memory has got a pointer problem, because the constructed
tree is going to be full of pointers that will only be valid for a
backend that has shmem attached at the same address that the creating
backend had.

Do we really want to give up flexibility of shmem attach address in
order to have a shared-memory plan cache?  I've been dubious about
the notion of a shared plan cache from day one (I think the locking
overhead is going to be intolerable) and this may be the problem
that kills it completely.

Before you say "we will never again care about flexibility of attach
address", consider these two scenarios:
1. Examining shmem using a debugging process started independently
   of the postmaster.
2. "Hot swapping" a new compilation of the postmaster/backend without
   service interruption.  This is possible (for changes that don't
   change the layout of shmem, of course) by killing and restarting
   the postmaster without killing extant backends.  But you couldn't
   assume that the new incarnation of the postmaster would attach to
   the existing shmem blocks at the exact same addresses the previous
   incarnation had been assigned.
We don't have either of these today but we might want to do them
at some point in the future.  There may be other reasons that I
didn't come up with in a moment's thought.


>>>> Plan you some changes in SPI? I have new code for SPI save plan
>>>> (per-context and via query cache). 
>> 
>> I have been wondering about that.  I don't like SPI saving plans in
>> TopMemoryContext because I don't think it knows how to get rid of them;
>> a plan saved there is effectively a permanent memory leak.  Not sure
>> what to do about it though.

>  We already told about it with Hiroshi. I rewrite SPI_sevaplan(), now it
> save each plan to separate context and is possible use SPI_freeplan().
> Also I add 'by_key' interface to SPI that allows save planns to the query 
> cache under some key (string or binary key). My SPI *not* use
> TopMemoryContext.

That all sounds good.  Plan cache in local memory seems reasonable to
me, I'm just worried about the shared-mem aspect of it.

			regards, tom lane

  8. Re: Misc. consequences of backend memory management changes

    Karel Zak <zakkr@zf.jcu.cz> — 2000-06-29T16:14:00Z

    On Thu, 29 Jun 2000, Tom Lane wrote:

> Karel Zak <zakkr@zf.jcu.cz> writes:
> >>>> Or skip for this specific variant context type independent
> >>>> MemoryContextCreate and init this common part itself? - (I vote for
> >>>> this)
> >> 
> >> No, certainly not.  Just pass a NULL for parent if you don't want to
> >> connect it up to the local context tree.
> 
> >  Well, the query cache shared memory pool and qCache_TopContext (parent of
> > all cache entry) are init during postmarter start up. In this time not exist
> > TopMemoryContext and 'node' for this context will alloc via malloc().
> 
> Look again ;-).  TopMemoryContext is created as the first or second step
> in postmaster startup, long before we attach to shared mem.

 I looking :-), I don't say that here is a problem. I create master 
qCache_TopContext like standard memory context, but only alloc method will
non-AllocSet. No problem. 

> 
> > *But* if I add entry to query cache I create for each entry separate memory 
> > context and it must be in shared memory, but in shared memory must be not
> > only data allocated via this context, in shmem must be _all_ for context
> > relevant data --- so, context independent data too (bacause context must
> > overlive backend end). A problem is that common MemoryContextCreate() expects 
> > that context 'node' is allocated in the TopMemoryContext or by force of 
> > malloc(). Not is possible allocate context node in a parent context.
> 
> Good point.
> 
> > I read your comment "since the node must survive resets of its parent
> > context", but how resolve this?
> 
> Well, there's a fundamental problem with keeping memory contexts that
> meet this API in shared memory anyway: the contexts have to contain
> parent/child pointers pointing at other contexts, and the contexts
> have to contain pointers to context-management function pointer structs.
> Neither of these are very safe for shared memory.

 First I must say that _all_ with shmem/local query cache _works_ now 
for me in current (7.0) memory design (I can send it). And I hope it must
works in 7.1 too.

 For better understanding I a little describe this cache (qcache).
In postmaster startup and shmem init qcache create shmem pool. This pool 
is split to blocks (now 1024b) these blocks are used in qcache contexts.
All free blocks are in freeblock list. During initialization is created
hash table (in shmem pool) for cached entry and qCache_TopContext (and 
alloc methods) too. All it is in postmaster.  
  If user (PREPARE command) or SPI (by_key interface) add a plan to qcache
it create new memory context in shmem pool and as parent is used master
qCache_TopContext. New plan is copy to shmem via copyObject().
  If user (EXECUTE) or SPI needs a plan in hash table is found relevant
pointer with plan and from this is copy to executor (or ..etc) context.  
  To the shared qcache pool has access only qcache's routines and all 
access are "dress" in spin locks. For real work in backend is always used
some plan copy from qcache.  --- It query cache. 


> You've probably noticed that we keep cross-pointers in shared memory in
> the form of offsets from the start of the shmem block, not as absolute
> pointers.  This is to support the possibility that the shmem block is
> mapped at different addresses in different backends.  (It's possible
> that we could give up that flexibility now that all the backends are
> forked from a postmaster that's already attached the shmem block.
> But I'd rather not wire such an assumption into the contents of shmem.)
> 
> Similarly, it would be risky to assume that the functions all appear
> at the same address in every backend connected to the shmem block.

 All work with shmem must be assure via locks.

> 
> What I think you probably want to do is create "placeholder" context
> nodes in backend local memory to represent the shared-memory contexts.
> The placeholder would contain the MemoryContext fields and a shmem
> offset to a struct in shared memory that is the "real" control info
> for the context.

 I not sure if backend already needs information about shmem pool. Now 
I have all meta-information about shmem pool in one struct (in shmem) 
that is at for all backend known pointer. If I need some information
about my pool I see this meta-data or scan hash table.

 I mean: we can create separate memory context tree based on some 
qCache_TopContext and init in postmaster. Standard backend memory routines
not will know and access to this tree. Only query cache will the bridge
between shmem and standard contexts, but shmem contexts are compatible and
is possible use it in palloc()/pfree(). 

 But I will think about your "placeholder" idea :-) I still a little not
understand who will need placeholder information - debug? The qcache
knows all from own structs.

> If every backend has a placeholder node for every context in shmem
> then you have a bit of a problem deleting shmem contexts, but I
> think you probably don't need to do that.  There should be a permanent

It must be possible destroy context. If query cache is full, it itself
remove a oldest plan that is mark as "removeable" (an example plans from 
SPI). 

> placeholder for the top-level shmem context (create this during
> backend startup), but placeholder context nodes for per-plan contexts
> only need to live for as long as you are copying nodetrees into the
> context.  They don't have to stick around longer than that.
> 
> 
> A few minutes later:

I always have "a few minutes later" after mail sending :-)  

> Now that I think about it, this whole notion of copying nodetrees
> into shared memory has got a pointer problem, because the constructed
> tree is going to be full of pointers that will only be valid for a
> backend that has shmem attached at the same address that the creating
> backend had.

I don't understand, 

test=# PREPARE aaa AS SELECT relname FROM pg_class 
       WHERE relname ~~ $1 USING text;
PREPARE
test=# \c test
You are now connected to database test.       <---------- new backend
test=# EXECUTE aaa USING 'pg_c%';
        relname
------------------------
 pg_class
 pg_class_oid_index
 pg_class_relname_index

it is in 7.0


> Before you say "we will never again care about flexibility of attach
> address", consider these two scenarios:
> 1. Examining shmem using a debugging process started independently
>    of the postmaster.
> 2. "Hot swapping" a new compilation of the postmaster/backend without
>    service interruption.  This is possible (for changes that don't
>    change the layout of shmem, of course) by killing and restarting
>    the postmaster without killing extant backends.  But you couldn't
>    assume that the new incarnation of the postmaster would attach to
>    the existing shmem blocks at the exact same addresses the previous
>    incarnation had been assigned.

 Ops, now I understand, you want on-the-fly change backend. 

I still mean "we will never again care about...." (I'm hard-headed :-) 

(!) All Data in the query cache must be dispensable. User's SPI routines must
allows rebuild query and user (PREPARE) must knows that prepared planns are
not VIEW (!)

 BTW. - if we want really "Hot swapping" for qcache too, we can (also)
save native not parsed query strings into query cache and rebuild it after 
on-the-fly backend change.   

I have some scenario too:

- PostgreSQL is backend for web application and this application very 
often send to PG same query and PG has feature that allows save 95% 
time of query parsing :-) 	

- or user has trigger (RI?) and this trigger always run only 
SPI_execp_by_key()...

Well, I prepare it with '#ifdef HAVE_QCACHE'.  

					Karel

  9. Re: Misc. consequences of backend memory management changes

    Peter Eisentraut <peter_e@gmx.net> — 2000-06-29T17:27:33Z

    Tom Lane writes:

> Will anyone object if I make CLOBBER_FREED_MEMORY defined by default
> until 7.1 release is near?

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

I've been thinking that we need a configure option for this sort of stuff,
like

   --enable-debug=memory,lock,foo,noipc

which would result in

#define MEMORY_DEBUG 1
#define LOCK_DEBUG 1
#define FOO_DEBUG 1
#undef IPC_DEBUG

Comments?


-- 
Peter Eisentraut                  Sernanders väg 10:115
peter_e@gmx.net                   75262 Uppsala
http://yi.org/peter-e/            Sweden

  10. Re: Misc. consequences of backend memory management changes

    Tom Lane <tgl@sss.pgh.pa.us> — 2000-06-29T18:30:07Z

    Peter Eisentraut <peter_e@gmx.net> writes:
>> Will anyone object if I make CLOBBER_FREED_MEMORY defined by default
>> until 7.1 release is near?

> I've been thinking that we need a configure option for this sort of stuff,
> like

>    --enable-debug=memory,lock,foo,noipc

> which would result in

> #define MEMORY_DEBUG 1
> #define LOCK_DEBUG 1
> #define FOO_DEBUG 1
> #undef IPC_DEBUG

> Comments?

Not unreasonable, though it would tend to encourage use of short,
hard-to-understand names for debug options :-(.  We could live with
that as long as there were adequate documentation about what each
option does in config.h.in.

If you wanna do it then I'd suggest folding cassert into the same
mechanism.

Last night I rearranged config.h.in so that the configure-driven symbols
are more clearly separated from the non-configurable symbols, and I also
separated out the debug symbols from the feature/limit symbols.  Should
make a good starting point for seeing what needs to be dealt with.
Unfortunately the existing debug symbols were mostly undocumented, and
I didn't take the time to dig to see what they do ... if anything ...

			regards, tom lane

  11. Re: Misc. consequences of backend memory management changes

    Peter Eisentraut <peter_e@gmx.net> — 2000-06-29T23:37:49Z

    Tom Lane writes:

> Last night I rearranged config.h.in so that the configure-driven
> symbols are more clearly separated from the non-configurable symbols,

I'd find it convenient if we could eventually create config.h.in
automatically from configure.in with autoheader, but in order to do that
configure needs to know about everything that's going on in there. It's
not nice to invite your users to edit header files anyway...


-- 
Peter Eisentraut                  Sernanders väg 10:115
peter_e@gmx.net                   75262 Uppsala
http://yi.org/peter-e/            Sweden

  12. config.h (was Re: Misc. consequences of backend memory management changes)

    Tom Lane <tgl@sss.pgh.pa.us> — 2000-06-30T01:01:21Z

    Peter Eisentraut <peter_e@gmx.net> writes:
> I'd find it convenient if we could eventually create config.h.in
> automatically from configure.in with autoheader, but in order to do that
> configure needs to know about everything that's going on in there. It's
> not nice to invite your users to edit header files anyway...

I looked at autoheader and it strikes me as yet another glorified
'cat'.  If we used it, we'd basically have an acconfig.h that contains
exactly what's now in the handgenerated config.h.in, and autoheader
would copy it all to config.h.in.

If we had a bunch of config.h.in's that could share code, this might
be useful, but for our present purposes I'm missing where the win is.

One thing that does occur to me is that a very large fraction of
config.h is now symbols that are supposed to be set by configure,
and as you say it's not good to give people the idea that they
should tweak those results by hand after configuring.  What do you
think of pulling the remaining hand-settable symbols out into a
separate file, maybe called something like "siteconfig.h"?  Then
config.h becomes purely a machine-generated file.  This would
also solve the problem of losing hand-set config choices if you
rerun configure.

			regards, tom lane

  13. Re: config.h (was Re: Misc. consequences of backend memory management changes)

    Brook Milligan <brook@biology.nmsu.edu> — 2000-06-30T14:56:26Z

       One thing that does occur to me is that a very large fraction of
   config.h is now symbols that are supposed to be set by configure,
   and as you say it's not good to give people the idea that they
   should tweak those results by hand after configuring.  What do you
   think of pulling the remaining hand-settable symbols out into a
   separate file, maybe called something like "siteconfig.h"?  Then
   config.h becomes purely a machine-generated file.  This would
   also solve the problem of losing hand-set config choices if you
   rerun configure.

config.h should contain only machine generated stuff for exactly the
reasons you mention.  Perhaps config.h.in should be something like:

/* siteconfig.h overrides various variables */
#include "siteconfig.h"

/* enable feature FOO (can be overridden by siteconfig.h) */
#ifndef FOO
#undefine FOO
#endif

Cheers,
Brook

  14. Re: Misc. consequences of backend memory management changes

    Chris Bitmead <chris@bitmead.com> — 2000-07-01T04:04:19Z

    Tom Lane wrote:

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

Could we instead have some configure option --devel or something that
turns on all the checks?

  15. Re: config.h (was Re: Misc. consequences of backend memory management changes)

    Peter Eisentraut <peter_e@gmx.net> — 2000-07-01T15:04:04Z

    Tom Lane writes:

> I looked at autoheader and it strikes me as yet another glorified
> 'cat'.  If we used it, we'd basically have an acconfig.h that contains
> exactly what's now in the handgenerated config.h.in, and autoheader
> would copy it all to config.h.in.

That mechanism is obsolete, autoheader is much smarter now (2.13). In
order to get the comments onto the symbols you write

AC_DEFINE(HAVE_FOO, [], [Define if you have foo])

I.e., you document the stuff where you define it.

> What do you think of pulling the remaining hand-settable symbols out
> into a separate file, maybe called something like "siteconfig.h"?  

That seems like a good idea.

-- 
Peter Eisentraut                  Sernanders väg 10:115
peter_e@gmx.net                   75262 Uppsala
http://yi.org/peter-e/            Sweden