Thread: pgsql: Fix memory leak in pgbench
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(-)
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
Attachment
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
Attachment
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.
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 supportedby 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> 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
Attachment
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] 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
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
Attachment
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.
>> 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. Yep, but I'm a compiler person, so I'm happy with compiling bleeding edge pg with bleeding edge gcc & clang. Note that there is also "caiman" running a non release gcc version. > Even if this can catch up compiler bugs using Postgres code at > a very early stage, Indeed, several times, and they were reported and fixed. > 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. ISTM that the answer is that they are experimental animals, hence the choice of stingy jellyfish names, and failures should not be investigated immediately. Maybe some "experimental" warning could be shown somewhere as a reminder? I can add it in the compiler version, something like: "Ubuntu 18.04.2 LTS **EXPERIMENTAL** gcc ..." -- Fabien.
Fabien COELHO <coelho@cri.ensmp.fr> writes:
> Maybe some "experimental" warning could be shown somewhere as a reminder?
> I can add it in the compiler version, something like:
> "Ubuntu 18.04.2 LTS **EXPERIMENTAL** gcc ..."
+1. Also you might consider marking that in a "note" for the
animal.
regards, tom lane
Hello Tom, >> I can add it in the compiler version, something like: >> "Ubuntu 18.04.2 LTS **EXPERIMENTAL** gcc ..." > > +1. Done, should be seen on next run. The list of compilers is accumulating on the "Members" page for caiman, handfish, moonjelly and seawasp. Not sure how useful it is to keep an extended history for quickly moving targets. I'd be interested in running corresponding -Werror animals, which would be likely to be in the red, if it would be acceptable with some **EXPERIMENTAL** or even **IGNORE** warning. Hmmm, probably people like to see an all green status page. > Also you might consider marking that in a "note" for the animal. I have, but have not found where to put an animal note. -- Fabien.