Thread: freefuncs.c is never called from anywhere!?
I was rather bemused to discover just now that the node-freeing
functions in nodes/freefuncs.c are never called from anywhere;
in fact, the module hasn't got a single exported entry point!
(I expect that freeObject() is supposed to be an external entry
point; perhaps it got demoted to a static during one of Bruce's
periodic get-rid-of-unreferenced-global-symbols passes.)
So much for all that tedious labor to maintain the freeXXX functions
every time we update a node type ;-)
Now I am not quite sure what to do. I was intending to use freeObject
to clean up rule qual/action trees during relcache flush --- up to now,
that cache data has been permanently leaked by any relcache flush
affecting a relation with rules. But if freefuncs.c hasn't actually
been used in a long time, it may well be suffering serious bit-rot.
I am worried about turning it on just before beta. I am especially
worried about turning it on for use only in a seldom-taken code path ---
if there are bugs in it, we may not find them until after release.
Should I chicken out and let the memory leak persist until we start
7.1 development cycle? Or go for it and hope the code's OK?
regards, tom lane
> I was rather bemused to discover just now that the node-freeing > functions in nodes/freefuncs.c are never called from anywhere; > in fact, the module hasn't got a single exported entry point! > > (I expect that freeObject() is supposed to be an external entry > point; perhaps it got demoted to a static during one of Bruce's > periodic get-rid-of-unreferenced-global-symbols passes.) > > So much for all that tedious labor to maintain the freeXXX functions > every time we update a node type ;-) Are you sure about this? I thought these things were called from macros. However, I can't find any macro that uses this like makeNode does with its pasteing. I would perhaps move it to a _deadcode directory and see what happens. Or should we 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, Pennsylvania19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> I was rather bemused to discover just now that the node-freeing
>> functions in nodes/freefuncs.c are never called from anywhere;
>> in fact, the module hasn't got a single exported entry point!
> Are you sure about this? I thought these things were called from
> macros.
nm --defined --extern shows that freefuncs.o exports no symbols.
Therefore it's impossible for any outside code to refer to it.
(Hmm, I wonder if any other modules are equally dead?)
> I would perhaps move it to a _deadcode directory and see
> what happens. Or should we enable it?
I certainly don't want to bit-bucket it --- I was just very surprised
that it's not currently being used.
After further consideration I realized that if RelationClearRelation
uses freeObject() to flush rules, then a DROP on a view will exercise
the code. So it's not quite as hard to test as I was thinking before.
I'm leaning towards the "go for it" answer at the moment...
regards, tom lane
> -----Original Message----- > From: owner-pgsql-hackers@postgreSQL.org > [mailto:owner-pgsql-hackers@postgreSQL.org]On Behalf Of Tom Lane > > I was rather bemused to discover just now that the node-freeing > functions in nodes/freefuncs.c are never called from anywhere; > in fact, the module hasn't got a single exported entry point! > > (I expect that freeObject() is supposed to be an external entry > point; perhaps it got demoted to a static during one of Bruce's > periodic get-rid-of-unreferenced-global-symbols passes.) > > So much for all that tedious labor to maintain the freeXXX functions > every time we update a node type ;-) > As far as I see,freeObject() has a fundamental problem. Probably it couldn't free multiple references safely. Regards. Hiroshi Inoue Inoue@tpf.co.jp
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> As far as I see,freeObject() has a fundamental problem.
> Probably it couldn't free multiple references safely.
Yes, that code will have a problem with node trees that have
circular references or multiple links to the same node.
Both of which are cases that we can currently generate :-(.
Of course, circular links will also break copyfuncs, equalfuncs,
and printfuncs. We have a known bug with the backend crashing
when -d is enabled for certain queries, because of infinite
recursion in printfuncs.
For the moment, the only thing I need the freefuncs code for is
to free nodetrees that have been created by stringToNode. AFAICT
that routine is incapable of creating circular links or multiple
links, so it should work.
Eventually it would be nice to have a better solution.
regards, tom lane
> -----Original Message----- > From: Tom Lane [mailto:tgl@sss.pgh.pa.us] > > "Hiroshi Inoue" <Inoue@tpf.co.jp> writes: > > As far as I see,freeObject() has a fundamental problem. > > Probably it couldn't free multiple references safely. > > Yes, that code will have a problem with node trees that have > circular references or multiple links to the same node. > Both of which are cases that we can currently generate :-(. > > Of course, circular links will also break copyfuncs, equalfuncs, > and printfuncs. We have a known bug with the backend crashing > when -d is enabled for certain queries, because of infinite > recursion in printfuncs. > Multiple links to the same node is not so fatal for other xxxxObject(). In fact they are used without big problem. But isn't it fatal for freeObject() ? Regards. Hiroshi Inoue Inoue@tpf.co.jp
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> Multiple links to the same node is not so fatal for other xxxxObject().
> In fact they are used without big problem.
> But isn't it fatal for freeObject() ?
Yes, of course. We could possibly fix that by maintaining a list of
already-freed nodes during any one freeObject() call, but that seems
painful and slow.
However, the case I'm interested in would only be trying to free
nodetrees that were created by either copyObject or stringToNode,
and since neither of those routines produce multiple or circular
links, it seems safe enough to use freeObject as-is for the purpose.
regards, tom lane
On Sat, 29 Jan 2000, Bruce Momjian wrote: > However, I can't find any macro that uses this like makeNode does with > its pasteing. I would perhaps move it to a _deadcode directory and see > what happens. Or should we enable it? Please no more _deadcode. Why do we have CVS? (In the same spirit it would also be nice to tag NOT_USED sections with a version number, so it could be yanked two or three releases past.) -- Peter Eisentraut Sernanders vaeg 10:115 peter_e@gmx.net 75262 Uppsala http://yi.org/peter-e/ Sweden
On Mon, 31 Jan 2000, Peter Eisentraut wrote:
> On Sat, 29 Jan 2000, Bruce Momjian wrote:
>
> > However, I can't find any macro that uses this like makeNode does with
> > its pasteing. I would perhaps move it to a _deadcode directory and see
> > what happens. Or should we enable it?
>
> Please no more _deadcode. Why do we have CVS?
>
> (In the same spirit it would also be nice to tag NOT_USED sections with a
> version number, so it could be yanked two or three releases past.)
Why not just yank it period? 'cvs diff' will show what was yanked, and
the log message could say just 'yanked NOT_USED code from source tree'...
Marc G. Fournier ICQ#7615664 IRC Nick: Scrappy
Systems Administrator @ hub.org
primary: scrappy@hub.org secondary: scrappy@{freebsd|postgresql}.org
On Mon, 31 Jan 2000, The Hermit Hacker wrote: > On Mon, 31 Jan 2000, Peter Eisentraut wrote: > > > On Sat, 29 Jan 2000, Bruce Momjian wrote: > > > > > However, I can't find any macro that uses this like makeNode does with > > > its pasteing. I would perhaps move it to a _deadcode directory and see > > > what happens. Or should we enable it? > > > > Please no more _deadcode. Why do we have CVS? > > > > (In the same spirit it would also be nice to tag NOT_USED sections with a > > version number, so it could be yanked two or three releases past.) > > Why not just yank it period? 'cvs diff' will show what was yanked, and > the log message could say just 'yanked NOT_USED code from source tree'... I *suspect* the purpose of some of these sections is that if it turns out somebody needed them for their application, we could just tell them to re-enable them there and there. If this is the case, then it has gone way past abuse already, though. Some parts of been NOT_USED for many years and most likely don't work anymore. -- Peter Eisentraut Sernanders vaeg 10:115 peter_e@gmx.net 75262 Uppsala http://yi.org/peter-e/ Sweden
On Mon, 31 Jan 2000, Peter Eisentraut wrote:
> On Mon, 31 Jan 2000, The Hermit Hacker wrote:
>
> > On Mon, 31 Jan 2000, Peter Eisentraut wrote:
> >
> > > On Sat, 29 Jan 2000, Bruce Momjian wrote:
> > >
> > > > However, I can't find any macro that uses this like makeNode does with
> > > > its pasteing. I would perhaps move it to a _deadcode directory and see
> > > > what happens. Or should we enable it?
> > >
> > > Please no more _deadcode. Why do we have CVS?
> > >
> > > (In the same spirit it would also be nice to tag NOT_USED sections with a
> > > version number, so it could be yanked two or three releases past.)
> >
> > Why not just yank it period? 'cvs diff' will show what was yanked, and
> > the log message could say just 'yanked NOT_USED code from source tree'...
>
> I *suspect* the purpose of some of these sections is that if it turns out
> somebody needed them for their application, we could just tell them to
> re-enable them there and there. If this is the case, then it has gone way
> past abuse already, though. Some parts of been NOT_USED for many years and
> most likely don't work anymore.
IMHO, if you want to take the time to do it, yank it. we have a cvs
history of everything we do, so it isn't as if its permanently lost, and,
as you say, with all the changes that have gone around some of the
NOT_USED code, they are most likely not even valid/working anymore :)
Makes for a good time to do clean up ... before beta and/or major release
...
Marc G. Fournier ICQ#7615664 IRC Nick: Scrappy
Systems Administrator @ hub.org
primary: scrappy@hub.org secondary: scrappy@{freebsd|postgresql}.org
Peter Eisentraut <e99re41@DoCS.UU.SE> writes:
>>>> Please no more _deadcode. Why do we have CVS?
> I *suspect* the purpose of some of these sections is that if it turns out
> somebody needed them for their application, we could just tell them to
> re-enable them there and there.
I think there are several different motivations, and it would be bad to
tar all the occurrences of NOT_USED with the same brush.
Some of them are indeed just code that was #ifdef'd out while testing
a replacement solution, and never physically removed. I think that's what
CVS is for; we should keep history in CVS versions, not in the current
sources.
Some of them are useful code that just happens not to be referenced
at the moment, but might be needed again at any time. (And who'd
think to go look in back CVS versions, when wondering "is there a
routine that ..."?)
Some of them are code that was disabled because it was broken, but with
hopes of fixing and re-enabling it someday. (The expensive-function
code in the optimizer seems to be an example of this.) The advantage
of leaving such code in current sources is that it has a better chance
of tracking global edits, such as routine or struct field renamings.
I know that I have usually ignored files in _deadcode directories when
making widespread changes, which means that those files drift further
and further away from any hope of future usefulness. This doesn't
happen quite as fast for code that's #ifdef NOT_USED but still present
in current source files.
I think it would be a bad idea to go around and yank out NOT_USED code
wholesale --- this should be looked at on a case-by-case basis, with
consideration of whether the code might be useful again in future.
But I think I might vote for pruning the _deadcode directories
wholesale. I suspect the files in them are suffering from
irrecoverable bit rot.
regards, tom lane
> I think it would be a bad idea to go around and yank out NOT_USED code > wholesale --- this should be looked at on a case-by-case basis, with > consideration of whether the code might be useful again in future. > > But I think I might vote for pruning the _deadcode directories > wholesale. I suspect the files in them are suffering from > irrecoverable bit rot. I disagree on _deadcode. While the code is rotted, it does implement some full functions that have no other history, like verion.c for versioning and xfunc for expensive functions. Yanking them means we can never know what they did. -- 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
> On Mon, 31 Jan 2000, Peter Eisentraut wrote: > > > On Sat, 29 Jan 2000, Bruce Momjian wrote: > > > > > However, I can't find any macro that uses this like makeNode does with > > > its pasteing. I would perhaps move it to a _deadcode directory and see > > > what happens. Or should we enable it? > > > > Please no more _deadcode. Why do we have CVS? > > > > (In the same spirit it would also be nice to tag NOT_USED sections with a > > version number, so it could be yanked two or three releases past.) > > Why not just yank it period? 'cvs diff' will show what was yanked, and > the log message could say just 'yanked NOT_USED code from source tree'... We will never think to look and find it again if we do that. -- 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
> On Sat, 29 Jan 2000, Bruce Momjian wrote: > > > However, I can't find any macro that uses this like makeNode does with > > its pasteing. I would perhaps move it to a _deadcode directory and see > > what happens. Or should we enable it? > > Please no more _deadcode. Why do we have CVS? No one looks in the old cvs versions. Better to have a separate directory. > > (In the same spirit it would also be nice to tag NOT_USED sections with a > version number, so it could be yanked two or three releases past.) Good idea, but most NOT_USED must be checked to see if there is anything valuable in there. If not, it can be removed. Age is not the issue. We have code from Berkeley in NOT_USED because sometimes we may need it. You can remove it when you understand why it was there, or what it is doing, and know we will not need it. We may be able to remove them all someday if some suggests that. They are usually quite old. -- 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
On Mon, 31 Jan 2000, Bruce Momjian wrote:
> > I think it would be a bad idea to go around and yank out NOT_USED code
> > wholesale --- this should be looked at on a case-by-case basis, with
> > consideration of whether the code might be useful again in future.
> >
> > But I think I might vote for pruning the _deadcode directories
> > wholesale. I suspect the files in them are suffering from
> > irrecoverable bit rot.
>
> I disagree on _deadcode. While the code is rotted, it does implement
> some full functions that have no other history, like verion.c for
> versioning and xfunc for expensive functions. Yanking them means we can
> never know what they did.
even 'yanked' code is still in the cvs repository ... is it something that
ppl actually refer to, or are just afraid of possibly needing/wanting to
go back and look at it? maybe just add a '.readme' file to a subdirectory
to note files that were removed or something like that?
Marc G. Fournier ICQ#7615664 IRC Nick: Scrappy
Systems Administrator @ hub.org
primary: scrappy@hub.org secondary: scrappy@{freebsd|postgresql}.org
On Mon, 31 Jan 2000, Bruce Momjian wrote:
> > On Sat, 29 Jan 2000, Bruce Momjian wrote:
> >
> > > However, I can't find any macro that uses this like makeNode does with
> > > its pasteing. I would perhaps move it to a _deadcode directory and see
> > > what happens. Or should we enable it?
> >
> > Please no more _deadcode. Why do we have CVS?
>
> No one looks in the old cvs versions. Better to have a separate
> directory.
Does anyone check the _deadcode directory? :) I never even remember its
there until you guys bring it up to move things into it *shrug*
Marc G. Fournier ICQ#7615664 IRC Nick: Scrappy
Systems Administrator @ hub.org
primary: scrappy@hub.org secondary: scrappy@{freebsd|postgresql}.org
> > No one looks in the old cvs versions. Better to have a separate > > directory. > > Does anyone check the _deadcode directory? :) I never even remember its > there until you guys bring it up to move things into it *shrug* We have removed some features like expensive functions, versioning and recipe that were moved there, and could be used if we need them again. -- 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
The Hermit Hacker <scrappy@hub.org> writes:
>> I disagree on _deadcode. While the code is rotted, it does implement
>> some full functions that have no other history, like verion.c for
>> versioning and xfunc for expensive functions. Yanking them means we can
>> never know what they did.
> even 'yanked' code is still in the cvs repository ...
Precisely. Seems to me we ought to think about our code maintenance
methods with knowledge that back versions will be available from CVS.
Keeping stuff in the current tree has some advantages if it's stuff
you think you might want again in the near term, but I think it's
the wrong way to deal with stuff that we're only keeping for historical
purposes. For example, if I wanted to try to understand the xfunc
code, I'd really have to go back to the last version where it worked;
the partially-patched files sitting in _deadcode would most likely be
more confusing than helpful...
regards, tom lane
On Mon, 31 Jan 2000, Tom Lane wrote:
> The Hermit Hacker <scrappy@hub.org> writes:
> >> I disagree on _deadcode. While the code is rotted, it does implement
> >> some full functions that have no other history, like verion.c for
> >> versioning and xfunc for expensive functions. Yanking them means we can
> >> never know what they did.
>
> > even 'yanked' code is still in the cvs repository ...
>
> Precisely. Seems to me we ought to think about our code maintenance
> methods with knowledge that back versions will be available from CVS.
> Keeping stuff in the current tree has some advantages if it's stuff
> you think you might want again in the near term, but I think it's
> the wrong way to deal with stuff that we're only keeping for historical
> purposes. For example, if I wanted to try to understand the xfunc
> code, I'd really have to go back to the last version where it worked;
> the partially-patched files sitting in _deadcode would most likely be
> more confusing than helpful...
have to agree here ... how much of the NOT_USED code is totally irrelevant
based on the changes around it?
Some sort of 'text log' of what is removed and date should be kept, if ppl
want to be able to go back ... basically, instead of 'moved to _deadcode',
just add a line to a text file stating 'function X removed on date Y' so
that ppl have a guide to look at the cvs repository with ...
Marc G. Fournier ICQ#7615664 IRC Nick: Scrappy
Systems Administrator @ hub.org
primary: scrappy@hub.org secondary: scrappy@{freebsd|postgresql}.org
> The Hermit Hacker <scrappy@hub.org> writes: > >> I disagree on _deadcode. While the code is rotted, it does implement > >> some full functions that have no other history, like verion.c for > >> versioning and xfunc for expensive functions. Yanking them means we can > >> never know what they did. > > > even 'yanked' code is still in the cvs repository ... > > Precisely. Seems to me we ought to think about our code maintenance > methods with knowledge that back versions will be available from CVS. > Keeping stuff in the current tree has some advantages if it's stuff > you think you might want again in the near term, but I think it's > the wrong way to deal with stuff that we're only keeping for historical > purposes. For example, if I wanted to try to understand the xfunc > code, I'd really have to go back to the last version where it worked; > the partially-patched files sitting in _deadcode would most likely be > more confusing than helpful... Personally, I am willing to yank it all now. I think we understand the code base well enough that someone who knows the code can go through and quickly identify the 90% of NOT_USED that is just junk and remove it. Tom Lane, you are the only person I know who can do this. Sorry. -- 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
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Personally, I am willing to yank it all now. I think we understand the
> code base well enough that someone who knows the code can go through and
> quickly identify the 90% of NOT_USED that is just junk and remove it.
> Tom Lane, you are the only person I know who can do this. Sorry.
Moi? There are parts I understand well, and other parts not.
In any case I don't have time now to make such a sweep.
What I'd suggest is that we all just modify our operating procedures
a little. Any time you have studied a bit of code long enough to be
pretty sure you understand how it works, feel free to cut out any
NOT_USED segments that you can't see any prospect of wanting again.
I've already been applying this philosophy to some extent in modules
that I've worked on, but I will try to do it on a regular basis from
here on. If we all do it, that should eliminate the useless stuff
over time.
Next question is what to do about the _deadcode subdirectories.
For the most part I don't even know what's in 'em ... do you?
regards, tom lane
Any status on this? > I was rather bemused to discover just now that the node-freeing > functions in nodes/freefuncs.c are never called from anywhere; > in fact, the module hasn't got a single exported entry point! > > (I expect that freeObject() is supposed to be an external entry > point; perhaps it got demoted to a static during one of Bruce's > periodic get-rid-of-unreferenced-global-symbols passes.) > > So much for all that tedious labor to maintain the freeXXX functions > every time we update a node type ;-) > > Now I am not quite sure what to do. I was intending to use freeObject > to clean up rule qual/action trees during relcache flush --- up to now, > that cache data has been permanently leaked by any relcache flush > affecting a relation with rules. But if freefuncs.c hasn't actually > been used in a long time, it may well be suffering serious bit-rot. > I am worried about turning it on just before beta. I am especially > worried about turning it on for use only in a seldom-taken code path --- > if there are bugs in it, we may not find them until after release. > > Should I chicken out and let the memory leak persist until we start > 7.1 development cycle? Or go for it and hope the code's OK? > > regards, tom lane > > ************ > -- 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
> On Mon, 31 Jan 2000, Tom Lane wrote: > > > The Hermit Hacker <scrappy@hub.org> writes: > > >> I disagree on _deadcode. While the code is rotted, it does implement > > >> some full functions that have no other history, like verion.c for > > >> versioning and xfunc for expensive functions. Yanking them means we can > > >> never know what they did. > > > > > even 'yanked' code is still in the cvs repository ... > > > > Precisely. Seems to me we ought to think about our code maintenance > > methods with knowledge that back versions will be available from CVS. > > Keeping stuff in the current tree has some advantages if it's stuff > > you think you might want again in the near term, but I think it's > > the wrong way to deal with stuff that we're only keeping for historical > > purposes. For example, if I wanted to try to understand the xfunc > > code, I'd really have to go back to the last version where it worked; > > the partially-patched files sitting in _deadcode would most likely be > > more confusing than helpful... > > have to agree here ... how much of the NOT_USED code is totally irrelevant > based on the changes around it? > > Some sort of 'text log' of what is removed and date should be kept, if ppl > want to be able to go back ... basically, instead of 'moved to _deadcode', > just add a line to a text file stating 'function X removed on date Y' so > that ppl have a guide to look at the cvs repository with ... I am thinking of going through the code and removing NOT_USED functions I know to be useless. OK? -- 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
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I am thinking of going through the code and removing NOT_USED functions
> I know to be useless. OK?
Don't see why not, though I'd recommend treading lightly. That unused-
functions script tends to pull out stuff that has been exported in the
expectation that someone would want it someday, but for one reason or
another it's not being called right at the moment. We don't really want
to delete that sort of code.
BTW, I've also been rather dubious about the automatic
demote-to-static-function script for the same reason --- when someone
comes along and needs function X, it's hard to tell whether X was
intended to be private or was intended to be public but got demoted by
the script. It's tough to maintain clear module APIs with tools like
that second-guessing the author's intentions.
regards, tom lane
> Any status on this?
Nothing done about it yet.
IIRC, some people were concerned about the fact that freeObject()
couldn't possibly cope with circular structures, multiply-linked
subexpressions, etc. I don't think that's a problem for my intended
use in the relcache --- the only structures I'll be freeing are ones
previously read in by the node-reading functions, and those aren't
going to have any surprises like that.
regards, tom lane
> BTW, I've also been rather dubious about the automatic > demote-to-static-function script for the same reason --- when someone > comes along and needs function X, it's hard to tell whether X was > intended to be private or was intended to be public but got demoted by > the script. It's tough to maintain clear module APIs with tools like > that second-guessing the author's intentions. Also, I don't touch the interfaces, only the backend code. There aren't many api stuff there, except spi, which I also don't touch. -- 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
On Fri, 9 Jun 2000, Tom Lane wrote:
> > Any status on this?
>
> Nothing done about it yet.
>
> IIRC, some people were concerned about the fact that freeObject()
> couldn't possibly cope with circular structures, multiply-linked
> subexpressions, etc. I don't think that's a problem for my intended
> use in the relcache --- the only structures I'll be freeing are ones
> previously read in by the node-reading functions, and those aren't
> going to have any surprises like that.
IMHO use separate memory context will better and more fast way than
freeObject(). I use this method in my suggested query cache and in the
SPI (in SPI_freeplan()) and it is very good (without potential leaks).
All in backend/nodes are (IMHO) very dificult keep up and recursion is
and not gratis feature too. (Homework: write PG in Fortran :-)
Karel
Karel Zak <zakkr@zf.jcu.cz> writes:
>> IIRC, some people were concerned about the fact that freeObject()
>> couldn't possibly cope with circular structures, multiply-linked
>> subexpressions, etc. I don't think that's a problem for my intended
>> use in the relcache --- the only structures I'll be freeing are ones
>> previously read in by the node-reading functions, and those aren't
>> going to have any surprises like that.
> IMHO use separate memory context will better and more fast way than
> freeObject().
A separate memory context for each relcache entry? I don't think so...
contexts aren't likely to be *that* cheap. Especially since I'd
probably need at least two contexts per relcache entry in order to do it
that way.
regards, tom lane
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> It's tough to maintain clear module APIs with tools like
>> that second-guessing the author's intentions.
> Also, I don't touch the interfaces, only the backend code. There aren't
> many api stuff there, except spi, which I also don't touch.
I was talking about intermodule APIs within the backend...
regards, tom lane
> Karel Zak <zakkr@zf.jcu.cz> writes: > >> IIRC, some people were concerned about the fact that freeObject() > >> couldn't possibly cope with circular structures, multiply-linked > >> subexpressions, etc. I don't think that's a problem for my intended > >> use in the relcache --- the only structures I'll be freeing are ones > >> previously read in by the node-reading functions, and those aren't > >> going to have any surprises like that. > > > IMHO use separate memory context will better and more fast way than > > freeObject(). > > A separate memory context for each relcache entry? I don't think so... No. I mean _common_ for manipulation with query/plan tree.
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> It's tough to maintain clear module APIs with tools like > >> that second-guessing the author's intentions. > > > Also, I don't touch the interfaces, only the backend code. There aren't > > many api stuff there, except spi, which I also don't touch. > > I was talking about intermodule APIs within the backend... Can you give an example? You mean a function that is meant to be exported to other modules in the backend, but is not called for some reason should not be marked as static? Yes, that is an issue, but reducing our code size and marking functions static/NOT_USED seems to be a larger benfit. Of course, IMHO. -- 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