Date: Thu, 16 Mar 2023 17:58:42 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: gkokolatos@pm.me, shiy.fnst@fujitsu.com,
	Michael Paquier <michael@paquier.xyz>,
	pgsql-hackers@lists.postgresql.org,
	Rachel Heaton <rachelmheaton@gmail.com>
Subject: Re: Add LZ4 compression in pg_dump
Message-ID: <ZBOfIs5AIiM7EDV6@telsasoft.com>
References: 
 <mYBKRYonRy5OiDDYxcxOOJ5xM9iLnuDsGP92zC7mjw8ew1wiccYZIpTvQEmtgRoAXRhaj91SAKwtFluUtRDZ3r6pRdGYLncWK7rXIQrHhFY=@pm.me>
 <22cb80ed-ec87-54c7-fd5b-cdcf79766bb1@enterprisedb.com>
 <8421f53d-c9f7-c02e-3278-9f9522a8e8f3@enterprisedb.com>
 <20230228235834.GC30529@telsasoft.com>
 <lsZgBfZRB5w5slcXnKwoL9qgpzdlAC_UwYHXfbj1oioWsNkckwr2BcLvvCi9-x7eV261s8AP5OHwROZ-QYtpeixxt73DQ_d6-LNfghYOaIQ=@pm.me>
 <b0ced9ea-e92d-f9d1-7a2f-075881b755b2@enterprisedb.com>
 <ZAfosKXCewHVENaZ@telsasoft.com>
 <c1493de3-7d97-c3c9-dd39-5a781e10a7b8@enterprisedb.com>
 <ZBJg6ylg17Yrj4Bm@telsasoft.com>
 <fc130e92-09ea-c5b0-a1a0-29f93c323371@enterprisedb.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <fc130e92-09ea-c5b0-a1a0-29f93c323371@enterprisedb.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZBOfIs5AIiM7EDV6%40telsasoft.com>
Precedence: bulk

On Thu, Mar 16, 2023 at 11:30:50PM +0100, Tomas Vondra wrote:
> On 3/16/23 01:20, Justin Pryzby wrote:
> > But try reading the diff while looking for the cause of a bug.  It's the
> > difference between reading 50, two-line changes, and reading a hunk that
> > replaces 100 lines with a different 100 lines, with empty/unrelated
> > lines randomly thrown in as context.
>
> I don't know, maybe I'm doing something wrong or maybe I just am bad at
> looking at diffs, but if I apply the patch you submitted on 8/3 and do
> the git-diff above (output attached), it seems pretty incomprehensible
> to me :-( I don't see 50 two-line changes (I certainly wouldn't be able
> to identify the root cause of the bug based on that).

It's true that most of the diff is still incomprehensible...

But look at the part relevant to the "empty-data" bug:

