------- Original Message -------
On Monday, May 8th, 2023 at 8:20 PM, Tomas Vondra
<tomas.von...@enterprisedb.com> wrote:
>
>
>
>
> On 5/8/23 18:19, gkokola...@pm.me wrote:
>
> > ------- Original Message -------
> > On Monday, May 8th, 2023 at 3:16 AM, Tom Lane t...@sss.pgh.pa.us wrote:
> >
> > > I wrote:
> > >
> > > > Michael Paquier mich...@paquier.xyz writes:
> > > >
> > > > > While testing this patch, I have triggered an error pointing out that
> > > > > the decompression path of LZ4 is broken for table data. I can
> > > > > reproduce that with a dump of the regression database, as of:
> > > > > make installcheck
> > > > > pg_dump --format=d --file=dump_lz4 --compress=lz4 regression
> > >
> > > > Ugh. Reproduced here ... so we need an open item for this.
> > >
> > > BTW, it seems to work with --format=c.
> >
> > Thank you for the extra tests. It seems that exists a gap in the test
> > coverage. Please find a patch attached that is addressing the issue
> > and attempt to provide tests for it.
>
>
> Seems I'm getting messages with a delay - this is mostly the same fix I
> ended up with, not realizing you already posted a fix.
Thank you very much for looking.
> I don't think we need the local "in" variable - the pointer parameter is
> local in the function, so we can modify it directly (with a cast).
> WriteDataToArchiveLZ4 does it that way too.
Sure, patch updated.
> The tests are definitely a good idea.
Thank you.
> I wonder if we should add a
> comment to DEFAULT_IO_BUFFER_SIZE mentioning that if we choose to
> increase the value in the future, we needs to tweak the tests too to use
> more data in order to exercise the buffering etc. Maybe it's obvious?
>
You are right. Added a comment both in the header and in the test.
I hope v2 gets closer to closing the open item for this.
Cheers,
//Georgios
>
> regards
>
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
From 89e7066d6c3c6a7eeb147c3f2d345c3046a4d155 Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos <gkokola...@pm.me>
Date: Mon, 8 May 2023 19:48:03 +0000
Subject: [PATCH v2] Advance input pointer when LZ4 compressing data
LZ4File_write() did not advance the input pointer on subsequent invocations of
LZ4F_compressUpdate(). As a result the generated compressed output would be a
compressed version of the same input chunk.
WriteDataToArchiveLZ4() which is also using LZ4F_compressUpdate() did not suffer
from this omission. Tests failed to catch this error because all of their input
would comfortably fit within the same input chunk. Tests have been added to
provide adequate coverage.
---
src/bin/pg_dump/compress_io.h | 7 ++++-
src/bin/pg_dump/compress_lz4.c | 2 ++
src/bin/pg_dump/t/002_pg_dump.pl | 46 ++++++++++++++++++++++++++++++++
3 files changed, 54 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h
index fd8752db0d..e8efc57f1a 100644
--- a/src/bin/pg_dump/compress_io.h
+++ b/src/bin/pg_dump/compress_io.h
@@ -17,7 +17,12 @@
#include "pg_backup_archiver.h"
-/* Default size used for IO buffers */
+/*
+ * Default size used for IO buffers
+ *
+ * When altering this value it might be useful to verify that the relevant tests
+ * cases are meaningfully updated to provide coverage.
+ */
#define DEFAULT_IO_BUFFER_SIZE 4096
extern char *supports_compression(const pg_compress_specification compression_spec);
diff --git a/src/bin/pg_dump/compress_lz4.c b/src/bin/pg_dump/compress_lz4.c
index f97b7550d1..b869780c0b 100644
--- a/src/bin/pg_dump/compress_lz4.c
+++ b/src/bin/pg_dump/compress_lz4.c
@@ -588,6 +588,8 @@ LZ4Stream_write(const void *ptr, size_t size, CompressFileHandle *CFH)
errno = (errno) ? errno : ENOSPC;
return false;
}
+
+ ptr = ((const char *) ptr) + chunk;
}
return true;
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 93e24d5145..d66f3b42ea 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -3108,6 +3108,52 @@ my %tests = (
},
},
+ 'CREATE TABLE test_compression_method' => {
+ create_order => 110,
+ create_sql => 'CREATE TABLE dump_test.test_compression_method (
+ col1 text
+ );',
+ regexp => qr/^
+ \QCREATE TABLE dump_test.test_compression_method (\E\n
+ \s+\Qcol1 text\E\n
+ \Q);\E
+ /xm,
+ like => {
+ %full_runs,
+ %dump_test_schema_runs,
+ section_pre_data => 1,
+ },
+ unlike => {
+ exclude_dump_test_schema => 1,
+ only_dump_measurement => 1,
+ },
+ },
+
+ # Insert enough data to surpass DEFAULT_IO_BUFFER_SIZE during
+ # (de)compression operations
+ 'COPY test_compression_method' => {
+ create_order => 111,
+ create_sql => 'INSERT INTO dump_test.test_compression_method (col1) '
+ . 'SELECT string_agg(a::text, \'\') FROM generate_series(1,4096) a;',
+ regexp => qr/^
+ \QCOPY dump_test.test_compression_method (col1) FROM stdin;\E
+ \n(?:\d{15277}\n){1}\\\.\n
+ /xm,
+ like => {
+ %full_runs,
+ data_only => 1,
+ section_data => 1,
+ only_dump_test_schema => 1,
+ test_schema_plus_large_objects => 1,
+ },
+ unlike => {
+ binary_upgrade => 1,
+ exclude_dump_test_schema => 1,
+ schema_only => 1,
+ only_dump_measurement => 1,
+ },
+ },
+
'CREATE TABLE fk_reference_test_table' => {
create_order => 21,
create_sql => 'CREATE TABLE dump_test.fk_reference_test_table (
--
2.34.1
