pgsql: Add LZ4 compression to pg_dump

Re: Tomas Vondra

Add LZ4 compression to pg_dump

This broke the TAP tests on Ubuntu 18.04 (bionic):

[17:06:45.513](0.000s) ok 1927 - compression_lz4_custom: should not dump test_table with 4-row INSERTs
# Running: pg_dump --jobs=2 --format=directory --compress=lz4:1 --file=/home/myon/projects/postgresql/pg/master/build/src/bin/pg_dump/tmp_check/tmp_test__aAO/compression_lz4_dir postgres
[17:06:46.651](1.137s) ok 1928 - compression_lz4_dir: pg_dump runs
# Running: /usr/bin/lz4 -z -f --rm /home/myon/projects/postgresql/pg/master/build/src/bin/pg_dump/tmp_check/tmp_test__aAO/compression_lz4_dir/blobs.toc /home/myon/projects/postgresql/pg/master/build/src/bin/pg_dump/tmp_check/tmp_test__aAO/compression_lz4_dir/blobs.toc.lz4
Incorrect parameters
Usage :
/usr/bin/lz4 [arg] [input] [output]

input : a filename
with no FILE, or when FILE is - or stdin, read standard input
Arguments :
-1 : Fast compression (default)
-9 : High compression
-d : decompression (default for .lz4 extension)
-z : force compression
-f : overwrite output without prompting
-h/-H : display help/long help and exit
[17:06:46.667](0.016s) not ok 1929 - compression_lz4_dir: compression commands
[17:06:46.668](0.001s)
[17:06:46.668](0.001s) # Failed test 'compression_lz4_dir: compression commands'
[17:06:46.669](0.000s) # at t/002_pg_dump.pl line 4274.
[17:06:46.670](0.001s) ok 1930 - compression_lz4_dir: glob check for /home/myon/projects/postgresql/pg/master/build/src/bin/pg_dump/tmp_check/tmp_test__aAO/compression_lz4_dir/toc.dat

The lz4 binary there doesn't have the --rm option yet.

liblz4-tool 0.0~r131-2ubuntu3

--rm appears in a single place only:

# Give coverage for manually compressed blob.toc files during
# restore.
compress_cmd => {
program => $ENV{'LZ4'},
args => [
'-z', '-f', '--rm',
"$tempdir/compression_lz4_dir/blobs.toc",
"$tempdir/compression_lz4_dir/blobs.toc.lz4",
],
},

18.04 will be EOL in a few weeks so it might be ok to just say it's
not supported, but removing the input file manually after calling lz4
would be an easy fix.

Christoph

On 8 Mar 2023, at 18:55, Christoph Berg <myon@debian.org> wrote:

18.04 will be EOL in a few weeks so it might be ok to just say it's
not supported, but removing the input file manually after calling lz4
would be an easy fix.

Is it reasonable to expect that this version of LZ4 can/will appear on any
other platform outside of archeology? Removing the file manually would be a
trivial way to stabilize but if it's only expected to happen on platforms which
are long since EOL by the time 16 ships then the added complication could be
hard to justify.

--
Daniel Gustafsson

On 3/8/23 20:20, Daniel Gustafsson wrote:

On 8 Mar 2023, at 18:55, Christoph Berg <myon@debian.org> wrote:

18.04 will be EOL in a few weeks so it might be ok to just say it's
not supported, but removing the input file manually after calling lz4
would be an easy fix.

Is it reasonable to expect that this version of LZ4 can/will appear on any
other platform outside of archeology? Removing the file manually would be a
trivial way to stabilize but if it's only expected to happen on platforms which
are long since EOL by the time 16 ships then the added complication could be
hard to justify.

IMO we should fix that. We have a bunch of buildfarm members running on
Ubuntu 18.04 (or older) - it's true none of them seems to be running TAP
tests. But considering how trivial the fix is ...

Barring objections, I'll push a fix early next week.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Mar 09, 2023 at 12:39:08AM +0100, Tomas Vondra wrote:

