pgsql: Fix memory leak in pgbench

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

pgsql: Fix memory leak in pgbench

Álvaro Herrera
Fix memory leak in pgbench

Commit 25ee70511ec2 introduced a memory leak in pgbench: some PGresult
structs were not being freed during error bailout, because we're now
doing more PQgetResult() calls than previously.  Since there's more
cleanup code outside the discard_response() routine than in it, refactor
the cleanup code, removing the routine.

This has little effect currently, since we abandon processing after
hitting errors, but if we ever get further pgbench features (such as
testing for serializable transactions), it'll matter.

Per Coverity.

Reviewed-by: Michaël Paquier

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/fe0e0b4fc7f0cdc2333bda08b199422a1e77a551

Modified Files
--------------
src/bin/pgbench/pgbench.c | 48 ++++++++++++++++++-----------------------------
1 file changed, 18 insertions(+), 30 deletions(-)

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Fabien COELHO-3

Hello Alvaro,

> Fix memory leak in pgbench

> Commit 25ee70511ec2 introduced a memory leak in pgbench: some PGresult
> structs were not being freed during error bailout, because we're now
> doing more PQgetResult() calls than previously.

Indeed, I did not consider cleaning up on errors when removing cset, and
if errors are handled somehow it would have caused a problem.

Thanks for the fix. I could have prepared a patch if told that there was
some problem, but it seems that pg coverity reports are private.

--
Fabien.


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Alvaro Herrera-9
Hello

On 2019-Apr-09, Fabien COELHO wrote:

> > Fix memory leak in pgbench
>
> > Commit 25ee70511ec2 introduced a memory leak in pgbench: some PGresult
> > structs were not being freed during error bailout, because we're now
> > doing more PQgetResult() calls than previously.
>
> Indeed, I did not consider cleaning up on errors when removing cset, and if
> errors are handled somehow it would have caused a problem.
>
> Thanks for the fix. I could have prepared a patch if told that there was
> some problem, but it seems that pg coverity reports are private.

They're indeed private.  In this case I could have opened it, since the
code is unreleased thus there's no security restriction, but since it's
so minor I didn't think it worthwhile.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Masahiko Sawada
In reply to this post by Álvaro Herrera
On Wed, Apr 10, 2019 at 2:00 AM Alvaro Herrera <[hidden email]> wrote:

>
> Fix memory leak in pgbench
>
> Commit 25ee70511ec2 introduced a memory leak in pgbench: some PGresult
> structs were not being freed during error bailout, because we're now
> doing more PQgetResult() calls than previously.  Since there's more
> cleanup code outside the discard_response() routine than in it, refactor
> the cleanup code, removing the routine.
>
> This has little effect currently, since we abandon processing after
> hitting errors, but if we ever get further pgbench features (such as
> testing for serializable transactions), it'll matter.
>
This change leads a compiler warning on my machine:

pgbench.c: In function ‘readCommandResponse’:
pgbench.c:2730: warning: ISO C90 forbids mixed declarations and code

