MIME-Version: 1.0
References: 
 <CAAZLFmQNJ0QNArpWEOZXwv=vbumcWKEHz-b1me5gBqRqG67EwQ@mail.gmail.com>
 <ab42G_tMNfBtULhZ@paquier.xyz>
In-Reply-To: <ab42G_tMNfBtULhZ@paquier.xyz>
From: Jianghua Yang <yjhjstz@gmail.com>
Date: Sat, 21 Mar 2026 06:09:46 -0700
Message-ID: 
 <CAAZLFmQeS4AFC6mNQNhyZWGUMXEkOmQA11UL28N-6c=CYtqnoA@mail.gmail.com>
Subject: Re: basebackup: add missing deflateEnd() in gzip compression sink
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@lists.postgresql.org
Content-Type: multipart/alternative; boundary="00000000000038f9dd064d8886f9"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAZLFmQeS4AFC6mNQNhyZWGUMXEkOmQA11UL28N-6c%3DCYtqnoA%40mail.gmail.com>
Precedence: bulk

--00000000000038f9dd064d8886f9
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

  Thanks for reviewing, Michael.

  > based on the argument of the permanent connection my take is that
  > this cleanup warrants a backpatch.

  Agreed. The current code leaks zlib internal state on every archive,
  and on a long-lived replication connection those allocations just
  pile up with no cleanup until the connection ends.

Michael Paquier <michael@paquier.xyz> =E4=BA=8E2026=E5=B9=B43=E6=9C=8820=E6=
=97=A5=E5=91=A8=E4=BA=94 23:09=E5=86=99=E9=81=93=EF=BC=9A

> On Fri, Mar 20, 2026 at 10:02:44AM -0700, Jianghua Yang wrote:
> > While reviewing the backup compression backends, I noticed that the gzi=
p
> > sink (basebackup_gzip.c) never calls deflateEnd(), unlike the lz4 and
> > zstd sinks which properly free their compression contexts.
> >
> > This means each archive (one per tablespace) leaks ~256KB of zlib
> > internal state until the backup's memory context is destroyed. On the
> > ERROR path, the zlib context is not released at all since there is no
> > gzip-specific cleanup callback.
>
> Yes, you are right on this one.  This code could be better in cleaning
> up the resources it is working on (and note that all the other
> gzip-related code paths call the end() part if doing an init() or an
> init2()).  Leaking that once per archive may not look like a big deal,
> but this is backend-side code we are talking about, and on a
> persistent replication connection it is not really cool to lose the
> context of this data with garbage coming from zlib piling up, so based
> on the argument of the permanent connection my take is that this
> cleanup warrants a backpatch.
> --
> Michael
>

--00000000000038f9dd064d8886f9
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">=C2=A0 Thanks for reviewing, Michael.<br><br>=C2=A0 &gt; b=
ased on the argument of the permanent connection my take is that<br>=C2=A0 =
&gt; this cleanup warrants a backpatch.<br><br>=C2=A0 Agreed. The current c=
ode leaks zlib internal state on every archive,<br>=C2=A0 and on a long-liv=
ed replication connection those allocations just<br>=C2=A0 pile up with no =
cleanup until the connection ends.</div><br><div class=3D"gmail_quote gmail=
_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">Michael Paquier &lt=
;<a href=3D"mailto:michael@paquier.xyz">michael@paquier.xyz</a>&gt; =E4=BA=
=8E2026=E5=B9=B43=E6=9C=8820=E6=97=A5=E5=91=A8=E4=BA=94 23:09=E5=86=99=E9=
=81=93=EF=BC=9A<br></div><blockquote class=3D"gmail_quote" style=3D"margin:=
0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">=
On Fri, Mar 20, 2026 at 10:02:44AM -0700, Jianghua Yang wrote:<br>
&gt; While reviewing the backup compression backends, I noticed that the gz=
ip<br>
&gt; sink (basebackup_gzip.c) never calls deflateEnd(), unlike the lz4 and<=
br>
&gt; zstd sinks which properly free their compression contexts.<br>
&gt; <br>
&gt; This means each archive (one per tablespace) leaks ~256KB of zlib<br>
&gt; internal state until the backup&#39;s memory context is destroyed. On =
the<br>
&gt; ERROR path, the zlib context is not released at all since there is no<=
br>
&gt; gzip-specific cleanup callback.<br>
<br>
Yes, you are right on this one.=C2=A0 This code could be better in cleaning=
<br>
up the resources it is working on (and note that all the other<br>
gzip-related code paths call the end() part if doing an init() or an<br>
init2()).=C2=A0 Leaking that once per archive may not look like a big deal,=
<br>
but this is backend-side code we are talking about, and on a<br>
persistent replication connection it is not really cool to lose the<br>
context of this data with garbage coming from zlib piling up, so based<br>
on the argument of the permanent connection my take is that this<br>
cleanup warrants a backpatch.<br>
--<br>
Michael<br>
</blockquote></div>

--00000000000038f9dd064d8886f9--