IMO we should fix that. We have a bunch of buildfarm members running on
Ubuntu 18.04 (or older) - it's true none of them seems to be running TAP
tests. But considering how trivial the fix is ...

Barring objections, I'll push a fix early next week.

+1, better to change that, thanks.  Actually, would --rm be OK even on
Windows?  As far as I can see, the CI detects a LZ4 command for the
VS2019 environment but not the liblz4 libraries that would be needed
to trigger the set of tests.
--
Michael

On 3/9/23 01:30, Michael Paquier wrote:

On Thu, Mar 09, 2023 at 12:39:08AM +0100, Tomas Vondra wrote:

IMO we should fix that. We have a bunch of buildfarm members running on
Ubuntu 18.04 (or older) - it's true none of them seems to be running TAP
tests. But considering how trivial the fix is ...

Barring objections, I'll push a fix early next week.

+1, better to change that, thanks. Actually, would --rm be OK even on
Windows? As far as I can see, the CI detects a LZ4 command for the
VS2019 environment but not the liblz4 libraries that would be needed
to trigger the set of tests.

Thanks for noticing that. I'll investigate next week.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3/9/23 19:00, Tomas Vondra wrote:

On 3/9/23 01:30, Michael Paquier wrote:

On Thu, Mar 09, 2023 at 12:39:08AM +0100, Tomas Vondra wrote:

IMO we should fix that. We have a bunch of buildfarm members running on
Ubuntu 18.04 (or older) - it's true none of them seems to be running TAP
tests. But considering how trivial the fix is ...

Barring objections, I'll push a fix early next week.

+1, better to change that, thanks. Actually, would --rm be OK even on
Windows? As far as I can see, the CI detects a LZ4 command for the
VS2019 environment but not the liblz4 libraries that would be needed
to trigger the set of tests.

Thanks for noticing that. I'll investigate next week.

So, here's a fix that should (I think) replace the 'lz4 --rm' with a
simple 'rm'. I have two doubts about this, though:

1) I haven't found a simple way to inject additional command into the
test. The pg_dump runs have a predefined list of "steps" to run:

-- compress_cmd
-- glob_patterns
-- command_like
-- restore_cmd

and I don't think there's a good place to inject the 'rm' so I ended up
adding a 'cleanup_cmd' right after 'compress_cmd'. But it seems a bit
strange / hacky. Maybe there's a better way?

2) I wonder if Windows will know what 'rm' means. I haven't found any
TAP test doing 'rm' and don't see 'rm' in any $ENV either.

That being said, I have no idea how to make this work on our Windows CI.
As mentioned, the environment is missing the lz4 library - there's a

setup_additional_packages_script: |
REM choco install -y --no-progress ...

in the .yml file, but AFAICS the chocolatey does not have lz4 :-/

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Tomas Vondra

and I don't think there's a good place to inject the 'rm' so I ended up
adding a 'cleanup_cmd' right after 'compress_cmd'. But it seems a bit
strange / hacky. Maybe there's a better way?

Does the file need to be removed at all? Could we leave it there and
have "make clean" remove it?

Christoph

On 3/14/23 11:34, Christoph Berg wrote:

Re: Tomas Vondra

and I don't think there's a good place to inject the 'rm' so I ended up
adding a 'cleanup_cmd' right after 'compress_cmd'. But it seems a bit
strange / hacky. Maybe there's a better way?

Does the file need to be removed at all? Could we leave it there and
have "make clean" remove it?

I don't think that'd work, because of the automatic "discovery" where we
check if a file exists, and if not we try to append .gz and .lz4. So if
you leave the .toc, we'd not find the .lz4, making the test useless ...

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Mar 14, 2023 at 12:16:16AM +0100, Tomas Vondra wrote:

On 3/9/23 19:00, Tomas Vondra wrote:

On 3/9/23 01:30, Michael Paquier wrote:

