Re: Add LZ4 compression in pg_dump

From: Alexander Lakhin <exclusion(at)gmail(dot)com>
To: gkokolatos(at)pm(dot)me
Cc: Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Rachel Heaton <rachelmheaton(at)gmail(dot)com>
Subject: Re: Add LZ4 compression in pg_dump
Date: 2023-03-11 18:00:00
Message-ID: 717391f1-4d73-45c0-e649-4b4aaff3f87d@gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Georgios,

11.03.2023 13:50, gkokolatos(at)pm(dot)me wrote:
> 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',
> ]
I'm not sure that I can promote additional checks (or determine where
to put them), but if some patch introduces a warning of a type that wasn't
present before, I think it's worth to eliminate the warning (if it is
sensible) to keep the source code check baseline at the same level
or even lift it up gradually.
I've also found that the same commit introduced a single instance of
the analyzer-possible-null-argument warning:
CPPFLAGS="-Og -fanalyzer -Wno-analyzer-malloc-leak -Wno-analyzer-file-leak
-Wno-analyzer-null-dereference -Wno-analyzer-shift-count-overflow
-Wno-analyzer-free-of-non-heap -Wno-analyzer-null-argument
-Wno-analyzer-double-free -Wanalyzer-possible-null-argument" ./configure
--with-lz4 -q && make -s -j8
compress_io.c: In function ‘hasSuffix’:
compress_io.c:158:47: warning: use of possibly-NULL ‘filename’ where non-null
expected [CWE-690] [-Wanalyzer-possible-null-argument]
  158 |         int                     filenamelen = strlen(filename);
      | ^~~~~~~~~~~~~~~~
  ‘InitDiscoverCompressFileHandle’: events 1-3
...

(I use gcc-11.3.)
As I can see, many existing uses of strdup() are followed by a check for
null result, so maybe it's a common practice and a similar check should
be added in InitDiscoverCompressFileHandle().
(There also a couple of other warnings introduced with the lz4 compression
patches, but those ones are not unique, so I maybe they aren't worth fixing.)

>> 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.
>>
Thanks! Your patch definitely fixes the issue.

Best regards,
Alexander

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Mark Dilger 2023-03-11 18:08:32 Re: Transparent column encryption
Previous Message John Naylor 2023-03-11 15:54:40 Re: [PoC] Improve dead tuple storage for lazy vacuum