pgsql: Fix memory leak in pgbench

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.

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

On Wed, Apr 10, 2019 at 2:00 AM Alvaro Herrera <alvherre@alvh.no-ip.org> 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

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

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&gt; 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.

On April 9, 2019 11:07:38 PM PDT, Michael Paquier <michael@paquier.xyz> 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.

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.

On Wed, Apr 10, 2019 at 3:10 PM Fabien COELHO <coelho@cri.ensmp.fr> 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&gt; 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

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

On Wed, Apr 10, 2019 at 11:12 PM Michael Paquier <michael@paquier.xyz> 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

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

Thomas Munro <thomas.munro@gmail.com> 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

On Thu, Apr 11, 2019 at 1:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <thomas.munro@gmail.com> 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]https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=grouse&amp;dt=2019-04-10%2009%3A06%3A08&amp;stg=make and koreaceratops[2]https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=koreaceratops&amp;dt=2019-04-10%2007%3A59%3A42&amp;stg=make using gcc
4.4.7 (not recent though) complains this warning.

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

Regards,

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

On Thu, Apr 11, 2019 at 12:58 PM Alvaro Herrera
<alvherre@2ndquadrant.com> 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

Hello Tom,

Thomas Munro <thomas.munro@gmail.com> 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.

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

Michael Paquier <michael@paquier.xyz> 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

Hi,

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

Michael Paquier <michael@paquier.xyz> 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

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.