Thread: [PATCH] ecpg: fix progname memory leak
Hi, Fix progname memory leak in ecpg client. Issue taken from todo list https://wiki.postgresql.org/wiki/Todo. Best regards, Maksim Kita
Attachment
On Wed, Oct 07, 2020 at 02:31:54PM +0300, Maksim Kita wrote: > Fix progname memory leak in ecpg client. > Issue taken from todo list https://wiki.postgresql.org/wiki/Todo. FWIW, I don't see much point in doing that. For one, we have a more-or-less established rule that progname remains set until the application leaves, and there are much more places where we leak memory like that. As one example, just see the various places where we use pg_strdup for option parsing. At the end, it does not really matter as these are applications running for a short amount of time. -- Michael
Attachment
On Thu, Oct 8, 2020 at 10:35:41AM +0900, Michael Paquier wrote: > On Wed, Oct 07, 2020 at 02:31:54PM +0300, Maksim Kita wrote: > > Fix progname memory leak in ecpg client. > > Issue taken from todo list https://wiki.postgresql.org/wiki/Todo. > > FWIW, I don't see much point in doing that. For one, we have a > more-or-less established rule that progname remains set until the > application leaves, and there are much more places where we leak > memory like that. As one example, just see the various places where > we use pg_strdup for option parsing. At the end, it does not really > matter as these are applications running for a short amount of time. Agreed, but what does the TODO item mean then? Fix small memory leaks in ecpg Memory leaks in a short running application like ecpg are not really a problem, but make debugging more complicated Should it be removed? -- Bruce Momjian <bruce@momjian.us> https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
On Thu, 2020-10-08 at 12:27 -0400, Bruce Momjian wrote: > On Thu, Oct 8, 2020 at 10:35:41AM +0900, Michael Paquier wrote: > > On Wed, Oct 07, 2020 at 02:31:54PM +0300, Maksim Kita wrote: > > > Fix progname memory leak in ecpg client. > > > Issue taken from todo list https://wiki.postgresql.org/wiki/Todo. > > > > FWIW, I don't see much point in doing that. For one, we have a > > more-or-less established rule that progname remains set until the > > application leaves, and there are much more places where we leak > > memory like that. As one example, just see the various places > > where > > we use pg_strdup for option parsing. At the end, it does not > > really > > matter as these are applications running for a short amount of > > time. > > Agreed, but what does the TODO item mean then? > > Fix small memory leaks in ecpg > Memory leaks in a short running application like ecpg are > not really > a problem, but make debugging more complicated > > Should it be removed? I'd say yes, let's remove it. Actually I wasn't even aware it's on there. While I agree that it makes debugging of memory handling in ecpg more difficult, I don't see much of a point in it. Michael -- Michael Meskes Michael at Fam-Meskes dot De Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org
On Thu, Oct 8, 2020 at 06:58:14PM +0200, Michael Meskes wrote: > On Thu, 2020-10-08 at 12:27 -0400, Bruce Momjian wrote: > > On Thu, Oct 8, 2020 at 10:35:41AM +0900, Michael Paquier wrote: > > > On Wed, Oct 07, 2020 at 02:31:54PM +0300, Maksim Kita wrote: > > > > Fix progname memory leak in ecpg client. > > > > Issue taken from todo list https://wiki.postgresql.org/wiki/Todo. > > > > > > FWIW, I don't see much point in doing that. For one, we have a > > > more-or-less established rule that progname remains set until the > > > application leaves, and there are much more places where we leak > > > memory like that. As one example, just see the various places > > > where > > > we use pg_strdup for option parsing. At the end, it does not > > > really > > > matter as these are applications running for a short amount of > > > time. > > > > Agreed, but what does the TODO item mean then? > > > > Fix small memory leaks in ecpg > > Memory leaks in a short running application like ecpg are > > not really > > a problem, but make debugging more complicated > > > > Should it be removed? > > I'd say yes, let's remove it. Actually I wasn't even aware it's on > there. While I agree that it makes debugging of memory handling in ecpg > more difficult, I don't see much of a point in it. OK, TODO removed. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
On Wed, Oct 7, 2020 at 6:35 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Oct 07, 2020 at 02:31:54PM +0300, Maksim Kita wrote:
> Fix progname memory leak in ecpg client.
> Issue taken from todo list https://wiki.postgresql.org/wiki/Todo.
FWIW, I don't see much point in doing that.
I hope that everyone takes 10 seconds and realizes that this appears to be this person's first submitted patch. I would think a little more respect for the attempt to patch a minor issue would be afforded to such a person. Seems technically sound and they are not trying to change the world with their first attempt.
Maybe a reminder that the TODO list is not always spectacular and that a double check with the list before doing something might be in order (in fact adding that to the top of the TODO list might be a great option here as well).
It's not going to win a Turing award - but I thought this project was a little more friendly then what I've seen in this thread towards a first time contributor.
John
On Thu, Oct 8, 2020 at 10:13:53AM -0700, John W Higgins wrote: > > On Wed, Oct 7, 2020 at 6:35 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Oct 07, 2020 at 02:31:54PM +0300, Maksim Kita wrote: > > Fix progname memory leak in ecpg client. > > Issue taken from todo list https://wiki.postgresql.org/wiki/Todo. > > FWIW, I don't see much point in doing that. > > > I hope that everyone takes 10 seconds and realizes that this appears to be this > person's first submitted patch. I would think a little more respect for the > attempt to patch a minor issue would be afforded to such a person. Seems > technically sound and they are not trying to change the world with their first > attempt. > > Maybe a reminder that the TODO list is not always spectacular and that a double > check with the list before doing something might be in order (in fact adding > that to the top of the TODO list might be a great option here as well). > > It's not going to win a Turing award - but I thought this project was a little > more friendly then what I've seen in this thread towards a first time > contributor. You mean like the warning at the top of the TODO list? https://wiki.postgresql.org/wiki/Todo#Development_Process WARNING for Developers: Unfortunately this list does not contain all the information necessary for someone to start coding a feature. Some of these items might have become unnecessary since they were added --- others might be desirable but the implementation might be unclear. When selecting items listed below, be prepared to first discuss the value of the feature. Do not assume that you can select one, code it and then expect it to be committed. Always discuss design on Hackers list before starting to code. The flow should be: Desirability -> Design -> Implement -> Test -> Review -> Commit -- Bruce Momjian <bruce@momjian.us> https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Michael Meskes <meskes@postgresql.org> writes:
> On Thu, 2020-10-08 at 12:27 -0400, Bruce Momjian wrote:
>> Agreed, but what does the TODO item mean then?
>>
>> Fix small memory leaks in ecpg
>> Memory leaks in a short running application like ecpg are not really
>> a problem, but make debugging more complicated
>>
>> Should it be removed?
> I'd say yes, let's remove it. Actually I wasn't even aware it's on
> there. While I agree that it makes debugging of memory handling in ecpg
> more difficult, I don't see much of a point in it.
Agreed. In theory there might be some point in removing leaks that happen
per-statement, so as to avoid unreasonable memory bloat when processing an
extremely long input file. In practice nobody has complained about that,
and if somebody did I'd probably question the sanity of putting so much
code into one file. (The C compiler would likely bloat even more while
processing the output...) Moreover, given the way the ecpg grammar works,
it'd be really painful to avoid all such leaks, and it might well
introduce bugs not fix them.
In any case, if this TODO item is going to lead to ideas as dubious
as "let's free progname before exiting", it's not helpful.
regards, tom lane
On Thu, Oct 8, 2020 at 10:13:53AM -0700, John W Higgins wrote:
>It's not going to win a Turing award - but I thought this project was a
>little more friendly then what I've seen in this thread towards a first
>time contributor.
Instead, it is unfriendly.
It takes a lot of motivation to "try" to submit a patch.
Good lucky Maksim Kita.
Thanks for the support John
regards,
Ranier Vilela
> On 8 Oct 2020, at 19:08, Bruce Momjian <bruce@momjian.us> wrote: > OK, TODO removed. Potentially controversial proposal: Should we perhaps remove (for some value of) the TODO list altogether? The value of the list is questionable as it's not actually a TODO list for established developers, and for aspiring new developers it's unlikely to give good ideas for where to start (I know there's a big warning but not everyone will read everything, the page is quite long). Now, I don't actually suggest we *remove* it, as there is valuable curated content, but that we rename it to something which won't encourage newcomers to pick something from the list simply because it exists. The name "TODO" implies that the list is something which it isn't. Thoughts? cheers ./daniel
On Fri, Oct 9, 2020 at 4:47 AM Daniel Gustafsson <daniel@yesql.se> wrote:
Now, I don't actually suggest we *remove* it, as there is valuable curated
content, but that we rename it to something which won't encourage newcomers to
pick something from the list simply because it exists. The name "TODO" implies
that the list is something which it isn't.
+1
The name is very much at odds with the content. To pick one baffling example, there's an entry under Free Space Map that dates to before the on-disk FSM was invented. A more accurate, if not charitable, description is "archive of stalled discussions". This is just a side effect of non-controversial todos getting finished over time. That could be helped with a round of aggressive culling.
Also, it's organized by functionality, which makes sense in a way, but I don't find very useful in this context. Better categories might be
help-wanted,
good-first-issue (I know this is a tough one),
feature-request,
won't-fix (The memory leaks under discussion go here, it seems),
code-cleanup,
research-project,
documentation,
performance,
user-experience,
sql-standard
etc.
I suspect we will eventually want something like a full-blown issue tracker, although I gather that's been rejected previously. But a wiki page is not really suited for this.
I've found that a better source of actual "todo"s from developers can be found in some commit messages, by grepping for "left for future work" or "for another day". I've gotten patch ideas (and more than one committed) from these. If I were asked, that's where I'd point aspiring developers.
On Fri, Oct 9, 2020 at 1:08 PM John Naylor <john.naylor@enterprisedb.com> wrote:
On Fri, Oct 9, 2020 at 4:47 AM Daniel Gustafsson <daniel@yesql.se> wrote:Now, I don't actually suggest we *remove* it, as there is valuable curated
content, but that we rename it to something which won't encourage newcomers to
pick something from the list simply because it exists. The name "TODO" implies
that the list is something which it isn't.+1
+1 as well. The way things are today, that is a terrible name, and has been for a double-digit number of years.
The name is very much at odds with the content. To pick one baffling example, there's an entry under Free Space Map that dates to before the on-disk FSM was invented. A more accurate, if not charitable, description is "archive of stalled discussions". This is just a side effect of non-controversial todos getting finished over time. That could be helped with a round of aggressive culling.
That is one very good example indeed.
Also, it's organized by functionality, which makes sense in a way, but I don't find very useful in this context. Better categories might behelp-wanted,good-first-issue (I know this is a tough one),feature-request,won't-fix (The memory leaks under discussion go here, it seems),code-cleanup,research-project,documentation,performance,user-experience,sql-standardetc.I suspect we will eventually want something like a full-blown issue tracker, although I gather that's been rejected previously. But a wiki page is not really suited for this.
Talk about "possibly controversial proposal" eh?
That said, having this in a different format would in no way fix the fact that the information is mislabeled and obsolete. It would likely be equally mislabeled and obsolete in an issue tracker. Just look at almost any of the other projects out there that *do* use an issue tracker and have been around for 20+ years.
That said, I am a believer in that we should have one, yes. But I am equally certain that it would not help with *this* particular problem.
Magnus Hagander <magnus@hagander.net> writes:
> That said, having this in a different format would in no way fix the fact
> that the information is mislabeled and obsolete. It would likely be
> equally mislabeled and obsolete in an issue tracker. Just look at almost
> any of the other projects out there that *do* use an issue tracker and have
> been around for 20+ years.
The long and the short of it is that a list like this is only of value
if it gets diligently maintained. Putting the data in a different tool
changes nothing about that. (A tool might reduce the effort involved,
*if* it's a good match to your workflow. But it won't reduce the effort
to zero.)
Consulting the wiki page's edit history shows that Bruce has been
doing some maintenance work, but almost nobody else does. Evidently
that's not enough.
I'd be +1 for the "aggressive culling" suggested upthread, but
I'm not particularly volunteering to do it, either.
regards, tom lane
On Fri, Oct 9, 2020 at 12:50:36PM -0400, Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: > > That said, having this in a different format would in no way fix the fact > > that the information is mislabeled and obsolete. It would likely be > > equally mislabeled and obsolete in an issue tracker. Just look at almost > > any of the other projects out there that *do* use an issue tracker and have > > been around for 20+ years. > > The long and the short of it is that a list like this is only of value > if it gets diligently maintained. Putting the data in a different tool > changes nothing about that. (A tool might reduce the effort involved, > *if* it's a good match to your workflow. But it won't reduce the effort > to zero.) > > Consulting the wiki page's edit history shows that Bruce has been > doing some maintenance work, but almost nobody else does. Evidently > that's not enough. > > I'd be +1 for the "aggressive culling" suggested upthread, but > I'm not particularly volunteering to do it, either. +1 -- Bruce Momjian <bruce@momjian.us> https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
On Fri, Oct 9, 2020 at 12:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Consulting the wiki page's edit history shows that Bruce has been
doing some maintenance work, but almost nobody else does. Evidently
that's not enough.
I'd be +1 for the "aggressive culling" suggested upthread, but
I'm not particularly volunteering to do it, either.
I'll start a separate thread for this. Working on one or a small number of sections at a time should be manageable.