@@ -2739,7 +2726,7 @@ readCommandResponse(CState *st, char *varprefix)
    while (res != NULL)
    {
        /* look now at the next result to know whether it is the last */
-       PGresult    *next_res = PQgetResult(st->con);
+       next_res = PQgetResult(st->con);
        bool is_last = (next_res == NULL);

I think we should declare is_last before doing next_res = PQgetResult(st->con).

Attached patch fixes it.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

pgbench.patch (676 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Michael Paquier-2
On Wed, Apr 10, 2019 at 01:19:11PM +0900, Masahiko Sawada wrote:
> This change leads a compiler warning on my machine:
>
> pgbench.c: In function ‘readCommandResponse’:
> pgbench.c:2730: warning: ISO C90 forbids mixed declarations and code

C99 does not forbid that (see d9dd406).
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Fabien COELHO-3
In reply to this post by Masahiko Sawada

Hello Masahiko-san,

> This change leads a compiler warning on my machine:
>
> pgbench.c: In function ‘readCommandResponse’:
> pgbench.c:2730: warning: ISO C90 forbids mixed declarations and code

<https://www.postgresql.org/docs/devel/source-conventions.html> says:

"Code in PostgreSQL should only rely on language features available in the
C99 standard"

So it should be all right wrt to warnings.

However the pg style in the same page says that intermingling decl & code
is not permitted, so the proposed patch should be applied.

--
Fabien.
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Andres Freund
In reply to this post by Michael Paquier-2


On April 9, 2019 11:07:38 PM PDT, Michael Paquier <[hidden email]> wrote:
>On Wed, Apr 10, 2019 at 01:19:11PM +0900, Masahiko Sawada wrote:
>> This change leads a compiler warning on my machine:
>>
>> pgbench.c: In function ‘readCommandResponse’:
>> pgbench.c:2730: warning: ISO C90 forbids mixed declarations and code
>
>C99 does not forbid that (see d9dd406).

Our own set of standards do however. Note that we still have wdeclaration-after-statement in our compiler flags if supported by the compiler.  CF configure.in.

Andres


--
Sent from my Android device with K-9 Mail. Please excuse my brevity.


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Fabien COELHO-3
In reply to this post by Michael Paquier-2

Bonjour Michaël,

>> This change leads a compiler warning on my machine:
>>
>> pgbench.c: In function ‘readCommandResponse’:
>> pgbench.c:2730: warning: ISO C90 forbids mixed declarations and code
>
> C99 does not forbid that (see d9dd406).

https://www.postgresql.org/docs/devel/source-conventions.html also says:

"A few features included in the C99 standard are, at this time, not be
permitted to be used in core PostgreSQL code. This currently includes
variable length arrays, intermingled declarations and code, // comments,
universal character names. Reasons for that include portability and
historical practices."

--
Fabien.
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Masahiko Sawada
In reply to this post by Fabien COELHO-3
On Wed, Apr 10, 2019 at 3:10 PM Fabien COELHO <[hidden email]> wrote:

>
>
> Hello Masahiko-san,
>
> > This change leads a compiler warning on my machine:
> >
> > pgbench.c: In function ‘readCommandResponse’:
> > pgbench.c:2730: warning: ISO C90 forbids mixed declarations and code
>
> <https://www.postgresql.org/docs/devel/source-conventions.html> says:
>
> "Code in PostgreSQL should only rely on language features available in the
> C99 standard"
>
> So it should be all right wrt to warnings.
>
> However the pg style in the same page says that intermingling decl & code
> is not permitted, so the proposed patch should be applied.

Thank you! I didn't know the detail of that PostgreSQL supports C99
standard but I hope the warning will be fixed and coming patches will
still obey that rule.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Michael Paquier-2
On Wed, Apr 10, 2019 at 04:05:34PM +0900, Masahiko Sawada wrote:
> Thank you! I didn't know the detail of that PostgreSQL supports C99
> standard but I hope the warning will be fixed and coming patches will
> still obey that rule.

Ah, thanks I missed that bit from the docs.  I should have paid more
attention.  I am sure that Álvaro will address your patch in a timely
manner.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Thomas Munro-5
On Wed, Apr 10, 2019 at 11:12 PM Michael Paquier <[hidden email]> wrote:
> On Wed, Apr 10, 2019 at 04:05:34PM +0900, Masahiko Sawada wrote:
> > Thank you! I didn't know the detail of that PostgreSQL supports C99
> > standard but I hope the warning will be fixed and coming patches will
> > still obey that rule.
>
> Ah, thanks I missed that bit from the docs.  I should have paid more
> attention.  I am sure that Álvaro will address your patch in a timely
> manner.

. o O ( Is it time to run with -Werror on some BF animals yet? )

--
Thomas Munro
https://enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Alvaro Herrera-9
In reply to this post by Masahiko Sawada
On 2019-Apr-10, Masahiko Sawada wrote:

> This change leads a compiler warning on my machine:
>
> pgbench.c: In function ‘readCommandResponse’:
> pgbench.c:2730: warning: ISO C90 forbids mixed declarations and code

> I think we should declare is_last before doing next_res = PQgetResult(st->con).
>
> Attached patch fixes it.

Thanks, pushed.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Tom Lane-2
In reply to this post by Thomas Munro-5
Thomas Munro <[hidden email]> writes:
> . o O ( Is it time to run with -Werror on some BF animals yet? )

I've got -Werror turned on on longfin; I'm surprised that it's not
whining about this.  Perhaps -Wdeclaration-after-statement doesn't
really do anything on clang?

Anyway, I'd be in favor of having at least one reasonably-recent-gcc
animal running with -Werror.  gcc and clang seem to have rather
different sets of warnings.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Masahiko Sawada
On Thu, Apr 11, 2019 at 1:54 PM Tom Lane <[hidden email]> wrote:

>
> Thomas Munro <[hidden email]> writes:
> > . o O ( Is it time to run with -Werror on some BF animals yet? )
>
> I've got -Werror turned on on longfin; I'm surprised that it's not
> whining about this.  Perhaps -Wdeclaration-after-statement doesn't
> really do anything on clang?
>
> Anyway, I'd be in favor of having at least one reasonably-recent-gcc
> animal running with -Werror.  gcc and clang seem to have rather
> different sets of warnings.

+1

FWIW, I've found at least grouse[1] and koreaceratops[2] using gcc
4.4.7 (not recent though) complains this warning.

[1] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=grouse&dt=2019-04-10%2009%3A06%3A08&stg=make
[2] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=koreaceratops&dt=2019-04-10%2007%3A59%3A42&stg=make

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Masahiko Sawada
In reply to this post by Alvaro Herrera-9
On Thu, Apr 11, 2019 at 12:58 PM Alvaro Herrera
<[hidden email]> wrote:

>
> On 2019-Apr-10, Masahiko Sawada wrote:
>
> > This change leads a compiler warning on my machine:
> >
> > pgbench.c: In function ‘readCommandResponse’:
> > pgbench.c:2730: warning: ISO C90 forbids mixed declarations and code
>
> > I think we should declare is_last before doing next_res = PQgetResult(st->con).
> >
> > Attached patch fixes it.
>
> Thanks, pushed.
>

Thank you!

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Fabien COELHO-3
In reply to this post by Tom Lane-2

Hello Tom,

> Thomas Munro <[hidden email]> writes:
>> . o O ( Is it time to run with -Werror on some BF animals yet? )
>
> I've got -Werror turned on on longfin; I'm surprised that it's not
> whining about this.  Perhaps -Wdeclaration-after-statement doesn't
> really do anything on clang?
>
> Anyway, I'd be in favor of having at least one reasonably-recent-gcc
> animal running with -Werror.  gcc and clang seem to have rather
> different sets of warnings.

Yep. Hopefully non contradictory:-)

I can set up farm animals with bleeding age clang/gcc (compiled weekly
from sources) with -Werror (seawasp & moonjelly are already running with
those but without -Werror).

However, I'm not sure how motivated the project is with keeping up with
the latest warnings, there could be some instability, it may require some
adjustements, but they are a glimpse of what is to come, so maybe a less
bleeding edge set of compilers should be selected.

--
Fabien.


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Michael Paquier-2
On Thu, Apr 11, 2019 at 09:03:19AM +0200, Fabien COELHO wrote:
> I can set up farm animals with bleeding age clang/gcc (compiled weekly from
> sources) with -Werror (seawasp & moonjelly are already running with those
> but without -Werror).

If possible, I would not recommend using compiled versions from
source, but just the latest stable versions released for gcc and
clang.  Even if this can catch up compiler bugs using Postgres code at
a very early stage, the false positives induced have been proving to
hurt because of the time investigations took to determine if this was
a problem on our side or not.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> On Thu, Apr 11, 2019 at 09:03:19AM +0200, Fabien COELHO wrote:
>> I can set up farm animals with bleeding age clang/gcc (compiled weekly from
>> sources) with -Werror (seawasp & moonjelly are already running with those
>> but without -Werror).

> If possible, I would not recommend using compiled versions from
> source, but just the latest stable versions released for gcc and
> clang.  Even if this can catch up compiler bugs using Postgres code at
> a very early stage, the false positives induced have been proving to
> hurt because of the time investigations took to determine if this was
> a problem on our side or not.

Yeah, I'm not excited about having to expend effort on working around
warnings seen only in unstable compilers.  I was thinking more of
using some solid-but-not-ancient gcc release.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Andres Freund
Hi,

On 2019-04-11 09:35:23 -0400, Tom Lane wrote:

> Michael Paquier <[hidden email]> writes:
> > On Thu, Apr 11, 2019 at 09:03:19AM +0200, Fabien COELHO wrote:
> >> I can set up farm animals with bleeding age clang/gcc (compiled weekly from
> >> sources) with -Werror (seawasp & moonjelly are already running with those
> >> but without -Werror).
>
> > If possible, I would not recommend using compiled versions from
> > source, but just the latest stable versions released for gcc and
> > clang.  Even if this can catch up compiler bugs using Postgres code at
> > a very early stage, the false positives induced have been proving to
> > hurt because of the time investigations took to determine if this was
> > a problem on our side or not.
>
> Yeah, I'm not excited about having to expend effort on working around
> warnings seen only in unstable compilers.  I was thinking more of
> using some solid-but-not-ancient gcc release.

I could easily add that to one of EXEC_BACKEND or -DDCOPY_PARSE_PLAN_TREES
-DRAW_EXPRESSION_COVERAGE_TEST, --disable-atomics, --disable-spinlocks
animals. I'd probably go for one of the latter, because they tend to
break very rarely for other reasons these days.

While the buildfarm page says they run gcc 6, it's actually just the
latest stable release, updated regularly.

I kinda wish the buildfarm wouldn't require manually updating compiler
versions...

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Fix memory leak in pgbench

Fabien COELHO-3

> I kinda wish the buildfarm wouldn't require manually updating compiler
> versions...

It does not. My cron script uses the update script to keep them up to
date:

  ./update_personality.pl --config=$PG/moonjelly.conf --compiler-version="$(gcc --version | head -1 | sed 's/gcc (GCC) //')"
  ./update_personality.pl --config=$PG/seawasp.conf --compiler-version="$(clang --version | head -1 | sed 's/^clang version //')"

--
Fabien.


12