Re: Add LZ4 compression in pg_dump

[gkokolatos]

------- Original Message -------
On Saturday, March 11th, 2023 at 7:00 AM, Alexander Lakhin 
<exclus...@gmail.com> wrote:

> Hello,
> 23.02.2023 23:24, Tomas Vondra wrote:
> 
> > On 2/23/23 16:26, Tomas Vondra wrote:
> > 
> > > Thanks for v30 with the updated commit messages. I've pushed 0001 after
> > > fixing a comment typo and removing (I think) an unnecessary change in an
> > > error message.
> > > 
> > > I'll give the buildfarm a bit of time before pushing 0002 and 0003.
> > 
> > I've now pushed 0002 and 0003, after minor tweaks (a couple typos etc.),
> > and marked the CF entry as committed. Thanks for the patch!
> > 
> > I wonder how difficult would it be to add the zstd compression, so that
> > we don't have the annoying "unsupported" cases.
> 
> 
> With the patch 0003 committed, a single warning -Wtype-limits appeared in the
> master branch:
> $ CPPFLAGS="-Og -Wtype-limits" ./configure --with-lz4 -q && make -s -j8
> compress_lz4.c: In function ‘LZ4File_gets’:
> compress_lz4.c:492:19: warning: comparison of unsigned expression in ‘< 0’ is
> always false [-Wtype-limits]
> 492 | if (dsize < 0)
> |

Thank you Alexander. Please find attached an attempt to address it.

> (I wonder, is it accidental that there no other places that triggers
> the warning, or some buildfarm animals had this check enabled before?)

I can not answer about the buildfarms. Do you think that adding an explicit
check for this warning in meson would help? I am a bit uncertain as I think
that type-limits are included in extra.

@@ -1748,6 +1748,7 @@ common_warning_flags = [
   '-Wshadow=compatible-local',
   # This was included in -Wall/-Wformat in older GCC versions
   '-Wformat-security',
+  '-Wtype-limits',
 ]

> 
> It is not a false positive as can be proved by the 002_pg_dump.pl modified as
> follows:
> - program => $ENV{'LZ4'},
> 
> + program => 'mv',
> 
> args => [
> 
> - '-z', '-f', '--rm',
> "$tempdir/compression_lz4_dir/blobs.toc",
> "$tempdir/compression_lz4_dir/blobs.toc.lz4",
> ],
> },

Correct, it is not a false positive. The existing testing framework provides
limited support for exercising error branches. Especially so when those are
dependent on generated output. 

> A diagnostic logging added shows:
> LZ4File_gets() after LZ4File_read_internal; dsize: 18446744073709551615
> 
> and pg_restore fails with:
> error: invalid line in large object TOC file
> ".../src/bin/pg_dump/tmp_check/tmp_test_22ri/compression_lz4_dir/blobs.toc": 
> "????"

It is a good thing that the restore fails with bad input. Yet it should
have failed earlier. The attached makes certain it does fail earlier. 

Cheers,
//Georgios

> 
> Best regards,
> Alexander
From b80bb52ef6546aee8c8431d7cc126fa4a76b543c Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos <gkokola...@pm.me>
Date: Sat, 11 Mar 2023 09:54:40 +0000
Subject: [PATCH v1] Respect return type of LZ4File_read_internal

The function LZ4File_gets() was storing the return value of
LZ4File_read_internal in a variable of the wrong type, disregarding sign-es.
As a consequence, LZ4File_gets() would not take the error path when it should.

In an attempt to improve readability, spell out the significance of a negative
return value of LZ4File_read_internal() in LZ4File_read().

Reported-by: Alexander Lakhin <exclus...@gmail.com>
---
 src/bin/pg_dump/compress_lz4.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/bin/pg_dump/compress_lz4.c b/src/bin/pg_dump/compress_lz4.c
index 63e794cdc6..cc039f0b47 100644
--- a/src/bin/pg_dump/compress_lz4.c
+++ b/src/bin/pg_dump/compress_lz4.c
@@ -453,7 +453,7 @@ LZ4File_read(void *ptr, size_t size, CompressFileHandle *CFH)
 	int			ret;
 
 	ret = LZ4File_read_internal(fs, ptr, size, false);
-	if (ret != size && !LZ4File_eof(CFH))
+	if (ret < 0 || (ret != size && !LZ4File_eof(CFH)))
 		pg_fatal("could not read from input file: %s", LZ4File_get_error(CFH));
 
 	return ret;
@@ -486,14 +486,14 @@ static char *
 LZ4File_gets(char *ptr, int size, CompressFileHandle *CFH)
 {
 	LZ4File    *fs = (LZ4File *) CFH->private_data;
-	size_t		dsize;
+	int			ret;
 
-	dsize = LZ4File_read_internal(fs, ptr, size, true);
-	if (dsize < 0)
+	ret = LZ4File_read_internal(fs, ptr, size, true);
+	if (ret < 0)
 		pg_fatal("could not read from input file: %s", LZ4File_get_error(CFH));
 
 	/* Done reading */
-	if (dsize == 0)
+	if (ret == 0)
 		return NULL;
 
 	return ptr;
-- 
2.34.1