[... incomprehensible changes elided ...]
>  static void
> -InitCompressorZlib(CompressorState *cs, int level)
> +DeflateCompressorInit(CompressorState *cs)
>  {
> +	GzipCompressorState *gzipcs;
>  	z_streamp	zp;
>  
> -	zp = cs->zp = (z_streamp) pg_malloc(sizeof(z_stream));
> +	gzipcs = (GzipCompressorState *) pg_malloc0(sizeof(GzipCompressorState));
> +	zp = gzipcs->zp = (z_streamp) pg_malloc(sizeof(z_stream));
>  	zp->zalloc = Z_NULL;
>  	zp->zfree = Z_NULL;
>  	zp->opaque = Z_NULL;
>  
>  	/*
> -	 * zlibOutSize is the buffer size we tell zlib it can output to.  We
> -	 * actually allocate one extra byte because some routines want to append a
> -	 * trailing zero byte to the zlib output.
> +	 * outsize is the buffer size we tell zlib it can output to.  We actually
> +	 * allocate one extra byte because some routines want to append a trailing
> +	 * zero byte to the zlib output.
>  	 */
> -	cs->zlibOut = (char *) pg_malloc(ZLIB_OUT_SIZE + 1);
> -	cs->zlibOutSize = ZLIB_OUT_SIZE;
> +	gzipcs->outbuf = pg_malloc(ZLIB_OUT_SIZE + 1);
> +	gzipcs->outsize = ZLIB_OUT_SIZE;
>  
> -	if (deflateInit(zp, level) != Z_OK)
> -		pg_fatal("could not initialize compression library: %s",
> -				 zp->msg);
> +	/* -Z 0 uses the "None" compressor -- not zlib with no compression */
> +	Assert(cs->compression_spec.level != 0);
> +
> +	if (deflateInit(zp, cs->compression_spec.level) != Z_OK)
> +		pg_fatal("could not initialize compression library: %s", zp->msg);
>  
>  	/* Just be paranoid - maybe End is called after Start, with no Write */
> -	zp->next_out = (void *) cs->zlibOut;
> -	zp->avail_out = cs->zlibOutSize;
> +	zp->next_out = gzipcs->outbuf;
> +	zp->avail_out = gzipcs->outsize;
> +
> +	/* Keep track of gzipcs */
> +	cs->private_data = gzipcs;
>  }
>  
>  static void
> -EndCompressorZlib(ArchiveHandle *AH, CompressorState *cs)
> +DeflateCompressorEnd(ArchiveHandle *AH, CompressorState *cs)
>  {
> -	z_streamp	zp = cs->zp;
> +	GzipCompressorState *gzipcs = (GzipCompressorState *) cs->private_data;
> +	z_streamp	zp;
>  
> +	zp = gzipcs->zp;
>  	zp->next_in = NULL;
>  	zp->avail_in = 0;
>  
>  	/* Flush any remaining data from zlib buffer */
> -	DeflateCompressorZlib(AH, cs, true);
> +	DeflateCompressorCommon(AH, cs, true);
>  
>  	if (deflateEnd(zp) != Z_OK)
>  		pg_fatal("could not close compression stream: %s", zp->msg);
>  
> -	free(cs->zlibOut);
> -	free(cs->zp);
> +	pg_free(gzipcs->outbuf);
> +	pg_free(gzipcs->zp);
> +	pg_free(gzipcs);
> +	cs->private_data = NULL;
>  }
>  
>  static void
> -DeflateCompressorZlib(ArchiveHandle *AH, CompressorState *cs, bool flush)
> +DeflateCompressorCommon(ArchiveHandle *AH, CompressorState *cs, bool flush)
>  {
> -	z_streamp	zp = cs->zp;
> -	char	   *out = cs->zlibOut;
> +	GzipCompressorState *gzipcs = (GzipCompressorState *) cs->private_data;
> +	z_streamp	zp = gzipcs->zp;
> +	void	   *out = gzipcs->outbuf;
>  	int			res = Z_OK;
>  
> -	while (cs->zp->avail_in != 0 || flush)
> +	while (gzipcs->zp->avail_in != 0 || flush)
>  	{
>  		res = deflate(zp, flush ? Z_FINISH : Z_NO_FLUSH);
>  		if (res == Z_STREAM_ERROR)
>  			pg_fatal("could not compress data: %s", zp->msg);
> -		if ((flush && (zp->avail_out < cs->zlibOutSize))
> +		if ((flush && (zp->avail_out < gzipcs->outsize))
>  			|| (zp->avail_out == 0)
>  			|| (zp->avail_in != 0)
>  			)
> @@ -289,18 +122,18 @@ DeflateCompressorZlib(ArchiveHandle *AH, CompressorState *cs, bool flush)
>  			 * chunk is the EOF marker in the custom format. This should never
>  			 * happen but ...
>  			 */
> -			if (zp->avail_out < cs->zlibOutSize)
> +			if (zp->avail_out < gzipcs->outsize)
>  			{
>  				/*
>  				 * Any write function should do its own error checking but to
>  				 * make sure we do a check here as well ...
>  				 */
> -				size_t		len = cs->zlibOutSize - zp->avail_out;
> +				size_t		len = gzipcs->outsize - zp->avail_out;
>  
> -				cs->writeF(AH, out, len);
> +				cs->writeF(AH, (char *) out, len);
>  			}
> -			zp->next_out = (void *) out;
> -			zp->avail_out = cs->zlibOutSize;
> +			zp->next_out = out;
> +			zp->avail_out = gzipcs->outsize;
>  		}
>  
>  		if (res == Z_STREAM_END)
> @@ -309,16 +142,26 @@ DeflateCompressorZlib(ArchiveHandle *AH, CompressorState *cs, bool flush)
>  }
>  
>  static void
> -WriteDataToArchiveZlib(ArchiveHandle *AH, CompressorState *cs,
> -					   const char *data, size_t dLen)
> +EndCompressorGzip(ArchiveHandle *AH, CompressorState *cs)
>  {
> -	cs->zp->next_in = (void *) unconstify(char *, data);
> -	cs->zp->avail_in = dLen;
> -	DeflateCompressorZlib(AH, cs, false);
> +	/* If deflation was initialized, finalize it */
> +	if (cs->private_data)
> +		DeflateCompressorEnd(AH, cs);
>  }
>  
>  static void
> -ReadDataFromArchiveZlib(ArchiveHandle *AH, ReadFunc readF)
> +WriteDataToArchiveGzip(ArchiveHandle *AH, CompressorState *cs,
> +					   const void *data, size_t dLen)
> +{
> +	GzipCompressorState *gzipcs = (GzipCompressorState *) cs->private_data;
> +
> +	gzipcs->zp->next_in = (void *) unconstify(void *, data);
> +	gzipcs->zp->avail_in = dLen;
> +	DeflateCompressorCommon(AH, cs, false);
> +}
> +
> +static void
> +ReadDataFromArchiveGzip(ArchiveHandle *AH, CompressorState *cs)
>  {
>  	z_streamp	zp;
>  	char	   *out;
> @@ -342,7 +185,7 @@ ReadDataFromArchiveZlib(ArchiveHandle *AH, ReadFunc readF)
>  				 zp->msg);
>  
>  	/* no minimal chunk size for zlib */
> -	while ((cnt = readF(AH, &buf, &buflen)))
> +	while ((cnt = cs->readF(AH, &buf, &buflen)))
>  	{
>  		zp->next_in = (void *) buf;
>  		zp->avail_in = cnt;
> @@ -382,389 +225,196 @@ ReadDataFromArchiveZlib(ArchiveHandle *AH, ReadFunc readF)
>  	free(out);
>  	free(zp);
>  }
[... more incomprehensible changes elided ...]