On Thu, Mar 09, 2023 at 12:39:08AM +0100, Tomas Vondra wrote:

IMO we should fix that. We have a bunch of buildfarm members running on
Ubuntu 18.04 (or older) - it's true none of them seems to be running TAP
tests. But considering how trivial the fix is ...

Barring objections, I'll push a fix early next week.

+1, better to change that, thanks. Actually, would --rm be OK even on
Windows? As far as I can see, the CI detects a LZ4 command for the
VS2019 environment but not the liblz4 libraries that would be needed
to trigger the set of tests.

Thanks for noticing that. I'll investigate next week.

So, here's a fix that should (I think) replace the 'lz4 --rm' with a
simple 'rm'. I have two doubts about this, though:

1) I haven't found a simple way to inject additional command into the
test. The pg_dump runs have a predefined list of "steps" to run:

-- compress_cmd
-- glob_patterns
-- command_like
-- restore_cmd

and I don't think there's a good place to inject the 'rm' so I ended up
adding a 'cleanup_cmd' right after 'compress_cmd'. But it seems a bit
strange / hacky. Maybe there's a better way?

I don't know if there's a better way, and I don't think it's worth
complicating the tests by more than about 2 lines to handle this.

2) I wonder if Windows will know what 'rm' means. I haven't found any
TAP test doing 'rm' and don't see 'rm' in any $ENV either.

CI probably will, since it's Andres' image built with git-tools and
other helpful stuff installed. But in general I think it won't; perl is
being used for all the portable stuff.

*If* you wanted to do something to fix this, you could create a key
called files_to_remove_after_loading, and run unlink on those files
rather than running a shell command. Or maybe just remove the file
unconditionally at the start of the script ?

That being said, I have no idea how to make this work on our Windows CI.
As mentioned, the environment is missing the lz4 library - there's a

setup_additional_packages_script: |
REM choco install -y --no-progress ...

in the .yml file, but AFAICS the chocolatey does not have lz4 :-/

I updated what I'd done in the zstd patch to also run with LZ4.
This won't apply directly due to other patches, but you get the idea...

Maybe it'd be good to have a commented-out "wraps" hint like there is
for choco. The downloaded files could be cached, too.

diff --git a/.cirrus.yml b/.cirrus.yml
index a3977a4036e..b4387a739f3 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -644,9 +644,11 @@ task:
     vcvarsall x64
     mkdir subprojects
     meson wrap install zstd
-    meson configure -D zstd:multithread=enabled --force-fallback-for=zstd
+    meson wrap install lz4
+    meson subprojects download
+    meson configure -D zstd:multithread=enabled --force-fallback-for=zstd --force-fallback-for=lz4
     set CC=c:\ProgramData\chocolatey\lib\ccache\tools\ccache-4.8-windows-x86_64\ccache.exe cl.exe
-    meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Db_pch=false -Dextra_lib_dirs=c:\openssl\1.1\lib -Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR% -DPG_TEST_EXTRA="%PG_TEST_EXTRA%" -D zstd=enabled -Dc_args="/Z7 /MDd" build
+    meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Db_pch=false -Dextra_lib_dirs=c:\openssl\1.1\lib -Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR% -DPG_TEST_EXTRA="%PG_TEST_EXTRA%" -D zstd=enabled -D lz4=enabled -Dc_args="/Z7 /MDd" build

build_script: |
vcvarsall x64

--
Justin

On 6 Apr 2023, at 18:39, Justin Pryzby <pryzby@telsasoft.com> wrote:

*If* you wanted to do something to fix this, you could create a key
called files_to_remove_after_loading, and run unlink on those files
rather than running a shell command. Or maybe just remove the file
unconditionally at the start of the script ?

Since the test is written in Perl, and Perl has a function for deleting files
which abstracts the platform differences, using it seems like a logical choice?
{cleanup_cmd} can be replaced with {cleanup_files} with an unlink called on
that?

--
Daniel Gustafsson