Обсуждение: lz4 --rm on Ubuntu 18.04 (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