Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Tuesday, May 9th, 2023 at 2:54 PM, Tomas Vondra 
 wrote:


> 
> 
> On 5/9/23 00:10, Michael Paquier wrote:
> 
> > On Mon, May 08, 2023 at 08:00:39PM +0200, Tomas Vondra wrote:
> > 
> > > The LZ4Stream_write() forgot to move the pointer to the next chunk, so
> > > it was happily decompressing the initial chunk over and over. A bit
> > > embarrassing oversight :-(
> > > 
> > > The custom format calls WriteDataToArchiveLZ4(), which was correct.
> > > 
> > > The attached patch fixes this for me.
> > 
> > Ouch. So this was corrupting the dumps and the compression when
> > trying to write more than two chunks at once, not the decompression
> > steps. That addresses the issue here as well, thanks!
> 
> 
> Yeah. Thanks for the report, should have been found during review.

Thank you both for looking. A small consolation is that now there are
tests for this case.

Moving on to the other open item for this, please find attached v2
of the patch as requested.

Cheers,
//Georgios

> 
> 
> regards
> 
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL CompanyFrom cb0a229be59dffe09cc0ceceececdbd06a559d3f Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Mon, 8 May 2023 11:58:57 +
Subject: [PATCH v2] Null terminate the output buffer of LZ4Stream_gets

LZ4Stream_gets did not null terminate its output buffer. Its callers expected
the buffer to be null terminated so they passed it around to functions such as
sscanf with unintended consequences.

Reported-by: Alexander Lakhin
---
 src/bin/pg_dump/compress_lz4.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_dump/compress_lz4.c b/src/bin/pg_dump/compress_lz4.c
index 423e1b7976..f97b7550d1 100644
--- a/src/bin/pg_dump/compress_lz4.c
+++ b/src/bin/pg_dump/compress_lz4.c
@@ -459,6 +459,10 @@ LZ4Stream_read_internal(LZ4State *state, void *ptr, int ptrsize, bool eol_flag)
 	if (!LZ4Stream_init(state, size, false /* decompressing */ ))
 		return -1;
 
+	/* No work needs to be done for a zero-sized output buffer */
+	if (size <= 0)
+		return 0;
+
 	/* Verify that there is enough space in the outbuf */
 	if (size > state->buflen)
 	{
@@ -636,7 +640,7 @@ LZ4Stream_gets(char *ptr, int size, CompressFileHandle *CFH)
 	LZ4State   *state = (LZ4State *) CFH->private_data;
 	int			ret;
 
-	ret = LZ4Stream_read_internal(state, ptr, size, true);
+	ret = LZ4Stream_read_internal(state, ptr, size - 1, true);
 	if (ret < 0 || (ret == 0 && !LZ4Stream_eof(CFH)))
 		pg_fatal("could not read from input file: %s", LZ4Stream_get_error(CFH));
 
@@ -644,6 +648,12 @@ LZ4Stream_gets(char *ptr, int size, CompressFileHandle *CFH)
 	if (ret == 0)
 		return NULL;
 
+	/*
+	 * Our caller expects the return string to be NULL terminated
+	 * and we know that ret is greater than zero.
+	 */
+	ptr[ret - 1] = '\0';
+
 	return ptr;
 }
 
-- 
2.34.1

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Monday, May 8th, 2023 at 8:20 PM, Tomas Vondra 
 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 CompanyFrom 89e7066d6c3c6a7eeb147c3f2d345c3046a4d155 Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Mon, 8 May 2023 19:48:03 +
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 => {

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Monday, May 8th, 2023 at 3:16 AM, Tom Lane  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.

Cheers,
//Georgios

> regards, tom laneFrom 8c6c86c362820e93f066992ede6e5ca23f128807 Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Mon, 8 May 2023 15:25:25 +
Subject: [PATCH v1] 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_lz4.c   |  5 +++-
 src/bin/pg_dump/t/002_pg_dump.pl | 44 
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_dump/compress_lz4.c b/src/bin/pg_dump/compress_lz4.c
index f97b7550d1..76211c82f0 100644
--- a/src/bin/pg_dump/compress_lz4.c
+++ b/src/bin/pg_dump/compress_lz4.c
@@ -564,6 +564,7 @@ LZ4Stream_write(const void *ptr, size_t size, CompressFileHandle *CFH)
 	LZ4State   *state = (LZ4State *) CFH->private_data;
 	size_t		status;
 	int			remaining = size;
+	const void *in = ptr;
 
 	/* Lazy init */
 	if (!LZ4Stream_init(state, size, true))
@@ -576,7 +577,7 @@ LZ4Stream_write(const void *ptr, size_t size, CompressFileHandle *CFH)
 		remaining -= chunk;
 
 		status = LZ4F_compressUpdate(state->ctx, state->buffer, state->buflen,
-	 ptr, chunk, NULL);
+	 in, chunk, NULL);
 		if (LZ4F_isError(status))
 		{
 			state->errcode = status;
@@ -588,6 +589,8 @@ LZ4Stream_write(const void *ptr, size_t size, CompressFileHandle *CFH)
 			errno = (errno) ? errno : ENOSPC;
 			return false;
 		}
+
+		in = ((const char *) in) + 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..c6b1225815 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -3108,6 +3108,50 @@ 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,
+		},
+	},
+
+	'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

Re: Add LZ4 compression in pg_dump

[gkokolatos]

On Sat, May 6, 2023 at 04:51, Michael Paquier <[mich...@paquier.xyz](mailto:On 
Sat, May 6, 2023 at 04:51, Michael Paquier < wrote:

> On Fri, May 05, 2023 at 02:13:28PM +, gkokola...@pm.me wrote:
>> Good point. I thought about it before submitting the patch. I
>> concluded that given the complexity and operations involved in
>> LZ4Stream_read_internal() and the rest of t he pg_dump/pg_restore
>> code, the memset() call will be negligible. However from the
>> readability point of view, the function is a bit cleaner with the
>> memset().
>>
>> I will not object to any suggestion though, as this is a very
>> trivial point. Please find attached a v2 of the patch following the
>> suggested approach.
>
> Please note that an open item has been added for this stuff.

Thank you but I am not certain I know what that means. Can you please explain?

Cheers,
//Georgios

> --
> Michael

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Friday, May 5th, 2023 at 3:23 PM, Andrew Dunstan  wrote:

> On 2023-05-05 Fr 06:02, gkokola...@pm.me wrote:
>
>> --- Original Message ---
>> On Friday, May 5th, 2023 at 8:00 AM, Alexander Lakhin
>> [](mailto:exclus...@gmail.com)
>> wrote:
>>
>>> 23.03.2023 20:10, Tomas Vondra wrote:
>>>
 So pushed all three parts, after updating the commit messages a bit.

 This leaves the empty-data issue (which we have a fix for) and the
 switch to LZ4F. And then the zstd part.
>>>
>>> I'm sorry that I haven't noticed/checked that before, but when trying to
>>> perform check-world with Valgrind I've discovered another issue presumably
>>> related to LZ4File_gets().
>>> When running under Valgrind:
>>> PROVE_TESTS=t/002_pg_dump.pl make check -C src/bin/pg_dump/
>>> I get:
>>> ...
>>> 07:07:11.683 ok 1939 - compression_lz4_dir: glob check for
>>> .../src/bin/pg_dump/tmp_check/tmp_test_HB6A/compression_lz4_dir/*.dat.lz4
>>> # Running: pg_restore --jobs=2 
>>> --file=.../src/bin/pg_dump/tmp_check/tmp_test_HB6A/compression_lz4_dir.sql
>>> .../src/bin/pg_dump/tmp_check/tmp_test_HB6A/compression_lz4_dir
>>>
>>> ==00:00:00:00.579 2811926== Conditional jump or move depends on 
>>> uninitialised value(s)
>>> ==00:00:00:00.579 2811926== at 0x4853376: rawmemchr 
>>> (vg_replace_strmem.c:1548)
>>> ==00:00:00:00.579 2811926== by 0x4C96A67: _IO_str_init_static_internal 
>>> (strops.c:41)
>>> ==00:00:00:00.579 2811926== by 0x4C693A2: _IO_strfile_read (strfile.h:95)
>>> ==00:00:00:00.579 2811926== by 0x4C693A2: __isoc99_sscanf 
>>> (isoc99_sscanf.c:28)
>>> ==00:00:00:00.579 2811926== by 0x11DB6F: _LoadLOs 
>>> (pg_backup_directory.c:458)
>>> ==00:00:00:00.579 2811926== by 0x11DD1E: _PrintTocData 
>>> (pg_backup_directory.c:422)
>>> ==00:00:00:00.579 2811926== by 0x118484: restore_toc_entry 
>>> (pg_backup_archiver.c:882)
>>> ==00:00:00:00.579 2811926== by 0x1190CC: RestoreArchive 
>>> (pg_backup_archiver.c:699)
>>> ==00:00:00:00.579 2811926== by 0x10F25D: main (pg_restore.c:414)
>>> ==00:00:00:00.579 2811926==
>>> ...
>>>
>>> It looks like the line variable returned by gets_func() here is not
>>> null-terminated:
>>> while ((CFH->gets_func(line, MAXPGPATH, CFH)) != NULL)
>>>
>>> {
>>> ...
>>> if (sscanf(line, "%u %" CppAsString2(MAXPGPATH) "s\n", , lofname) != 2)
>>> ...
>>> And Valgrind doesn't like it.
>>
>> Valgrind is correct to not like it. LZ4Stream_gets() got modeled after
>> gets() when it should have been modeled after fgets().
>>
>> Please find a patch attached to address it.
>
> Isn't using memset here a bit wasteful? Why not just put a null at the end 
> after calling LZ4Stream_read_internal(), which tells you how many bytes it 
> has written?

Good point. I thought about it before submitting the patch. I concluded that 
given the complexity and operations involved in LZ4Stream_read_internal() and 
the rest of the pg_dump/pg_restore code, the memset() call will be negligible. 
However from the readability point of view, the function is a bit cleaner with 
the memset().

I will not object to any suggestion though, as this is a very trivial point. 
Please find attached a v2 of the patch following the suggested approach.

Cheers,

//Georgios

> cheers
>
> andrew
>
> --
> Andrew Dunstan
> EDB:
> https://www.enterprisedb.comFrom 65dbce1eb81597e3dd44eff62d8d667b0a3322da Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Fri, 5 May 2023 09:47:02 +
Subject: [PATCH v2] Null terminate the output buffer of LZ4Stream_gets

LZ4Stream_gets did not null terminate its output buffer. Its callers expected
the buffer to be null terminated so they passed it around to functions such as
sscanf with unintended consequences.

Reported-by: Alexander Lakhin
---
 src/bin/pg_dump/compress_lz4.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_dump/compress_lz4.c b/src/bin/pg_dump/compress_lz4.c
index 423e1b7976..0f447919b2 100644
--- a/src/bin/pg_dump/compress_lz4.c
+++ b/src/bin/pg_dump/compress_lz4.c
@@ -459,6 +459,10 @@ LZ4Stream_read_internal(LZ4State *state, void *ptr, int ptrsize, bool eol_flag)
 	if (!LZ4Stream_init(state, size, false /* decompressing */ ))
 		return -1;
 
+	/* No work needs to be done for a zero-sized output buffer */
+	if (size <= 0)
+		return 0;
+
 	/* Verify that there is enough space in the outbuf */
 	if (size > state->buflen)
 	{
@@ -636,7 +640,9 @@ LZ4Stream_gets(char *ptr, int size, CompressFileHandle *CFH)
 	LZ4State   *state = (LZ4State *) CFH->private_data;
 	int			ret;
 
-	ret = LZ4Stream_read_internal(state, ptr, size, true);
+	Assert(size > 1);
+
+	ret = LZ4Stream_read_internal(state, ptr, size - 1, true);
 	if (ret < 0 || (ret == 0 && !LZ4Stream_eof(CFH)))
 		pg_fatal("could not read from input file: %s", LZ4Stream_get_error(CFH));
 
@@ -644,6 +650,12 @@ LZ4Stream_gets(char *ptr, int size, CompressFileHandle *CFH)
 	if (ret == 0)
 		return NULL;
 
+	/*
+	 * Our caller expects the return string to be 

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Friday, May 5th, 2023 at 8:00 AM, Alexander Lakhin  
wrote:


> 
> 
> 23.03.2023 20:10, Tomas Vondra wrote:
> 
> > So pushed all three parts, after updating the commit messages a bit.
> > 
> > This leaves the empty-data issue (which we have a fix for) and the
> > switch to LZ4F. And then the zstd part.
> 
> 
> I'm sorry that I haven't noticed/checked that before, but when trying to
> perform check-world with Valgrind I've discovered another issue presumably
> related to LZ4File_gets().
> When running under Valgrind:
> PROVE_TESTS=t/002_pg_dump.pl make check -C src/bin/pg_dump/
> I get:
> ...
> 07:07:11.683 ok 1939 - compression_lz4_dir: glob check for
> .../src/bin/pg_dump/tmp_check/tmp_test_HB6A/compression_lz4_dir/*.dat.lz4
> # Running: pg_restore --jobs=2 
> --file=.../src/bin/pg_dump/tmp_check/tmp_test_HB6A/compression_lz4_dir.sql
> .../src/bin/pg_dump/tmp_check/tmp_test_HB6A/compression_lz4_dir
> 
> ==00:00:00:00.579 2811926== Conditional jump or move depends on uninitialised 
> value(s)
> ==00:00:00:00.579 2811926== at 0x4853376: rawmemchr (vg_replace_strmem.c:1548)
> ==00:00:00:00.579 2811926== by 0x4C96A67: _IO_str_init_static_internal 
> (strops.c:41)
> ==00:00:00:00.579 2811926== by 0x4C693A2: _IO_strfile_read (strfile.h:95)
> ==00:00:00:00.579 2811926== by 0x4C693A2: __isoc99_sscanf (isoc99_sscanf.c:28)
> ==00:00:00:00.579 2811926== by 0x11DB6F: _LoadLOs (pg_backup_directory.c:458)
> ==00:00:00:00.579 2811926== by 0x11DD1E: _PrintTocData 
> (pg_backup_directory.c:422)
> ==00:00:00:00.579 2811926== by 0x118484: restore_toc_entry 
> (pg_backup_archiver.c:882)
> ==00:00:00:00.579 2811926== by 0x1190CC: RestoreArchive 
> (pg_backup_archiver.c:699)
> ==00:00:00:00.579 2811926== by 0x10F25D: main (pg_restore.c:414)
> ==00:00:00:00.579 2811926==
> ...
> 
> It looks like the line variable returned by gets_func() here is not
> null-terminated:
> while ((CFH->gets_func(line, MAXPGPATH, CFH)) != NULL)
> 
> {
> ...
> if (sscanf(line, "%u %" CppAsString2(MAXPGPATH) "s\n", , lofname) != 2)
> ...
> And Valgrind doesn't like it.
> 

Valgrind is correct to not like it. LZ4Stream_gets() got modeled after
gets() when it should have been modeled after fgets().

Please find a patch attached to address it.

Cheers,
//Georgios

> Best regards,
> AlexanderFrom 587873da2b563c59b281051c2636cda667abf099 Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Fri, 5 May 2023 09:47:02 +
Subject: [PATCH] Null terminate the output buffer of LZ4Stream_gets

LZ4Stream_gets did not null terminate its output buffer. Its callers expected
the buffer to be null terminated so they passed it around to functions such as
sscanf with unintended consequences.

Reported-by: Alexander Lakhin
---
 src/bin/pg_dump/compress_lz4.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_dump/compress_lz4.c b/src/bin/pg_dump/compress_lz4.c
index 423e1b7976..26c9a8b280 100644
--- a/src/bin/pg_dump/compress_lz4.c
+++ b/src/bin/pg_dump/compress_lz4.c
@@ -459,6 +459,10 @@ LZ4Stream_read_internal(LZ4State *state, void *ptr, int ptrsize, bool eol_flag)
 	if (!LZ4Stream_init(state, size, false /* decompressing */ ))
 		return -1;
 
+	/* No work needs to be done for a zero-sized output buffer */
+	if (size <= 0)
+		return 0;
+
 	/* Verify that there is enough space in the outbuf */
 	if (size > state->buflen)
 	{
@@ -636,7 +640,12 @@ LZ4Stream_gets(char *ptr, int size, CompressFileHandle *CFH)
 	LZ4State   *state = (LZ4State *) CFH->private_data;
 	int			ret;
 
-	ret = LZ4Stream_read_internal(state, ptr, size, true);
+	Assert(size > 1);
+
+	/* Our caller expects the return string to be NULL terminated */
+	memset(ptr, '\0', size);
+
+	ret = LZ4Stream_read_internal(state, ptr, size - 1, true);
 	if (ret < 0 || (ret == 0 && !LZ4Stream_eof(CFH)))
 		pg_fatal("could not read from input file: %s", LZ4Stream_get_error(CFH));
 
-- 
2.34.1

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Tuesday, April 25th, 2023 at 8:02 AM, Michael Paquier  
wrote:


> 
> 
> On Wed, Apr 12, 2023 at 07:53:53PM -0500, Justin Pryzby wrote:
> 
> > I doubt it - in the !HAVE_LIBZ case, it's currently an "if" statement
> > with nothing but a comment, which isn't a problem.
> > 
> > I think the only issue with an empty "if" is when you have no braces,
> > like:
> > 
> > if (...)
> > #if ...
> > something;
> > #endif
> > 
> > // problem here //
> 
> 
> (My apologies for the late reply.)
> 
> Still it could be easily messed up, and that's not a style that
> really exists in the tree, either, because there are always #else
> blocks set up in such cases. Another part that makes me a bit
> uncomfortable is that we would still call twice
> parse_compress_specification(), something that should not happen but
> we are doing so on HEAD because the default compression_algorithm_str
> is "none" and we want to enforce "gzip" for custom and directory
> formats when building with zlib.
> 
> What about just moving this block a bit up, just before the
> compression spec parsing, then? If we set compression_algorithm_str,
> the specification is compiled with the expected default, once instead
> of twice.

For what is worth, I think this would be the best approach. +1

Cheers,
//Georgios

> --
> Michael

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Wednesday, March 29th, 2023 at 12:02 AM, Tomas Vondra 
 wrote:


> 
> 
> On 3/28/23 18:07, gkokola...@pm.me wrote:
> 
> > --- Original Message ---
> > On Friday, March 24th, 2023 at 10:30 AM, gkokola...@pm.me gkokola...@pm.me 
> > wrote:
> > 
> > > --- Original Message ---
> > > On Thursday, March 23rd, 2023 at 6:10 PM, Tomas Vondra 
> > > tomas.von...@enterprisedb.com wrote:
> > > 
> > > > This leaves the empty-data issue (which we have a fix for) and the
> > > > switch to LZ4F. And then the zstd part.
> > > 
> > > Please expect promptly a patch for the switch to frames.
> > 
> > Please find the expected patch attached. Note that the bulk of the
> > patch is code unification, variable renaming to something more
> > appropriate, and comment addition. These are changes that are not
> > strictly necessary to switch to LZ4F. I do believe that are
> > essential for code hygiene after the switch and they do belong
> > on the same commit.
> 
> 
> I think the patch is fine, but I'm wondering if the renames shouldn't go
> a bit further. It removes references to LZ4File struct, but there's a
> bunch of functions with LZ4File_ prefix. Why not to simply use LZ4_
> prefix? We don't have GzipFile either.
> 
> Sure, it might be a bit confusing because lz4.h uses LZ4_ prefix, but
> then we probably should not define LZ4_compressor_init ...

This is a good point. The initial thought was that since lz4.h is now
removed, such ambiguity will not be present. In v2 of the patch the
function is renamed to `LZ4State_compression_init` since this name
describes better its purpose. It initializes the LZ4State for
compression.

As for the LZ4File_ prefix, I have no objections. Please find the
prefix changed to LZ4Stream_. For the record, the word 'File' is not
unique to the lz4 implementation. The common data structure used by
the API in compress_io.h:

   typedef struct CompressFileHandle CompressFileHandle; 

The public functions for this API are named:

  InitCompressFileHandle
  InitDiscoverCompressFileHandle
  EndCompressFileHandle

And within InitCompressFileHandle the pattern is:

if (compression_spec.algorithm == PG_COMPRESSION_NONE)
InitCompressFileHandleNone(CFH, compression_spec);
else if (compression_spec.algorithm == PG_COMPRESSION_GZIP)
InitCompressFileHandleGzip(CFH, compression_spec);
else if (compression_spec.algorithm == PG_COMPRESSION_LZ4)
InitCompressFileHandleLZ4(CFH, compression_spec);

It was felt that a prefix was required due to the inclusion 'lz4.h'
header where naming functions as 'LZ4_' would be wrong. The prefix
'LZ4File_' seemed to be in line with the naming of the rest of
the relevant functions and structures. Other compressions, gzip and
none, did not face the same issue.

To conclude, I think that having a prefix is slightly preferred
over not having one. Since the prefix `LZ4File_` is not desired,
I propose `LZ4Stream_` in v2.

I will not object to dismissing the argument and drop `File` from
the prefix, if so requested.

> 
> Also, maybe the comments shouldn't use "File API" when compress_io.c
> calls that "Compressed stream API".

Done.

Cheers,
//Georgios

> 
> 
> regards
> 
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL CompanyFrom b17b60cc1ff608f85c6c75ab19ad40c0863cfa93 Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Fri, 31 Mar 2023 09:16:52 +
Subject: [PATCH v2] Use LZ4 frames in pg_dump's compressor API.

This change allows for greater compaction of data, especially so in very narrow
relations, by avoiding at least a compaction header and footer per row. Since
LZ4 frames are now used by both compression APIs, some code deduplication
opportunities have become obvious and are also implemented.

While at it, rename LZ4File* functions to LZ4Stream* to improve readability.

Reported by: Justin Pryzby
---
 src/bin/pg_dump/compress_lz4.c | 420 +
 1 file changed, 275 insertions(+), 145 deletions(-)

diff --git a/src/bin/pg_dump/compress_lz4.c b/src/bin/pg_dump/compress_lz4.c
index fc2f4e116d..7023b11a2c 100644
--- a/src/bin/pg_dump/compress_lz4.c
+++ b/src/bin/pg_dump/compress_lz4.c
@@ -17,7 +17,6 @@
 #include "compress_lz4.h"
 
 #ifdef USE_LZ4
-#include 
 #include 
 
 /*
@@ -29,133 +28,286 @@
 #endif
 
 /*--
- * Compressor API
- *--
+ * Common to both APIs
  */
 
-typedef struct LZ4CompressorState
+/*
+ * State used for LZ4 (de)compression by both APIs.
+ */
+typedef struct LZ4State
 {
-	char	   *outbuf;
-	size_t		outsize;
-} LZ4CompressorState;
+	/*
+	 * Used by the Stream API to keep track of the file stream.
+	 */
+	FILE	   *fp;
+
+	LZ4F_preferences_t prefs;
+
+	LZ4F_compressionContext_t	ctx;
+	LZ4F_decompressionContext_t	dtx;
+
+	/*
+	 * Used by the Stream API's lazy initialization.
+	 */
+	bool		inited;
+
+	/*
+	 * Used by the Stream API to distinguish between compression

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Friday, March 24th, 2023 at 10:30 AM, gkokola...@pm.me  
wrote:

> 
> --- Original Message ---
> On Thursday, March 23rd, 2023 at 6:10 PM, Tomas Vondra 
> tomas.von...@enterprisedb.com wrote:
> 
> > This leaves the empty-data issue (which we have a fix for) and the
> > switch to LZ4F. And then the zstd part.
> 
> Please expect promptly a patch for the switch to frames.

Please find the expected patch attached. Note that the bulk of the
patch is code unification, variable renaming to something more
appropriate, and comment addition. These are changes that are not
strictly necessary to switch to LZ4F. I do believe that are
essential for code hygiene after the switch and they do belong
on the same commit. 

Cheers,
//Georgios

> 
> Cheers,
> //GeorgiosFrom c289fb8d49b680ad180a44b20fff1dc9553b7494 Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Tue, 28 Mar 2023 15:48:06 +
Subject: [PATCH v1] Use LZ4 frames in pg_dump's compressor API.

This change allows for greater compaction of data, especially so in very narrow
relations, by avoiding at least a compaction header and footer per row. Since
LZ4 frames are now used by both compression APIs, some code deduplication
opportunities have become obvious and are also implemented.

Reported by: Justin Pryzby
---
 src/bin/pg_dump/compress_lz4.c | 358 ++---
 1 file changed, 244 insertions(+), 114 deletions(-)

diff --git a/src/bin/pg_dump/compress_lz4.c b/src/bin/pg_dump/compress_lz4.c
index fc2f4e116d..078dc35cd6 100644
--- a/src/bin/pg_dump/compress_lz4.c
+++ b/src/bin/pg_dump/compress_lz4.c
@@ -17,7 +17,6 @@
 #include "compress_lz4.h"
 
 #ifdef USE_LZ4
-#include 
 #include 
 
 /*
@@ -29,102 +28,279 @@
 #endif
 
 /*--
- * Compressor API
- *--
+ * Common to both APIs
  */
 
-typedef struct LZ4CompressorState
+/*
+ * State used for LZ4 (de)compression by both APIs.
+ */
+typedef struct LZ4State
 {
-	char	   *outbuf;
-	size_t		outsize;
-} LZ4CompressorState;
+	/*
+	 * Used by the File API to keep track of the file stream.
+	 */
+	FILE	   *fp;
+
+	LZ4F_preferences_t prefs;
+
+	LZ4F_compressionContext_t	ctx;
+	LZ4F_decompressionContext_t	dtx;
+
+	/*
+	 * Used by the File API's lazy initialization.
+	 */
+	bool		inited;
+
+	/*
+	 * Used by the File API to distinguish between compression
+	 * and decompression operations.
+	 */
+	bool		compressing;
+
+	/*
+	 * Used by the Compressor API to mark if the compression
+	 * headers have been written after initialization.
+	 */
+	bool		needs_header_flush;
+
+	size_t		buflen;
+	char	   *buffer;
+
+	/*
+	 * Used by the File API to store already uncompressed
+	 * data that the caller has not consumed.
+	 */
+	size_t		overflowalloclen;
+	size_t		overflowlen;
+	char	   *overflowbuf;
+
+	/*
+	 * Used by both APIs to keep track of the compressed
+	 * data length stored in the buffer.
+	 */
+	size_t		compressedlen;
+
+	/*
+	 * Used by both APIs to keep track of error codes.
+	 */
+	size_t		errcode;
+} LZ4State;
+
+/*
+ * Initialize the required LZ4State members for compression. Write the LZ4 frame
+ * header in a buffer keeping track of its length. Users of this function can
+ * choose when and how to write the header to a file stream.
+ *
+ * Returns true on success. In case of a failure returns false, and stores the
+ * error code in state->errcode.
+ */
+static bool
+LZ4_compression_state_init(LZ4State *state)
+{
+	size_t		status;
+
+	state->buflen = LZ4F_compressBound(DEFAULT_IO_BUFFER_SIZE, >prefs);
+
+	/*
+	 * LZ4F_compressBegin requires a buffer that is greater or equal to
+	 * LZ4F_HEADER_SIZE_MAX. Verify that the requirement is met.
+	 */
+	if (state->buflen < LZ4F_HEADER_SIZE_MAX)
+		state->buflen = LZ4F_HEADER_SIZE_MAX;
+
+	status = LZ4F_createCompressionContext(>ctx, LZ4F_VERSION);
+	if (LZ4F_isError(status))
+	{
+		state->errcode = status;
+		return false;
+	}
+
+	state->buffer = pg_malloc(state->buflen);
+	status = LZ4F_compressBegin(state->ctx,
+state->buffer, state->buflen,
+			   >prefs);
+	if (LZ4F_isError(status))
+	{
+		state->errcode = status;
+		return false;
+	}
+
+	state->compressedlen = status;
+
+	return true;
+}
+
+/*--
+ * Compressor API
+ *--
+ */
 
 /* Private routines that support LZ4 compressed data I/O */
-static void ReadDataFromArchiveLZ4(ArchiveHandle *AH, CompressorState *cs);
-static void WriteDataToArchiveLZ4(ArchiveHandle *AH, CompressorState *cs,
-  const void *data, size_t dLen);
-static void EndCompressorLZ4(ArchiveHandle *AH, CompressorState *cs);
 
 static void
 ReadDataFromArchiveLZ4(ArchiveHandle *AH, CompressorState *cs)
 {
-	LZ4_streamDecode_t lz4StreamDecode;
-	char	   *buf;
-	char	   *decbuf;
-	size_t		buflen;
-	size_t		cnt;
-
-	buflen = DEFAULT_IO_BUFFER_SIZE;
-	buf = pg_malloc(buflen);
-	decbuf = pg_malloc(buflen);
+	size_t		r;
+	size_t		readbuflen;
+	char	   *outbuf;
+	char	   *readbuf;
+	

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Thursday, March 16th, 2023 at 11:30 PM, Tomas Vondra 
 wrote:


> 
> 
> 
> 
> On 3/16/23 01:20, Justin Pryzby wrote:
> 
> > On Mon, Mar 13, 2023 at 10:47:12PM +0100, Tomas Vondra wrote:
> > 
> > > 
> > > Thanks. I don't want to annoy you too much, but could you split the
> > > patch into the "empty-data" fix and all the other changes (rearranging
> > > functions etc.)? I'd rather not mix those in the same commit.
> > 
> > I don't know if that makes sense? The "empty-data" fix creates a new
> > function called DeflateCompressorInit(). My proposal was to add the new
> > function in the same place in the file as it used to be.
> 
> 
> Got it. In that case I agree it's fine to do that in a single commit.

For what is worth, I think that this patch should get a +1 and get in. It
solves the empty writes problem and includes a test to a previous untested
case.

Cheers,
//Georgios

> 
> > The patch also moves the pg_fatal() that's being removed. I don't think
> > it's going to look any cleaner to read a history involving the
> > pg_fatal() first being added, then moved, then removed. Anyway, I'll
> > wait while the community continues discussion about the pg_fatal().
> 
> 
> I think the agreement was to replace the pg_fatal with and assert, and I
> see your patch already does that.
> 
> 
> regards
> 
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Thursday, March 23rd, 2023 at 6:10 PM, Tomas Vondra 
 wrote:



> 
> So pushed all three parts, after updating the commit messages a bit.

Thank you very much.

> 
> This leaves the empty-data issue (which we have a fix for) and the
> switch to LZ4F. And then the zstd part.

Please expect promptly a patch for the switch to frames.

Cheers,
//Georgios

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Thursday, March 16th, 2023 at 10:20 PM, Tomas Vondra 
 wrote:


> 
> 
> 
> 
> On 3/16/23 18:04, gkokola...@pm.me wrote:
> 
> > --- Original Message ---
> > On Tuesday, March 14th, 2023 at 4:32 PM, Tomas Vondra 
> > tomas.von...@enterprisedb.com wrote:
> > 
> > > On 3/14/23 16:18, gkokola...@pm.me wrote:
> > > 
> > > > ...> Would you mind me trying to come with a patch to address your 
> > > > points?
> > > 
> > > That'd be great, thanks. Please keep it split into smaller patches - two
> > > might work, with one patch for "cosmetic" changes and the other tweaking
> > > the API error-handling stuff.
> > 
> > Please find attached a set for it. I will admit that the splitting in the
> > series might not be ideal and what you requested. It is split on what
> > seemed as a logical units. Please advice how a better split can look like.
> > 
> > 0001 is unifying types and return values on the API
> > 0002 is addressing the constant definitions
> > 0003 is your previous 0004 adding comments
> 
> 
> Thanks. I think the split seems reasonable - the goal was to not mix
> different changes, and from that POV it works.
> 
> I'm not sure I understand the Gzip_read/Gzip_write changes in 0001. I
> mean, gzread/gzwrite returns int, so how does renaming the size_t
> variable solve the issue of negative values for errors? I mean, this
> 
> - size_t ret;
> + size_t gzret;
> 
> - ret = gzread(gzfp, ptr, size);
> + gzret = gzread(gzfp, ptr, size);
> 
> means we still lost the information gzread() returned a negative value,
> no? We'll still probably trigger an error, but it's a bit weird.

You are obviously correct. My bad, I miss-read the return type of gzread().

Please find an amended version attached.

> Unless I'm missing something, if gzread() ever returns -1 or some other
> negative error value, we'll cast it to size_t, while condition will
> evaluate to "true" and we'll happily chew on some random chunk of data.
> 
> So the confusion is (at least partially) a preexisting issue ...
> 
> For gzwrite() it seems to be fine, because that only returns 0 on error.
> OTOH it's defined to take 'int size' but then we happily pass size_t
> values to it.
> 
> As I wrote earlier, this apparently assumes we never need to deal with
> buffers larger than int, and I don't think we have the ambition to relax
> that (I'm not sure it's even needed / possible).

Agreed.


> I see the read/write functions are now defined as int, but we only ever
> return 0/1 from them, and then interpret that as bool. Why not to define
> it like that? I don't think we need to adhere to the custom that
> everything returns "int". This is an internal API. Or if we want to
> stick to int, I'd define meaningful "nice" constants for 0/1.

The return types are now booleans and the callers have been made aware.


> 0002 seems fine to me. I see you've ditched the idea of having two
> separate buffers, and replaced them with DEFAULT_IO_BUFFER_SIZE. Fine
> with me, although I wonder if this might have negative impact on
> performance or something (but I doubt that).
> 

I doubt that too. Thank you.

> 0003 seems fine too.

Thank you.


> > As far as the error handling is concerned, you had said upthread:
> > 
> > > I think the right approach is to handle all library errors and not just
> > > let them through. So Gzip_write() needs to check the return value, and
> > > either call pg_fatal() or translate it to an error defined by the API.
> > 
> > While working on it, I thought it would be clearer and more consistent
> > for the pg_fatal() to be called by the caller of the individual functions.
> > Each individual function can keep track of the specifics of the error
> > internally. Then the caller upon detecting that there was an error by
> > checking the return value, can call pg_fatal() with a uniform error
> > message and then add the specifics by calling the get_error_func().
> 
> 
> I agree it's cleaner the way you did it.
> 
> I was thinking that with each compression function handling error
> internally, the callers would not need to do that. But I haven't
> realized there's logic to detect ENOSPC and so on, and we'd need to
> duplicate that in every compression func.
> 

If you agree, I can prepare a patch to improve on the error handling
aspect of the API as a separate thread, since here we are trying to
focus on correctness.

Cheers,
//Georgios

> 
> regards
> 
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL CompanyFrom eeac82b647dc4021e1dcf22d8cc59840fbde8847 Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Fri, 17 Mar 2023 15:29:05 +
Subject: [PATCH v3 3/3] Improve compress_lz4 documentation.

Author: Tomas Vondra
---
 src/bin/pg_dump/compress_lz4.c | 25 -
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/src/bin/pg_dump/compress_lz4.c b/src/bin/pg_dump/compress_lz4.c
index 2f3e552f51..fc2f4e116d 100644
--- 

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Tuesday, March 14th, 2023 at 4:32 PM, Tomas Vondra 
 wrote:


> 
> 
> 
> 
> On 3/14/23 16:18, gkokola...@pm.me wrote:
> 
> > ...> Would you mind me trying to come with a patch to address your points?
> 
> 
> That'd be great, thanks. Please keep it split into smaller patches - two
> might work, with one patch for "cosmetic" changes and the other tweaking
> the API error-handling stuff.

Please find attached a set for it. I will admit that the splitting in the
series might not be ideal and what you requested. It is split on what
seemed as a logical units. Please advice how a better split can look like.

0001 is unifying types and return values on the API
0002 is addressing the constant definitions
0003 is your previous 0004 adding comments

As far as the error handling is concerned, you had said upthread:

> I think the right approach is to handle all library errors and not just
> let them through. So Gzip_write() needs to check the return value, and
> either call pg_fatal() or translate it to an error defined by the API.

While working on it, I thought it would be clearer and more consistent
for the pg_fatal() to be called by the caller of the individual functions.
Each individual function can keep track of the specifics of the error
internally. Then the caller upon detecting that there was an error by
checking the return value, can call pg_fatal() with a uniform error
message and then add the specifics by calling the get_error_func().

Thoughts?

Cheers,
//Georgios

> 
> regards
> 
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL CompanyFrom 4aa7603d891c62bf9d95af9910b8fb4b0fe2fb10 Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Thu, 16 Mar 2023 16:30:09 +
Subject: [PATCH v2 2/3] Clean up constants in pg_dump's compression API.

Prior to the introduction of the API, pg_dump would use the ZLIB_[IN|OUT]_SIZE
constants to handle buffer sizes throughout. This behaviour is confusing after
the introduction of the API. Ammend it by introducing a DEFAULT_IO_BUFFER_SIZE
constant to use when appropriate while giving the opportunity to specific
compression implementations to use their own.

With the help and guidance of Tomas Vondra.
---
 src/bin/pg_dump/compress_gzip.c   | 22 +++---
 src/bin/pg_dump/compress_io.h |  5 ++---
 src/bin/pg_dump/compress_lz4.c| 17 +
 src/bin/pg_dump/compress_none.c   |  4 ++--
 src/bin/pg_dump/pg_backup_directory.c |  4 ++--
 5 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/src/bin/pg_dump/compress_gzip.c b/src/bin/pg_dump/compress_gzip.c
index 29e2fd8d50..4106d4c866 100644
--- a/src/bin/pg_dump/compress_gzip.c
+++ b/src/bin/pg_dump/compress_gzip.c
@@ -120,8 +120,8 @@ WriteDataToArchiveGzip(ArchiveHandle *AH, CompressorState *cs,
 		 * actually allocate one extra byte because some routines want to
 		 * append a trailing zero byte to the zlib output.
 		 */
-		gzipcs->outbuf = pg_malloc(ZLIB_OUT_SIZE + 1);
-		gzipcs->outsize = ZLIB_OUT_SIZE;
+		gzipcs->outsize = DEFAULT_IO_BUFFER_SIZE;
+		gzipcs->outbuf = pg_malloc(gzipcs->outsize + 1);
 
 		/*
 		 * A level of zero simply copies the input one block at the time. This
@@ -158,10 +158,10 @@ ReadDataFromArchiveGzip(ArchiveHandle *AH, CompressorState *cs)
 	zp->zfree = Z_NULL;
 	zp->opaque = Z_NULL;
 
-	buf = pg_malloc(ZLIB_IN_SIZE);
-	buflen = ZLIB_IN_SIZE;
+	buflen = DEFAULT_IO_BUFFER_SIZE;
+	buf = pg_malloc(buflen);
 
-	out = pg_malloc(ZLIB_OUT_SIZE + 1);
+	out = pg_malloc(DEFAULT_IO_BUFFER_SIZE + 1);
 
 	if (inflateInit(zp) != Z_OK)
 		pg_fatal("could not initialize compression library: %s",
@@ -176,14 +176,14 @@ ReadDataFromArchiveGzip(ArchiveHandle *AH, CompressorState *cs)
 		while (zp->avail_in > 0)
 		{
 			zp->next_out = (void *) out;
-			zp->avail_out = ZLIB_OUT_SIZE;
+			zp->avail_out = DEFAULT_IO_BUFFER_SIZE;
 
 			res = inflate(zp, 0);
 			if (res != Z_OK && res != Z_STREAM_END)
 pg_fatal("could not uncompress data: %s", zp->msg);
 
-			out[ZLIB_OUT_SIZE - zp->avail_out] = '\0';
-			ahwrite(out, 1, ZLIB_OUT_SIZE - zp->avail_out, AH);
+			out[DEFAULT_IO_BUFFER_SIZE - zp->avail_out] = '\0';
+			ahwrite(out, 1, DEFAULT_IO_BUFFER_SIZE - zp->avail_out, AH);
 		}
 	}
 
@@ -192,13 +192,13 @@ ReadDataFromArchiveGzip(ArchiveHandle *AH, CompressorState *cs)
 	while (res != Z_STREAM_END)
 	{
 		zp->next_out = (void *) out;
-		zp->avail_out = ZLIB_OUT_SIZE;
+		zp->avail_out = DEFAULT_IO_BUFFER_SIZE;
 		res = inflate(zp, 0);
 		if (res != Z_OK && res != Z_STREAM_END)
 			pg_fatal("could not uncompress data: %s", zp->msg);
 
-		out[ZLIB_OUT_SIZE - zp->avail_out] = '\0';
-		ahwrite(out, 1, ZLIB_OUT_SIZE - zp->avail_out, AH);
+		out[DEFAULT_IO_BUFFER_SIZE - zp->avail_out] = '\0';
+		ahwrite(out, 1, DEFAULT_IO_BUFFER_SIZE - zp->avail_out, AH);
 	}
 
 	if (inflateEnd(zp) != Z_OK)
diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h
index 

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Monday, March 13th, 2023 at 9:21 PM, Tomas Vondra 
 wrote:


>
>
>
>
> On 3/11/23 11:50, gkokola...@pm.me wrote:
>
> > --- 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:

>
>
> Thanks for the patch.
>
> I did look if there are other places that might have the same issue, and
> I think there are - see attached 0002. For example LZ4File_write is
> declared to return size_t, but then it also does
>
> if (LZ4F_isError(status))
> {
> fs->errcode = status;
>
> return -1;
> }
>
> That won't work :-(

You are right. It is confusing.

>
> And these issues may not be restricted to lz4 code - Gzip_write is
> declared to return size_t, but it does
>
> return gzwrite(gzfp, ptr, size);
>
> and gzwrite returns int. Although, maybe that's correct, because
> gzwrite() is "0 on error" so maybe this is fine ...
>
> However, Gzip_read assigns gzread() to size_t, and that does not seem
> great. It probably will still trigger the following pg_fatal() because
> it'd be very lucky to match the expected 'size', but it's confusing.

Agreed.

>
>
> I wonder whether CompressorState should use int or size_t for the
> read_func/write_func callbacks. I guess no option is perfect, i.e. no
> data type will work for all compression libraries we might use (lz4 uses
> int while lz4f uses size_t, to there's that).
>
> It's a bit weird the "open" functions return int and the read/write
> size_t. Maybe we should stick to int, which is what the old functions
> (cfwrite etc.) did.
>
You are right. These functions are modeled by the open/fread/
fwrite etc, and they have kept the return types of these ones. Their
callers do check the return value of read_func and write_func against
the requested size of bytes to be transferred.

>
> But I think the actual problem here is that the API does not clearly
> define how errors are communicated. I mean, it's nice to return the
> value returned by the library function without "mangling" it by
> conversion to size_t, but what if the libraries communicate errors in
> different way? Some may return "0" while others may return "-1".

Agreed.

>
> I think the right approach is to handle all library errors and not just
> let them through. So Gzip_write() needs to check the return value, and
> either call pg_fatal() or translate it to an error defined by the API.

It makes sense. It will change some of the behaviour of the callers,
mostly on what constitutes an error, and what error message is emitted.
This is a reasonable change though.

>
> For example we might say "returns 0 on error" and then translate all
> library-specific errors to that.

Ok.

> While looking at the code I realized a couple function comments don't
> say what's returned in case of error, etc. So 0004 adds those.
>
> 0003 is a couple minor assorted comments/questions:
>
> - Should we move ZLIB_OUT_SIZE/ZLIB_IN_SIZE to compress_gzip.c?

It would make things clearer.

> - Why are LZ4 buffer sizes different (ZLIB has both 4kB)?

Clearly some comments are needed, if the difference makes sense.

> - I wonder if we actually need LZ4F_HEADER_SIZE_MAX? Is it even possible
> for LZ4F_compressBound to return value this small (especially for 16kB
> input buffer)?
>

I would recommend to keep it. Earlier versions of LZ4F_HEADER_SIZE_MAX
do not have it. Later versions do advise to use it.

Would you mind me trying to come with a patch to address your points?

Cheers,
//Georgios

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

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Monday, March 13th, 2023 at 10:47 PM, Tomas Vondra 
 wrote:



> 
> > Change pg_fatal() to an assertion+comment;
> 
> 
> Yeah, that's reasonable. I'd even ditch the assert/comment, TBH. We
> could add such protections against "impossible" stuff to a zillion other
> places and the confusion likely outweighs the benefits.
> 

A minor note to add is to not ignore the lessons learned from a7885c9bb.

For example, as the testing framework stands, one can not test that the
contents of the custom format are indeed compressed. One can infer it by
examining the header of the produced dump and searching for the
compression flag. The code responsible for writing the header and the
code responsible for actually dealing with data, is not the same. Also,
the compression library itself will happily read and write uncompressed
data.

A pg_fatal, assertion, or similar, is the only guard rail against this
kind of error. Without it, the tests will continue passing even after
e.g. a wrong initialization of the API. It was such a case that lead to
a7885c9bb in the first place. I do think that we wish it to be an
"impossible" case. Also it will be an untested case with some history
without such a guard rail.

Of course I will not object to removing it, if you think that is more
confusing than useful.

Cheers,
//Georgios

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

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Saturday, March 11th, 2023 at 7:00 AM, Alexander Lakhin 
 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,
> AlexanderFrom b80bb52ef6546aee8c8431d7cc126fa4a76b543c Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Sat, 11 Mar 2023 09:54:40 +
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 
---
 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

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Wednesday, March 1st, 2023 at 5:20 PM, Tomas Vondra 
 wrote:


> 
> 
> 
> 
> On 2/25/23 15:05, Justin Pryzby wrote:
> 
> > On Fri, Feb 24, 2023 at 11:02:14PM -0600, Justin Pryzby wrote:
> > 
> > > I have some fixes (attached) and questions while polishing the patch for
> > > zstd compression. The fixes are small and could be integrated with the
> > > patch for zstd, but could be applied independently.
> > 
> > One more - WriteDataToArchiveGzip() says:
> > 
> > + if (cs->compression_spec.level == 0)
> > + pg_fatal("requested to compress the archive yet no level was specified");
> > 
> > That was added at e9960732a.
> > 
> > But if you specify gzip:0, the compression level is already enforced by
> > validate_compress_specification(), before hitting gzip.c:
> > 
> > | pg_dump: error: invalid compression specification: compression algorithm 
> > "gzip" expects a compression level between 1 and 9 (default at -1)
> > 
> > 5e73a6048 intended that to work as before, and you can specify -Z0:
> > 
> > The change is backward-compatible, hence specifying only an integer
> > leads to no compression for a level of 0 and gzip compression when the
> > level is greater than 0.
> > 
> > $ time ./src/bin/pg_dump/pg_dump -h /tmp regression -t int8_tbl -Fp 
> > --compress 0 |file -
> > /dev/stdin: ASCII text
> 
> 
> FWIW I agree we should make this backwards-compatible - accept "0" and
> treat it as no compression.
> 
> Georgios, can you prepare a patch doing that?

Please find a patch attached. However I am a bit at a loss, the backwards
compatible behaviour has not changed. Passing -Z0/--compress=0 does produce
a non compressed output. So I am not really certain as to what broke and
needs fixing.

What commit 5e73a6048 did fail to do, is test the backwards compatible
behaviour. The attached amends it.

Cheers,
//Georgios

> 
> 
> regards
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL CompanyFrom 99c2da94ecbeacf997270dd26fc5c0a63ffcedd4 Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Thu, 2 Mar 2023 16:10:03 +
Subject: [PATCH vX] Add test for backwards compatible -Z0 option in pg_dump

Commit 5e73a6048 expanded pg_dump with the ability to use compression
specifications. A commonly shared code which lets the user control in an
extended way the method, level, and other details of a desired compression.

Prior to this commit, pg_dump could only accept an integer for the
-Z/--compress option. An integer value of 0 had the special meaning of non
compression. Commit 5e73a6048 respected and maintained this behaviour for
backwards compatibility.

However no tests covered this scenario. The current commit adds coverage for
this case.
---
 src/bin/pg_dump/t/002_pg_dump.pl | 13 +
 1 file changed, 13 insertions(+)

diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 7b5a6e190c..ec7aaab884 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -76,6 +76,19 @@ my %pgdump_runs = (
 		],
 	},
 
+	# Verify that the backwards compatible option -Z0 produces
+	# non compressed output
+	compression_none_plain => {
+		test_key   => 'compression',
+		# Enforce this test when compile option is available
+		compile_option => 'gzip',
+		dump_cmd   => [
+			'pg_dump',  '--format=plain',
+			'-Z0', "--file=$tempdir/compression_none_plain.sql",
+			'postgres',
+		],
+	},
+
 	# Do not use --no-sync to give test coverage for data sync.
 	compression_gzip_custom => {
 		test_key   => 'compression',
-- 
2.34.1

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Wednesday, March 1st, 2023 at 12:58 AM, Justin Pryzby  
wrote:



> I found that e9960732a broke writing of empty gzip-compressed data,
> specifically LOs. pg_dump succeeds, but then the restore fails:
> 
> postgres=# SELECT lo_create(1234);
> lo_create | 1234
> 
> $ time ./src/bin/pg_dump/pg_dump -h /tmp -d postgres -Fc 
> |./src/bin/pg_dump/pg_restore -f /dev/null -v
> pg_restore: implied data-only restore
> pg_restore: executing BLOB 1234
> pg_restore: processing BLOBS
> pg_restore: restoring large object with OID 1234
> pg_restore: error: could not uncompress data: (null)
> 

Thank you for looking. This was an untested case.

> The inline patch below fixes it, but you won't be able to apply it
> directly, as it's on top of other patches which rename the functions
> back to "Zlib" and rearranges the functions to their original order, to
> allow running:
> 
> git diff --diff-algorithm=minimal -w 
> e9960732a~:./src/bin/pg_dump/compress_io.c ./src/bin/pg_dump/compress_gzip.c
> 

Please find a patch attached that can be applied directly.

> The current function order avoids 3 lines of declarations, but it's
> obviously pretty useful to be able to run that diff command. I already
> argued for not calling the functions "Gzip" on the grounds that the name
> was inaccurate.

I have no idea why we are back on the naming issue. I stand by the name
because in my humble opinion helps the code reader. There is a certain
uniformity when the compression_spec.algorithm and the compressor
functions match as the following code sample shows.

if (compression_spec.algorithm == PG_COMPRESSION_NONE) 
InitCompressorNone(cs, compression_spec);
else if (compression_spec.algorithm == PG_COMPRESSION_GZIP)
InitCompressorGzip(cs, compression_spec);
else if (compression_spec.algorithm == PG_COMPRESSION_LZ4)
InitCompressorLZ4(cs, compression_spec);
 
When the reader wants to see what happens when the PG_COMPRESSION_XXX
is set, has to simply search for the XXX part. I think that this is
justification enough for the use of the names.

> 
> I'd want to create an empty large object in src/test/sql/largeobject.sql
> to exercise this tested during pgupgrade. But unfortunately that
> doesn't use -Fc, so this isn't hit. Empty input is an important enough
> test case to justify a tap test, if there's no better way.

Please find in the attached a test case that exercises this codepath.

Cheers,
//GeorgiosFrom 95450f0e7e90f0a1a3cdfc12c760a9520bd2995f Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Wed, 1 Mar 2023 12:42:32 +
Subject: [PATCH vX] Properly gzip compress when no data is available

When creating dumps with the Compressor API, it is possible to only call the
Allocate and End compressor functions without ever writing any data. The gzip
implementation wrongly assumed that the write function would be called and
defered the initialization of the internal compression system for the first
write call. The End call would only finilize the internal compression system if
that was ever initialized.

The problem with that approach is that it violated the expectations of the
internal compression system during decompression.

This commit initializes the internal compression system during the Allocate
call, under the condition that a write function was provided by the caller.
Given that decompression does not need to keep track of any state, the
compressor's private_data member is now populated only during compression.

Tests are added to cover this scenario.

Initial patch by Justin Pruzby.

Reported-by: Justin Pryzby
---
 src/bin/pg_dump/compress_gzip.c  | 118 +--
 src/bin/pg_dump/t/002_pg_dump.pl |  27 ++-
 2 files changed, 91 insertions(+), 54 deletions(-)

diff --git a/src/bin/pg_dump/compress_gzip.c b/src/bin/pg_dump/compress_gzip.c
index 0af65afeb4..f5d32cf059 100644
--- a/src/bin/pg_dump/compress_gzip.c
+++ b/src/bin/pg_dump/compress_gzip.c
@@ -32,9 +32,48 @@ typedef struct GzipCompressorState
 	size_t		outsize;
 } GzipCompressorState;
 
+
 /* Private routines that support gzip compressed data I/O */
 static void
-DeflateCompressorGzip(ArchiveHandle *AH, CompressorState *cs, bool flush)
+DeflateCompressorInit(CompressorState *cs)
+{
+	GzipCompressorState *gzipcs;
+	z_streamp	zp;
+
+	gzipcs = (GzipCompressorState *) pg_malloc0(sizeof(GzipCompressorState));
+	zp = gzipcs->zp = (z_streamp) pg_malloc(sizeof(z_stream));
+	zp->zalloc = Z_NULL;
+	zp->zfree = Z_NULL;
+	zp->opaque = Z_NULL;
+
+	/*
+	 * outsize is the buffer size we tell zlib it can output to.  We
+	 * actually allocate one extra byte because some routines want to
+	 * append a trailing zero byte to the zlib output.
+	 */
+	gzipcs->outbuf = pg_malloc(ZLIB_OUT_SIZE + 1);
+	gzipcs->outsize = ZLIB_OUT_SIZE;
+
+	/*
+	 * A level of zero simply copies the input one block at the time. This
+	 * is probably 

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Saturday, February 25th, 2023 at 3:05 PM, Justin Pryzby 
 wrote:


> 
> 
> On Fri, Feb 24, 2023 at 11:02:14PM -0600, Justin Pryzby wrote:
> 
> > I have some fixes (attached) and questions while polishing the patch for
> > zstd compression. The fixes are small and could be integrated with the
> > patch for zstd, but could be applied independently.


Please find some comments on the rest of the fixes patch that Tomas has not
commented on.

can be compressed with the gzip or
-   lz4tool.
+   lz4 tools.

+1

 The compression method can be set to gzip or
-lz4 or none for no compression.
+lz4, or none for no compression.

I am not a native English speaker. Yet I think that if one adds commas
in one of the options, then one should add commas to all the options.
Namely, the aboveis missing a comma between gzip and lz4. However I
think that not having any commas still works grammatically and
syntactically.

-   /*
-* A level of zero simply copies the input one block at the 
time. This
-* is probably not what the user wanted when calling this 
interface.
-*/
-   if (cs->compression_spec.level == 0)
-   pg_fatal("requested to compress the archive yet no 
level was specified");


I disagree with change. WriteDataToArchiveGzip() is far away from
what ever the code in pg_dump.c is doing. Any non valid values for
level will emit an error in when the proper gzip/zlib code is
called. A zero value however, will not emit such error. Having the
extra check there is a future proof guarantee in a very low cost.
Furthermore, it quickly informs the reader of the code about that
specific value helping with readability and comprehension.

If any change is required, something for which I vote strongly
against, I would at least recommend to replace it with an
assertion.

- * Initialize a compress file stream. Deffer the compression algorithm
+ * Initialize a compress file stream. Infer the compression algorithm

:+1:

-   # Skip command-level tests for gzip if there is no support for it.
+   # Skip command-level tests for gzip/lz4 if they're not supported.

We will be back at that again soon. Maybe change to:

Skip command-level test for unsupported compression methods

To include everything.


-   ($pgdump_runs{$run}->{compile_option} eq 'gzip' && 
!$supports_gzip) ||
-   ($pgdump_runs{$run}->{compile_option} eq 'lz4' && 
!$supports_lz4))
+   (($pgdump_runs{$run}->{compile_option} eq 'gzip' && 
!$supports_gzip) ||
+   ($pgdump_runs{$run}->{compile_option} eq 'lz4' && 
!$supports_lz4)))

Good catch, :+1:

Cheers,
//Georgios

> --
> Justin

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Sunday, February 26th, 2023 at 3:59 PM, Tomas Vondra 
 wrote:


> 
> 
> 
> 
> On 2/25/23 06:02, Justin Pryzby wrote:
> 
> > I have some fixes (attached) and questions while polishing the patch for
> > zstd compression. The fixes are small and could be integrated with the
> > patch for zstd, but could be applied independently.
> > 
> > - I'm unclear about get_error_func(). That's called in three places
> > from pg_backup_directory.c, after failures from write_func(), to
> > supply an compression-specific error message to pg_fatal(). But it's
> > not being used outside of directory format, nor for errors for other
> > function pointers, or even for all errors in write_func(). Is there
> > some reason why each compression method's write_func() shouldn't call
> > pg_fatal() directly, with its compression-specific message ?
> 
> 
> I think there are a couple more places that might/should call
> get_error_func(). For example ahwrite() in pg_backup_archiver.c now
> simply does
> 
> if (bytes_written != size * nmemb)
> WRITE_ERROR_EXIT;
> 
> but perhaps it should call get_error_func() too. There are probably
> other places that call write_func() and should use get_error_func().

Agreed, calling get_error_func() would be preferable to a fatal error. It
should be the caller of the api who decides how to proceed.

> 
> > - I still think supports_compression() should be renamed, or made into a
> > static function in the necessary file. The main reason is that it's
> > more clear what it indicates - whether compression is "implemented by
> > pgdump" and not whether compression is "supported by this postgres
> > build". It also seems possible that we'd want to add a function
> > called something like supports_compression(), indicating whether the
> > algorithm is supported by the current build. It'd be better if pgdump
> > didn't subjugate that name.
> 
> 
> If we choose to rename this to have pgdump_ prefix, fine with me. But I
> don't think there's a realistic chance of conflict, as it's restricted
> to pgdump header etc. And it's not part of an API, so I guess we could
> rename that in the future if needed.

Agreed, it is very unrealistic that one will include that header file anywhere
but within pg_dump. Also. I think that adding a prefix, "pgdump", "pg_dump",
or similar does not add value and subtracts readability. 

> 
> > - Finally, the "Nothing to do in the default case" comment comes from
> > Michael's commit 5e73a6048:
> > 
> > + /*
> > + * Custom and directory formats are compressed by default with gzip when
> > + * available, not the others.
> > + /
> > + if ((archiveFormat == archCustom || archiveFormat == archDirectory) &&
> > + !user_compression_defined)
> > {
> > #ifdef HAVE_LIBZ
> > - if (archiveFormat == archCustom || archiveFormat == archDirectory)
> > - compressLevel = Z_DEFAULT_COMPRESSION;
> > - else
> > + parse_compress_specification(PG_COMPRESSION_GZIP, NULL,
> > + _spec);
> > +#else
> > + / Nothing to do in the default case */
> > #endif
> > - compressLevel = 0;
> > }
> > 
> > As the comment says: for -Fc and -Fd, the compression is set to zlib, if
> > enabled, and when not otherwise specified by the user.
> > 
> > Before 5e73a6048, this set compressLevel=0 for -Fp and -Ft, and when
> > zlib was unavailable.
> > 
> > But I'm not sure why there's now an empty "#else". I also don't know
> > what "the default case" refers to.
> > 
> > Maybe the best thing here is to move the preprocessor #if, since it's no
> > longer in the middle of a runtime conditional:
> > 
> > #ifdef HAVE_LIBZ
> > + if ((archiveFormat == archCustom || archiveFormat == archDirectory) &&
> > + !user_compression_defined)
> > + parse_compress_specification(PG_COMPRESSION_GZIP, NULL,
> > + _spec);
> > #endif
> > 
> > ...but that elicits a warning about "variable set but not used"...
> 
> 
> Not sure, I need to think about this a bit.

Not having warnings is preferable, isn't it? I can understand the confusion
on the message though. Maybe a phrasing like:
/* Nothing to do for the default case when LIBZ is not available */
is easier to understand.

Cheers,
//Georgios

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

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Friday, February 24th, 2023 at 5:35 AM, Michael Paquier 
 wrote:


> 
> 
> On Thu, Feb 23, 2023 at 07:51:16PM -0600, Justin Pryzby wrote:
> 
> > On Thu, Feb 23, 2023 at 09:24:46PM +0100, Tomas Vondra wrote:
> > 
> > > 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!
> > 
> > A big thanks from me to everyone involved.
> 
> 
> Wow, nice! The APIs are clear to follow.

I am out of words, thank you all so very much. I learned a lot. 

> 
> > I'll send a patch soon. I first submitted patches for that 2 years ago
> > (before PGDG was ready to add zstd).
> > https://commitfest.postgresql.org/31/2888/
> 
> 
> Thanks. It should be straight-forward to see that in 16, I guess.
> --
> Michael

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Friday, January 27th, 2023 at 6:23 PM, Justin Pryzby  
wrote:


> 
> 
> On Thu, Jan 26, 2023 at 12:22:45PM -0600, Justin Pryzby wrote:
> 
> > That commit also added this to pg-dump.c:
> > 
> > + case PG_COMPRESSION_ZSTD:
> > + pg_fatal("compression with %s is not yet supported", "ZSTD");
> > + break;
> > + case PG_COMPRESSION_LZ4:
> > + pg_fatal("compression with %s is not yet supported", "LZ4");
> > + break;
> > 
> > In 002, that could be simplified by re-using the supports_compression()
> > function. (And maybe the same in WriteDataToArchive()?)
> 
> 
> The first patch aims to minimize references to ".gz" and "GZIP" and
> ZLIB. pg_backup_directory.c comments still refers to ".gz". I think
> the patch should ideally change to refer to "the compressed file
> extension" (similar to compress_io.c), avoiding the need to update it
> later.
> 
> I think the file extension stuff could be generalized, so it doesn't
> need to be updated in multiple places (pg_backup_directory.c and
> compress_io.c). Maybe it's useful to add a function to return the
> extension of a given compression method. It could go in compression.c,
> and be useful in basebackup.
> 
> For the 2nd patch:
> 
> I might be in the minority, but I still think some references to "gzip"
> should say "zlib":
> 
> +} GzipCompressorState;
> +
> +/* Private routines that support gzip compressed data I/O */
> +static void
> +DeflateCompressorGzip(ArchiveHandle *AH, CompressorState *cs, bool flush)
> 
> In my mind, three things here are misleading, because it doesn't use
> gzip headers:
> 
> | GzipCompressorState, DeflateCompressorGzip, "gzip compressed".
> 
> This comment is about exactly that:
> 
> * underlying stream. The second API is a wrapper around fopen/gzopen and
> * friends, providing an interface similar to those, but abstracts away
> * the possible compression. Both APIs use libz for the compression, but
> * the second API uses gzip headers, so the resulting files can be easily
> * manipulated with the gzip utility.
> 
> AIUI, Michael says that it's fine that the user-facing command-line
> options use "-Z gzip" (even though the "custom" format doesn't use gzip
> headers). I'm okay with that, as long as that's discussed/understood.
> 

Thank you for the input Justin. I am currently waiting for input from a
third person to get some conclusion. I thought that it should be stated
before my inactiveness is considered as indifference, which is not.

Cheers,
//Georgios

> --
> Justin

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Wednesday, January 25th, 2023 at 7:00 PM, Justin Pryzby 
 wrote:


> 
> 
> On Wed, Jan 25, 2023 at 03:37:12PM +, gkokola...@pm.me wrote:
> 

> While looking at this, I realized that commit 5e73a6048 introduced a
> regression:
> 
> @@ -3740,19 +3762,24 @@ ReadHead(ArchiveHandle *AH)
> 
> - if (AH->compression != 0)
> 
> - pg_log_warning("archive is compressed, but this installation does not 
> support compression -- no data will be available");
> + if (AH->compression_spec.algorithm == PG_COMPRESSION_GZIP)
> 
> + pg_fatal("archive is compressed, but this installation does not support 
> compression");
> 
> Before, it was possible to restore non-data chunks of a dump file, even
> if the current build didn't support its compression. But that's now
> impossible - and it makes the code we're discussing in RestoreArchive()
> unreachable.

Nice catch!

Cheers,
//Georgios

> --
> Justin

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Wednesday, January 25th, 2023 at 6:28 PM, Tomas Vondra 
 wrote:


> 
> 
> 
> On 1/25/23 16:37, gkokola...@pm.me wrote:
> 
> > --- Original Message ---
> > On Wednesday, January 25th, 2023 at 2:42 AM, Justin Pryzby 
> > pry...@telsasoft.com wrote:
> > 
> > > On Tue, Jan 24, 2023 at 03:56:20PM +, gkokola...@pm.me wrote:
> > > 
> > > > On Monday, January 23rd, 2023 at 7:00 PM, Justin Pryzby 
> > > > pry...@telsasoft.com wrote:
> > > > 
> > > > > On Mon, Jan 23, 2023 at 05:31:55PM +, gkokola...@pm.me wrote:
> > > > > 
> > > > > > Please find attached v23 which reintroduces the split.
> > > > > > 
> > > > > > 0001 is reworked to have a reduced footprint than before. Also in 
> > > > > > an attempt
> > > > > > to facilitate the readability, 0002 splits the API's and the 
> > > > > > uncompressed
> > > > > > implementation in separate files.
> > > > > 
> > > > > Thanks for updating the patch. Could you address the review comments I
> > > > > sent here ?
> > > > > https://www.postgresql.org/message-id/20230108194524.GA27637%40telsasoft.com
> > > > 
> > > > Please find v24 attached.
> > > 
> > > Thanks for updating the patch.
> > > 
> > > In 001, RestoreArchive() does:
> > > 
> > > > -#ifndef HAVE_LIBZ
> > > > - if (AH->compression_spec.algorithm == PG_COMPRESSION_GZIP &&
> > > > - AH->PrintTocDataPtr != NULL)
> > > > + supports_compression = false;
> > > > + if (AH->compression_spec.algorithm == PG_COMPRESSION_NONE ||
> > > > + AH->compression_spec.algorithm == PG_COMPRESSION_GZIP)
> > > > + supports_compression = true;
> > > > +
> > > > + if (AH->PrintTocDataPtr != NULL)
> > > > {
> > > > for (te = AH->toc->next; te != AH->toc; te = te->next)
> > > > {
> > > > if (te->hadDumper && (te->reqs & REQ_DATA) != 0)
> > > > - pg_fatal("cannot restore from compressed archive (compression not 
> > > > supported in this installation)");
> > > > + {
> > > > +#ifndef HAVE_LIBZ
> > > > + if (AH->compression_spec.algorithm == PG_COMPRESSION_GZIP)
> > > > + supports_compression = false;
> > > > +#endif
> > > > + if (supports_compression == false)
> > > > + pg_fatal("cannot restore from compressed archive (compression not 
> > > > supported in this installation)");
> > > > + }
> > > > }
> > > > }
> > > > -#endif
> > > 
> > > This first checks if the algorithm is implemented, and then checks if
> > > the algorithm is supported by the current build - that confused me for a
> > > bit. It seems unnecessary to check for unimplemented algorithms before
> > > looping. That also requires referencing both GZIP and LZ4 in two
> > > places.
> > 
> > I am not certain that it is unnecessary, at least not in the way that is
> > described. The idea is that new compression methods can be added, without
> > changing the archive's version number. It is very possible that it is
> > requested to restore an archive compressed with a method not implemented
> > in the current binary. The first check takes care of that and sets
> > supports_compression only for the supported versions. It is possible to
> > enter the loop with supports_compression already set to false, for example
> > because the archive was compressed with ZSTD, triggering the fatal error.
> > 
> > Of course, one can throw the error before entering the loop, yet I think
> > that it does not help the readability of the code. IMHO it is easier to
> > follow if the error is thrown once during that check.
> 
> 
> Actually, I don't understand why 0001 moves the check into the loop. I
> mean, why not check HAVE_LIBZ before the loop?

The intention is to be able to restore archives that don't contain
data. In that case compression becomes irrelevant as only the data in
an archive is compressed.

> 
> > > I think it could be written to avoid the need to change for added
> > > compression algorithms:
> > > 
> > > + if (te->hadDumper && (te->reqs & REQ_DATA) != 0)
> > > 
> > > + {
> > > + /* Check if the compression algorithm is supported */
> > > + pg_compress_specification spec;
> > > + parse_compress_specification(AH->compression_spec.algorithm, NULL, 
> > > );
> > > 
> > > + if (spec->parse_error != NULL)
> > > 
> > > + pg_fatal(spec->parse_error);
> > > 
> > > + }
> > 
> > I am not certain how that would work in the example with ZSTD above.
> > If I am not wrong, parse_compress_specification() will not throw an error
> > if the codebase supports ZSTD, yet this specific pg_dump binary will not
> > support it because ZSTD is not implemented. parse_compress_specification()
> > is not aware of that and should not be aware of it, should it?
> 
> 
> Not sure. What happens in a similar situation now? That is, when trying
> to deal with an archive gzip-compressed in a build without libz?


In case that there are no data chunks, the archive will be restored.

Cheers,
//Georgios


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

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Friday, January 20th, 2023 at 12:34 AM, Tomas Vondra 
 wrote:


> 
> 
> On 1/19/23 18:55, Tomas Vondra wrote:
> 
> > Hi,
> > 
> > On 1/19/23 17:42, gkokola...@pm.me wrote:
> > 
> > > ...
> > > 
> > > Agreed. It was initially submitted as one patch. Then it was requested to 
> > > be
> > > split up in two parts, one to expand the use of the existing API and one 
> > > to
> > > replace with the new interface. Unfortunately the expansion of usage of 
> > > the
> > > existing API requires some tweaking, but that is not a very good reason 
> > > for
> > > the current patch set. I should have done a better job there.
> > > 
> > > Please find v22 attach which combines back 0001 and 0002. It is missing 
> > > the
> > > documentation that was discussed above as I wanted to give a quick 
> > > feedback.
> > > Let me know if you think that the combined version is the one to move 
> > > forward
> > > with.
> > 
> > Thanks, I'll take a look.
> 
> 
> After taking a look and thinking about it a bit more, I think we should
> keep the two parts separate. I think Michael (or whoever proposed) the
> split was right, it makes the patches easier to grok.
> 

Excellent. I will attempt a better split this time round.

> 
> While reading the thread, I also noticed this:
> 
> > By the way, I think that this 0002 should drop all the default clauses
> > in the switches for the compression method so as we'd catch any
> > missing code paths with compiler warnings if a new compression method
> > is added in the future.
> 
> 
> Now I realize why there were "not yet implemented" errors for lz4/zstd
> in all the switches, and why after removing them you had to add a
> default branch.
> 
> We DON'T want a default branch, because the idea is that after adding a
> new compression algorithm, we get warnings about switches not handling
> it correctly.
> 
> So I guess we should walk back this change too :-( It's probably easier
> to go back to v20 from January 16, and redo the couple remaining things
> I commented on.
> 

Sure.

> 
> FWIW I think this is a hint that adding LZ4/ZSTD options, in 5e73a6048,
> but without implementation, was not a great idea. It mostly defeats the
> idea of getting the compiler warnings - all the places already handle
> PG_COMPRESSION_LZ4/PG_COMPRESSION_ZSTD by throwing a pg_fatal. So you'd
> have to grep for the options, inspect all the places or something like
> that anyway. The warnings would only work for entirely new methods.
> 
> However, I now also realize the compressor API in 0002 replaces all of
> this with calls to a generic API callback, so trying to improve this was
> pretty silly from me.

I can try to do a better job at splitting things up.

> 
> Please, fix the couple remaining details in v20, add the docs for the
> callbacks, and I'll try to polish it and get it committed.

Excellent. Allow me an attempt to polish and expect a new version soon.

Cheers,
//Georgios

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

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Thursday, January 19th, 2023 at 4:45 PM, Tomas Vondra 
 wrote:


> 
> 
> On 1/18/23 20:05, gkokola...@pm.me wrote:
> 
> > --- Original Message ---
> > On Wednesday, January 18th, 2023 at 3:00 PM, Tomas Vondra 
> > tomas.von...@enterprisedb.com wrote:
> > 
> > > Hi,
> > > 
> > > On 1/16/23 16:14, gkokola...@pm.me wrote:
> > > 
> > > > Hi,
> > > > 
> > > > I admit I am completely at lost as to what is expected from me anymore.
> > 
> > 
> > 
> > > Unfortunately, this plays against this patch - I'm certainly in favor of
> > > adding lz4 (and other compression algos) into pg_dump, but if I commit
> > > 0001 we get little benefit, and the other parts actually adding lz4/zstd
> > > are treated as "WIP / for completeness" so it's unclear when we'd get to
> > > commit them.
> > 
> > Thank you for your kindness and for taking the time to explain.
> > 
> > > So if I could recommend one thing, it'd be to get at least one of those
> > > WIP patches into a shape that's likely committable right after 0001.
> > 
> > This was clearly my fault. I misunderstood a suggestion upthread to focus
> > on the first patch of the series and ignore documentation and comments on
> > the rest.
> > 
> > Please find v21 to contain 0002 and 0003 in a state which I no longer 
> > consider
> > as WIP but worthy of proper consideration. Some guidance on where is best 
> > to add
> > documentation in 0002 for the function pointers in CompressFileHandle will
> > be welcomed.
> 
> 
> This is internal-only API, not meant for use by regular users and/or
> extension authors, so I don't think we need sgml docs. I'd just add
> regular code-level documentation to compress_io.h.
> 
> For inspiration see docs for "struct ReorderBuffer" in reorderbuffer.h,
> or "struct _archiveHandle" in pg_backup_archiver.h.
> 
> Or what other kind of documentation you had in mind?

This is exactly what I was after. I was between compress_io.c and compress_io.h.
Thank you.

> > > > I had posted v19-0001 for a committer's consideration and v19-000{2,3} 
> > > > for completeness.
> > > > Please find a rebased v20 attached.
> > > 
> > > I took a quick look at 0001, so a couple comments (sorry if some of this
> > > was already discussed in the thread):
> > 
> > Much appreciated!
> > 
> > > 1) I don't think a "refactoring" patch should reference particular
> > > compression algorithms (lz4/zstd), and in particular I don't think we
> > > should have "not yet implemented" messages. We only have a couple other
> > > places doing that, when we didn't have a better choice. But here we can
> > > simply reject the algorithm when parsing the options, we don't need to
> > > do that in a dozen other places.
> > 
> > I have now removed lz4/zstd from where they were present with the exception
> > of pg_dump.c which is responsible for parsing.
> 
> 
> I'm not sure I understand why leave the lz4/zstd in this place?

You are right, it is not obvious. Those were added in 5e73a60488 which is
already committed in master and I didn't want to backtrack. Of course, I am
not opposing in doing so if you wish.

> 
> > > 2) I wouldn't reorder the cases in WriteDataToArchive, i.e. I'd keep
> > > "none" at the end. It might make backpatches harder.
> > 
> > Agreed. However a 'default' is needed in order to avoid compilation 
> > warnings.
> > Also note that 0002 completely does away with cases within 
> > WriteDataToArchive.
> 
> 
> OK, although that's also a consequence of using a "switch" instead of
> plan "if" branches.
> 
> Furthermore, I'm not sure we really need the pg_fatal() about invalid
> compression method in these default blocks. I mean, how could we even
> get to these places when the build does not support the algorithm? All
> of this (ReadDataFromArchive, WriteDataToArchive, EndCompressor, ...)
> happens looong after the compressor was initialized and the method
> checked, no? So maybe either this should simply do Assert(false) or use
> a different error message.

I like Assert(false).

> > > 3) While building, I get bunch of warnings about missing cfdopen()
> > > prototype and pg_backup_archiver.c not knowing about cfdopen() and
> > > adding an implicit prototype (so I doubt it actually works).
> > 
> > Fixed. cfdopen() got prematurely introduced in 5e73a6048 and then got 
> > removed
> > in 69fb29d1af. v20 failed to properly take 69fb29d1af in consideration. Note
> > that cfdopen is removed in 0002 which explains why cfbot didn't complain.
> 
> 
> OK.
> 
> > > 4) "cfp" struct no longer wraps gzFile, but the comment was not updated.
> > > FWIW I'm not sure switching to "void *" is an improvement, maybe it'd be
> > > better to have a "union" of correct types?
> > 
> > Please find and updated comment and a union in place of the void *. Also
> > note that 0002 completely does away with cfp in favour of a new struct
> > CompressFileHandle. I maintained the void * there because it is used by
> > private methods of the compressors. 0003 

Re: Add LZ4 compression in pg_dump

[gkokolatos]

Oh, I didn’t realize you took over Justin? Why? After almost a year of work?

This is rather disheartening.

On Mon, Jan 16, 2023 at 02:56, Justin Pryzby  wrote:

> On Mon, Jan 16, 2023 at 10:28:50AM +0900, Michael Paquier wrote:
>> On Sat, Jan 14, 2023 at 03:43:09PM -0600, Justin Pryzby wrote:
>> > On Sun, Jan 08, 2023 at 01:45:25PM -0600, Justin Pryzby wrote:
>> >> pg_compress_specification is being passed by value, but I think it
>> >> should be passed as a pointer, as is done everywhere else.
>> >
>> > ISTM that was an issue with 5e73a6048, affecting a few public and
>> > private functions. I wrote a pre-preparatory patch which changes to
>> > pass by reference.
>>
>> The functions changed by 0001 are cfopen[_write](),
>> AllocateCompressor() and ReadDataFromArchive(). Why is it a good idea
>> to change these interfaces which basically exist to handle inputs?
>
> I changed to pass pg_compress_specification as a pointer, since that's
> the usual convention for structs, as followed by the existing uses of
> pg_compress_specification.
>
>> Is there some benefit in changing compression_spec within the
>> internals of these routines before going back one layer down to their
>> callers? Changing the compression_spec on-the-fly in these internal
>> paths could be risky, actually, no?
>
> I think what you're saying is that if the spec is passed as a pointer,
> then the called functions shouldn't set spec->algorithm=something.
>
> I agree that if they need to do that, they should use a local variable.
> Which looks to be true for the functions that were changed in 001.
>
>> > And addressed a handful of other issues I reported as separate fixup
>> > commits. And changed to use LZ4 by default for CI.
>>
>> Are your slight changes shaped as of 0003-f.patch, 0005-f.patch and
>> 0007-f.patch on top of the original patches sent by Georgios?
>
> Yes, the original patches, rebased as needed on top of HEAD and 001...
>
>> >> pg_compress_algorithm is being writen directly into the pg_dump header.
>>
>> Do you mean that this is what happens once the patch series 0001~0008
>> sent upthread is applied on HEAD?
>
> Yes
>
>> - /*
>> - * For now the compression type is implied by the level. This will need
>> - * to change once support for more compression algorithms is added,
>> - * requiring a format bump.
>> - */
>> - WriteInt(AH, AH->compression_spec.level);
>> + AH->WriteBytePtr(AH, AH->compression_spec.algorithm);
>>
>> I may be missing something here, but it seems to me that you ought to
>> store as well the level in the dump header, or it would not be
>> possible to report in the dump's description what was used? Hence,
>> K_VERS_1_15 should imply that we have both the method compression and
>> the compression level.
>
> Maybe. But the "level" isn't needed for decompression for any case I'm
> aware of.
>
> Also, dumps with the default compression level currently say:
> "Compression: -1", which does't seem valuable.
>
> --
> Justin

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Monday, December 19th, 2022 at 6:27 PM, Justin Pryzby  
wrote:


> 
> 
> On Mon, Dec 19, 2022 at 05:03:21PM +, gkokola...@pm.me wrote:
> 
> > > > 001 still doesn't compile on freebsd, and 002 doesn't compile on
> > > > windows. Have you checked test results from cirrusci on your private
> > > > github account ?
> > 
> > There are still known gaps in 0002 and 0003, for example documentation,
> > and I have not been focusing too much on those. You are right, it is helpful
> > and kind to try to reduce the noise. The attached should have hopefully
> > tackled the ci errors.
> 
> 
> Yep. Are you using cirrusci under your github account ?

Thank you. To be very honest, I am not using github exclusively to post patches.
Sometimes I do, sometimes I do not. Is github a requirement?

To answer your question, some of my github accounts are integrated with 
cirrusci,
others are not.

The current cfbot build is green for what is worth.
https://cirrus-ci.com/build/5934319840002048

> 
> > > FYI, I have re-added an entry to the CF app to get some automated
> > > coverage:
> > > https://commitfest.postgresql.org/41/3571/
> > 
> > Much obliged. Should I change the state to "ready for review" when post a
> > new version or should I leave that to the senior personnel?
> 
> 
> It's better to update it to reflect what you think its current status
> is. If you think it's ready for review.

Thank you.

> 
> > > > 002 breaks "pg_dump -Fc -Z2" because (I think) AllocateCompressor()
> > > > doesn't store the passed-in compression_spec.
> > 
> > I am afraid I have not been able to reproduce this error. I tried both
> > debian and freebsd after I addressed the compilation warnings. Which
> > error did you get? Is it still present in the attached?
> 
> 
> It's not that there's an error - it's that compression isn't working.
> 
> $ ./tmp_install/usr/local/pgsql/bin/pg_dump -h /tmp -Z1 -Fp regression |wc -c
> 659956
> $ ./tmp_install/usr/local/pgsql/bin/pg_dump -h /tmp -Z2 -Fp regression |wc -c
> 637192
> 
> $ ./tmp_install/usr/local/pgsql/bin/pg_dump -h /tmp -Z1 -Fc regression |wc -c
> 1954890
> $ ./tmp_install/usr/local/pgsql/bin/pg_dump -h /tmp -Z2 -Fc regression |wc -c
> 1954890
> 

Thank you. Now I understand what you mean. Trying the same on top of v18-0003
on Ubuntu 22.04 yields:

$ for compression in none gzip:1 gzip:6 gzip:9; do \
  pg_dump --format=custom --compress="$compression" -f 
regression."$compression".dump -d regression; \
  wc -c regression."$compression".dump; \
  done;
14963753 regression.none.dump
3600183 regression.gzip:1.dump
3223755 regression.gzip:6.dump
3196903 regression.gzip:9.dump

and on FreeBSD 13.1

$ for compression in none gzip:1 gzip:6 gzip:9; do \
  pg_dump --format=custom --compress="$compression" -f 
regression."$compression".dump -d regression; \
  wc -c regression."$compression".dump; \
  done;
14828822 regression.none.dump
3584304 regression.gzip:1.dump
3208548 regression.gzip:6.dump
3182044 regression.gzip:9.dump

Although there are some variations between the installations, within the same
installation the size of the dump file is shrinking as expected.

Investigating a bit further on the issue, you are correct in identifying an
issue in v17. Up until v16, the compressor function looked like:

+InitCompressorGzip(CompressorState *cs, int compressionLevel)
+{
+   GzipCompressorState *gzipcs;
+
+   cs->readData = ReadDataFromArchiveGzip;
+   cs->writeData = WriteDataToArchiveGzip;
+   cs->end = EndCompressorGzip;
+
+   gzipcs = (GzipCompressorState *) 
pg_malloc0(sizeof(GzipCompressorState));
+   gzipcs->compressionLevel = compressionLevel;

V17 considered that more options could become available in the future
and changed the signature of the relevant Init functions to:

+InitCompressorGzip(CompressorState *cs, const pg_compress_specification 
compression_spec)
+{
+   GzipCompressorState *gzipcs;
+
+   cs->readData = ReadDataFromArchiveGzip;
+   cs->writeData = WriteDataToArchiveGzip;
+   cs->end = EndCompressorGzip;
+
+   gzipcs = (GzipCompressorState *) 
pg_malloc0(sizeof(GzipCompressorState));
+

V18 reinstated the assignment in similar fashion to InitCompressorNone and 
InitCompressorLz4:

+void
+InitCompressorGzip(CompressorState *cs, const pg_compress_specification 
compression_spec)
+{
+   GzipCompressorState *gzipcs;
+
+   cs->readData = ReadDataFromArchiveGzip;
+   cs->writeData = WriteDataToArchiveGzip;
+   cs->end = EndCompressorGzip;
+
+   cs->compression_spec = compression_spec;
+
+   gzipcs = (GzipCompressorState *) 
pg_malloc0(sizeof(GzipCompressorState));

A test case can be added which performs a check similar to the loop above.
Create a custom dump with the least and most compression for each method.
Then verify that the output sizes differ as expected. This addition could
become 0001 in the current series.

Thoughts?

Cheers,
//Georgios

> --
> 

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Thursday, December 1st, 2022 at 3:05 AM, Michael Paquier 
 wrote:


> 
> 
> On Wed, Nov 30, 2022 at 05:11:44PM +, gkokola...@pm.me wrote:
> 
> > Fair enough. The atteched v11 does that. 0001 introduces compression
> > specification and is using it throughout. 0002 paves the way to the
> > new interface by homogenizing the use of cfp. 0003 introduces the new
> > API and stores the compression algorithm in the custom format header
> > instead of the compression level integer. Finally 0004 adds support for
> > LZ4.
> 
> 
> I have been looking at 0001, and.. Hmm. I am really wondering
> whether it would not be better to just nuke this warning into orbit.
> This stuff enforces non-compression even if -Z has been used to a
> non-default value. This has been moved to its current location by
> cae2bb1 as of this thread:
> https://www.postgresql.org/message-id/20160526.185551.242041780.horiguchi.kyotaro%40lab.ntt.co.jp
> 
> However, this is only active if -Z is used when not building with
> zlib. At the end, it comes down to whether we want to prioritize the
> portability of pg_dump commands specifying a -Z/--compress across
> environments knowing that these may or may not be built with zlib,
> vs the amount of simplification/uniformity we would get across the
> binaries in the tree once we switch everything to use the compression
> specifications. Now that pg_basebackup and pg_receivewal are managed
> by compression specifications, and that we'd want more compression
> options for pg_dump, I would tend to do the latter and from now on
> complain if attempting to do a pg_dump -Z under --without-zlib with a
> compression level > 0. zlib is also widely available, and we don't
> document the fact that non-compression is enforced in this case,
> either. (Two TAP tests with the custom format had to be tweaked.)

Fair enough. Thank you for looking. However I have a small comment
on your new patch.

-   /* Custom and directory formats are compressed by default, others not */
-   if (compressLevel == -1)
-   {
-#ifdef HAVE_LIBZ
-   if (archiveFormat == archCustom || archiveFormat == 
archDirectory)
-   compressLevel = Z_DEFAULT_COMPRESSION;
-   else
-#endif
-   compressLevel = 0;
-   }


Nuking the warning from orbit and changing the behaviour around disabling
the requested compression when the libraries are not present, should not
mean that we need to change the behaviour of default values for different
formats. Please find v13 attached which reinstates it.

Which in itself it got me looking and wondering why the tests succeeded.
The only existing test covering that path is `defaults_dir_format` in
`002_pg_dump.pl`. However as the test is currently written it does not
check whether the output was compressed. The restore command would succeed
in either case. A simple `gzip -t -r` against the directory will not
suffice to test it, because there exist files which are never compressed
in this format (.toc). A little bit more involved test case would need
to be written, yet before I embark to this journey, I would like to know
if you would agree to reinstate the defaults for those formats.

> 
> As per the patch, it is true that we do not need to bump the format of
> the dump archives, as we can still store only the compression level
> and guess the method from it. I have added some notes about that in
> ReadHead and WriteHead to not forget.

Agreed. A minor suggestion if you may.

 #ifndef HAVE_LIBZ
-   if (AH->compression != 0)
+   if (AH->compression_spec.algorithm == PG_COMPRESSION_GZIP)
pg_log_warning("archive is compressed, but this installation 
does not support compression -- no data will be available");
 #endif

It would seem a more consistent to error out in this case. We do error
in all other cases where the compression is not available.

> 
> Most of the changes are really-straight forward, and it has resisted
> my tests, so I think that this is in a rather-commitable shape as-is.

Thank you.

Cheers,
//Georgios

> --
> MichaelFrom 16e10b38cc8eb6eb5b1ffc15365d7e6ce23eef0a Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Thu, 1 Dec 2022 08:58:51 +
Subject: [PATCH v13] Teach pg_dump about compress_spec and use it throughout.

Align pg_dump with the rest of the binaries which use common compression. It is
teaching pg_dump.c about the common compression definitions and interfaces. Then
it propagates those throughout the code.
---
 doc/src/sgml/ref/pg_dump.sgml   |  34 +--
 src/bin/pg_dump/compress_io.c   | 107 
 src/bin/pg_dump/compress_io.h   |  20 ++--
 src/bin/pg_dump/pg_backup.h |   7 +-
 src/bin/pg_dump/pg_backup_archiver.c|  78 +-
 src/bin/pg_dump/pg_backup_archiver.h|  10 +-
 src/bin/pg_dump/pg_backup_custom.c  |   6 +-
 

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Monday, November 21st, 2022 at 12:13 AM, Michael Paquier 
 wrote:


> 
> 
> On Sun, Nov 20, 2022 at 11:26:11AM -0600, Justin Pryzby wrote:
> 
> > I think this patch record should be closed for now. You can re-open the
> > existing patch record once a patch is ready to be reviewed.
> 
> 
> Indeed. As of things are, this is just a dead entry in the CF which
> would be confusing. I have marked it as RwF.

Thank you for closing it.

For the record I am currently working on it simply unsure if I should submit
WIP patches and add noise to the list or wait until it is in a state that I
feel that the comments have been addressed.

A new version that I feel that is in a decent enough state for review should
be ready within this week. I am happy to drop the patch if you think I should
not work on it though.

Cheers,
//Georgios

> --
> Michael

Re: Add LZ4 compression in pg_dump

[gkokolatos]

On Wed, Nov 2, 2022 at 14:28, Justin Pryzby  wrote:

> Checking if you'll be able to submit new patches soon ?

Thank you for checking up. Expect new versions within this commitfest cycle.

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---
On Sunday, June 26th, 2022 at 5:55 PM, Justin Pryzby  
wrote:


>
>
> Hi,
>
> Will you be able to send a rebased patch for the next CF ?

Thank you for taking an interest in the PR. The plan is indeed to sent
a new version.

> If you update for the review comments I sent in March, I'll plan to do another
> round of review.

Thank you.

>
> On Sat, Mar 26, 2022 at 11:21:56AM -0500, Justin Pryzby wrote:
>
> > LZ4F_HEADER_SIZE_MAX isn't defined in old LZ4.
> >
> > I ran into that on an ubuntu LTS, so I don't think it's so old that it
> > shouldn't be handled more gracefully. LZ4 should either have an explicit
> > version check, or else shouldn't depend on that feature (or should define a
> > safe fallback version if the library header doesn't define it).
> >
> > https://packages.ubuntu.com/liblz4-1
> >
> > 0003: typo: of legacy => or legacy
> >
> > There are a large number of ifdefs being added here - it'd be nice to 
> > minimize
> > that. basebackup was organized to use separate files, which is one way.
> >
> > $ git grep -c 'ifdef .*LZ4' src/bin/pg_dump/compress_io.c
> > src/bin/pg_dump/compress_io.c:19
> >
> > In last year's CF entry, I had made a union within CompressorState. LZ4
> > doesn't need z_streamp (and ztsd will need ZSTD_outBuffer, ZSTD_inBuffer,
> > ZSTD_CStream).
> >
> > 0002: I wonder if you're able to re-use any of the basebackup parsing stuff
> > from commit ffd53659c. You're passing both the compression method and level.
> > I think there should be a structure which includes both. In the future, that
> > can also handle additional options. I hope to re-use these same things for
> > wal_compression=method:level.
> >
> > You renamed this:
> >
> > |- COMPR_ALG_LIBZ
> > |-} CompressionAlgorithm;
> > |+ COMPRESSION_GZIP,
> > |+} CompressionMethod;
> >
> > ..But I don't think that's an improvement. If you were to change it, it 
> > should
> > say something like PGDUMP_COMPRESS_ZLIB, since there are other compression
> > structs and typedefs. zlib is not idential to gzip, which uses a different
> > header, so in WriteDataToArchive(), LIBZ is correct, and GZIP is incorrect.
> >
> > The cf* changes in pg_backup_archiver could be split out into a separate
> > commit. It's strictly a code simplification - not just preparation for more
> > compression algorithms. The commit message should "See also:
> > bf9aa490db24b2334b3595ee33653bf2fe39208c".
> >
> > The changes in 0002 for cfopen_write seem insufficient:
> > |+ if (compressionMethod == COMPRESSION_NONE)
> > |+ fp = cfopen(path, mode, compressionMethod, 0);
> > | else
> > | {
> > | #ifdef HAVE_LIBZ
> > | char *fname;
> > |
> > | fname = psprintf("%s.gz", path);
> > |- fp = cfopen(fname, mode, compression);
> > |+ fp = cfopen(fname, mode, compressionMethod, compressionLevel);
> > | free_keep_errno(fname);
> > | #else
> >
> > The only difference between the LIBZ and uncompressed case is the file
> > extension, and it'll be the only difference with LZ4 too. So I suggest to
> > first handle the file extension, and the rest of the code path is not
> > conditional on the compression method. I don't think cfopen_write even needs
> > HAVE_LIBZ - can't you handle that in cfopen_internal() ?
> >
> > This patch rejects -Z0, which ought to be accepted:
> > ./src/bin/pg_dump/pg_dump -h /tmp regression -Fc -Z0 |wc
> > pg_dump: error: can only specify -Z/--compress [LEVEL] when method is set
> >
> > Your 0003 patch shouldn't reference LZ4:
> > +#ifndef HAVE_LIBLZ4
> > + if (*compressionMethod == COMPRESSION_LZ4)
> > + supports_compression = false;
> > +#endif
> >
> > The 0004 patch renames zlibOutSize to outsize - I think the patch series 
> > should
> > be constructed such as to minimize the size of the method-specific patches. 
> > I
> > say this anticipating also adding support for zstd. The preliminary patches
> > should have all the boring stuff. It would help for reviewing to keep the
> > patches split up, or to enumerate all the boring things that are being 
> > renamed
> > (like change OutputContext to cfp, rename zlibOutSize, ...).
> >
> > 0004: The include should use  and not "lz4.h"
> >
> > freebsd/cfbot is failing.
> >
> > I suggested off-list to add an 0099 patch to change LZ4 to the default, to
> > exercise it more on CI.
>
>
> On Sat, Mar 26, 2022 at 01:33:36PM -0500, Justin Pryzby wrote:
>
> > On Sat, Mar 26, 2022 at 11:21:56AM -0500, Justin Pryzby wrote:
> >
> > > You're passing both the compression method and level. I think there should
> > > be a structure which includes both. In the future, that can also handle
> > > additional options.
> >
> > I'm not sure if there's anything worth saving, but I did that last year with
> > 0003-Support-multiple-compression-algs-levels-opts.patch
> > I sent a rebased copy off-list.
> > https://www.postgresql.org/message-id/flat/20210104025321.ga9...@telsasoft.com#ca1b9f9d3552c87fa874731cad9d8391
> >
> > | fatal("not built with LZ4 support");
> > | fatal("not built with 

Re: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}

[gkokolatos]

--- Original Message ---
On Wednesday, April 13th, 2022 at 7:25 AM, Michael Paquier 
 wrote:


>
>
> On Tue, Apr 12, 2022 at 06:22:48PM +0900, Michael Paquier wrote:
>
> > On Mon, Apr 11, 2022 at 12:46:02PM +, gkokola...@pm.me wrote:
> >
> > > It looks good. If you choose to discard the comment regarding the use of
> > > 'method' over 'algorithm' from above, can you please use the full word in 
> > > the
> > > variable, e.g. 'wal_compress_algorithm' instead of 'wal_compress_algo'. I 
> > > can
> > > not really explain it, the later reads a bit rude. Then again that may be 
> > > just
> > > me.
> >
> > Thanks. I have been able to do an extra pass on 0001 and 0002, fixing
> > those naming inconsistencies with "algo" vs "algorithm" that you and
> > Robert have reported, and applied them. For 0003, I'll look at it
> > later. Attached is a rebase with improvements about the variable
> > names.
>
> This has been done with the proper renames. With that in place, I see
> no reason now to not be able to set the compression level as it is
> possible to pass it down with the options available. This requires
> only a couple of lines, as of the attached. LZ4 has a dummy structure
> called LZ4F_INIT_PREFERENCES to initialize LZ4F_preferences_t, that
> holds the compression level before passing it down to
> LZ4F_compressBegin(), but that's available only in v1.8.3. Using it
> risks lowering down the minimal version of LZ4 we are able to use now,
> but replacing that with a set of memset()s is also a way to set up
> things as per its documentation.
>
> Thoughts?

It's really not hard to add compression level. However we had briefly
discussed it in the original thread [1] and decided against. That is why
I did not write that code. If the community thinks differently now, let
me know if you would like me to offer a patch for it.

Cheers,
//Georgios


[1] 
https://www.postgresql.org/message-id/flat/CABUevEwuq7XXyd4fA0W3jY9MsJu9B2WRbHumAA%2B3WzHrGAQjsg%40mail.gmail.com#b6456fa2adc1cdb049a57bf3587666b9

Re: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}

[gkokolatos]

On Monday, April 11th, 2022 at 8:52 AM, Michael Paquier 
wrote:

> This is something I think we had better fix before beta1, because now
> we have binaries that use an inconsistent set of options. So,
> attached is a patch set aimed at rework this option set from the
> ground, taking advantage of the recent work done by Robert and others
> for pg_basebackup:

Agreed. It is rather inconsistent now.

> - 0001 is a simple rename of backup_compression.{c,h} to
> compression.{c,h}, removing anything related to base backups from
> that. One extra reason behind this renaming is that I would like to
> use this infra for pg_dump, but that's material for 16~.

I agree with the design. If you permit me a couple of nitpicks regarding naming.

+typedef enum pg_compress_algorithm
+{
+   PG_COMPRESSION_NONE,
+   PG_COMPRESSION_GZIP,
+   PG_COMPRESSION_LZ4,
+   PG_COMPRESSION_ZSTD
+} pg_compress_algorithm;

Elsewhere in the codebase, (e.g. create_table.sgml, alter_table.sgml,
brin_tuple.c, detoast.c, toast_compression.c, tupdesc.c, gist.c to mention a
few) variations of of the nomenclature "compression method" are used, like
'VARDATA_COMPRESSED_GET_COMPRESS_METHOD' or 'InvalidCompressionMethod' etc. I
feel that it would be nicer if we followed one naming rule for this and I
recommend to substitute algorithm for method throughout.

On a similar note, it would help readability to be able to distinguish at a
glance the type from the variable. Maybe uppercase or camelcase the type?

Last, even though it is not needed now, it will be helpful to have a
PG_COMPRESSION_INVALID in some scenarios. Though we can add it when we come to
it.

> - 0002 removes WalCompressionMethod, replacing it by
> pg_compress_algorithm as these are the same enums. Robert complained
> about the confusion that WalCompressionMethod could lead to as this
> could be used for the compression of data, and not only WAL. I have
> renamed some variables to be more consistent, while on it.

It looks good. If you choose to discard the comment regarding the use of
'method' over 'algorithm' from above, can you please use the full word in the
variable, e.g. 'wal_compress_algorithm' instead of 'wal_compress_algo'. I can
not really explain it, the later reads a bit rude. Then again that may be just
me.

> - 0003 is the actual option rework for pg_receivewal. This takes
> advantage of 0001, leading to the result of removing --compress-method
> and replacing it with --compress, taking care of the backward
> compatibility problems for an integer value, aka 0 implies no
> compression and val > 0 implies gzip. One bonus reason to switch to
> that is that this would make the addition of zstd for pg_receivewal
> easier in the future.

Looks good.

> I am going to add an open item for this stuff. Comments or thoughts?

I agree that it is better to not release pg_receivewal with the distinct set of
options.

Cheers,
//Georgios

> Thanks,
> --
> Michael

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---

On Tuesday, April 5th, 2022 at 12:55 PM, Michael Paquier  
wrote:

> On Tue, Apr 05, 2022 at 07:13:35AM +, gkokola...@pm.me wrote:
> No need to carry it forward anymore, I think ;)

Thank you for committing!

Cheers,
//Georgios

> --
> Michael

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---

On Tuesday, April 5th, 2022 at 3:34 AM, Michael Paquier  
wrote:
> On Fri, Apr 01, 2022 at 03:06:40PM +, gkokola...@pm.me wrote:

> Splitting the program and its arguments makes sense.

Great.

> At the end I am finishing with the attached. I also saw an overlap
> with the addition of --jobs for the directory format vs not using the
> option, so I have removed the case where --jobs was not used in the
> directory format.

Thank you. I agree with the attached and I will carry it forward to the
rest of the patchset.

Cheers,
//Georgios

> --
> Michael

Re: Add LZ4 compression in pg_dump

[gkokolatos]

On Thursday, March 31st, 2022 at 4:34 AM, Michael Paquier  
wrote:
> On Wed, Mar 30, 2022 at 03:32:55PM +, gkokola...@pm.me wrote:
> > On Wednesday, March 30th, 2022 at 7:54 AM, Michael Paquier 
> > mich...@paquier.xyz wrote:
>
> Okay. 0002 looks fine as-is, and I don't mind the extra fatal()
> calls. These could be asserts but that's not a big deal one way or
> the other. And the cleanup is now applied.

Thank you very much.

> > > + my $compress_program = $ENV{GZIP_PROGRAM};
> > > It seems to me that it is enough to rely on {compress_cmd}, hence
> > > there should be no need for $compress_program, no?
> >
> > Maybe not. We don't want to the tests to fail if the utility is not
> > installed. That becomes even more evident as more methods are added.
> > However I realized that the presence of the environmental variable does
> > not guarrantee that the utility is actually installed. In the attached,
> > the existance of the utility is based on the return value of system_log().
>
> Hmm. [.. thinks ..] The thing that's itching me here is that you
> align the concept of compression with gzip, but that's not going to be
> true once more compression options are added to pg_dump, and that
> would make $supports_compression and $compress_program_exists
> incorrect. Perhaps the right answer would be to rename all that with
> a suffix like "_gzip" to make a difference? Or would there be enough
> control with a value of "compression_gzip" instead of "compression" in
> test_key?

I understand the itch. Indeed when LZ4 is added as compression method, this
block changes slightly. I went with the minimum amount changed. Please find
in 0001 of the attached this variable renamed as $gzip_program_exist. I thought
that as prefix it will match better the already used $ENV{GZIP_PROGRAM}.

> +my $compress_program_exists = (system_log("$ENV{GZIP_PROGRAM}", '-h',
> + '>', '/dev/null') == 0);
>
> Do we need this command execution at all? In all the other tests, we
> rely on a simple "if (!defined $gzip || $gzip eq '');", so we could do
> the same here.

You are very correct that we are using the simple version, and that is what
it was included in the previous versions of the current patch. However, I
did notice that the variable is hard-coded in Makefile.global.in and it does
not go through configure. By now, gzip is considered an essential package
in most installations, and this hard-code makes sense. Though I did remove
the utility from my system, (apt remove gzip) and tried the test with the
simple "if (!defined $gzip || $gzip eq '');", which predictably failed. For
this, I went with the system call, it is not too expensive and is rather
reliable.

It is true that the rest of the TAP tests that use this, e.g. in pg_basebackup,
also failed. There is an argument to go simple and I will be happy to revert
to the previous version.

> A last thing is that we should perhaps make a clear difference between
> the check that looks at if the code has been built with zlib and the
> check for the presence of GZIP_PROGRAM, as it can be useful in some
> environments to be able to run pg_dump built with zlib, even if the
> GZIP_PROGRAM command does not exist (I don't think this can be the
> case, but other tests are flexible).

You are very correct. We do that already in the current patch. Note that we skip
the test only when we specifically have to execute a compression command. Not
all compression tests define such command, exactly so that we can test those
cases as well. The point of using an external utility program is in order to
extend the coverage in previously untested yet supported scenarios, e.g. manual
compression of the *.toc files.

Also in the case where it will actually skip the compression command because the
gzip program is not present, it will execute the pg_dump command first.

> As of now, the patch relies on
> pg_dump enforcing uncompression if building under --without-zlib even
> if --compress/-Z is used, but that also means that those compression
> tests overlap with the existing tests in this case. Wouldn't it be
> more consistent to check after $supports_compression when executing
> the dump command for test_key = "compression[_gzip]"? This would mean
> keeping GZIP_PROGRAM as sole check when executing the compression
> command.

I can see the overlap case. Yet, I understand the test_key as serving different
purpose, as it is a key of %tests and %full_runs. I do not expect the database
content of the generated dump to change based on which compression method is 
used.

In the next round, I can see one explitcly requesting --compress=none to 
override
defaults. There is a benefit to group the tests for this scenario under the same
test_key, i.e. compression.

Also there will be cases where if the program exists, yet the codebase is 
compiled
without support for the method. Then compress_cmd or the restore_cmd that 
follows
will fail. For example, in the plain output, if we try to uncompress the 
generated
the test 

Re: Add LZ4 compression in pg_dump

[gkokolatos]

--- Original Message ---

On Saturday, March 26th, 2022 at 12:13 AM, Rachel Heaton 
 wrote:

> On Fri, Mar 25, 2022 at 6:22 AM Justin Pryzby pry...@telsasoft.com wrote:
>
> > On Fri, Mar 25, 2022 at 01:20:47AM -0400, Greg Stark wrote:
> >
> > > It seems development on this has stalled. If there's no further work
> > > happening I guess I'll mark the patch returned with feedback. Feel
> > > free to resubmit it to the next CF when there's progress.

We had some progress yet we didn't want to distract the list with too many
emails. Of course, it seemed stalled to the outside observer, yet I simply
wanted to set the record straight and say that we are actively working on it.

> >
> > Since it's a reasonably large patch (and one that I had myself started 
> > before)
> > and it's only been 20some days since (minor) review comments, and since the
> > focus right now is on committing features, and not reviewing new patches, 
> > and
> > this patch is new one month ago, and its 0002 not intended for pg15, 
> > therefor
> > I'm moving it to the next CF, where I hope to work with its authors to 
> > progress
> > it.

Thank you. It is much appreciated. We will sent updates when the next commitfest
starts in July as to not distract from the 15 work. Then, we can take it from 
there.

>
> Hi Folks,
>
> Here is an updated patchset from Georgios, with minor assistance from myself.
> The comments above should be addressed, but please let us know if

A small amendment to the above statement. This patchset does not include the
refactoring of compress_io suggested by Mr Paquier in the same thread, as it is
missing documentation. An updated version will be sent to include those changes
on the next commitfest.

> there are other things to go over. A functional change in this
> patchset is when `--compress=none` is passed to pg_dump, it will not
> compress for directory type (previously, it would use gzip if
> present). The previous default behavior is retained.
>
> - Rachel

Plug minor memleak in pg_dump

[gkokolatos]

Hi,

I noticed a minor memleak in pg_dump. ReadStr() returns a malloc'ed pointer 
which
should then be freed. While reading the Table of Contents, it was called as an 
argument
within a function call, leading to a memleak.

Please accept the attached as a proposed fix.

Cheers,

//GeorgiosFrom 9473d1106c2816550094f5dea939fc3c6a469f4d Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Tue, 1 Feb 2022 13:08:48 +
Subject: [PATCH v1] Plug minor leak while reading Table of Contents

ReadStr() returns a malloc'ed pointer. Using it directly in a function
call results in a memleak. Rewrite to use a temporary buffer which is
then freed.
---
 src/bin/pg_dump/pg_backup_archiver.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 49bf090..b2d7d21 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2574,8 +2574,15 @@ ReadToc(ArchiveHandle *AH)
 			te->tableam = ReadStr(AH);
 
 		te->owner = ReadStr(AH);
-		if (AH->version < K_VERS_1_9 || strcmp(ReadStr(AH), "true") == 0)
+		if (AH->version < K_VERS_1_9)
 			pg_log_warning("restoring tables WITH OIDS is not supported anymore");
+		else
+		{
+			tmp = ReadStr(AH);
+			if (strcmp(tmp, "true") == 0)
+pg_log_warning("restoring tables WITH OIDS is not supported anymore");
+			pg_free(tmp);
+		}
 
 		/* Read TOC entry dependencies */
 		if (AH->version >= K_VERS_1_5)
-- 
2.34.1

Re: Refactoring of compression options in pg_basebackup

[gkokolatos]

On Wednesday, January 5th, 2022 at 9:00 AM, Michael Paquier 
 wrote:

> On Mon, Jan 03, 2022 at 03:35:57PM +, gkokola...@pm.me wrote:
> > I propose to initialize streamer to NULL when declared at the top of
> > CreateBackupStreamer().
>
> Yes, that may be noisy.  Done this way.

Great.

> > You can see that after the check_pg_config() test, only 3 tests follow,
> > namely:
> >  *  $node->command_ok()
> >  *  is(scalar(), ...)
> >  *  is($gzip_is_valid, ...)
>
> Indeed.  The CF bot was complaining about that, actually.

Great.

> Thinking more about this stuff, pg_basebackup --compress is an option
> that exists already for a couple of years, and that's independent of
> the backend-side compression that Robert and Jeevan are working on, so
> I'd like to move on this code cleanup.  We can always figure out the
> LZ4 part for pg_basebackup after, if necessary.

I agree that the cleanup in itself is helpful. It feels awkward to have two
utilities under the same path, with distinct options for the same
functionality.

When the backend-side compression is completed, were there really be a need for
client-side compression? If yes, then it seems logical to have distinct options
for them and this cleanup makes sense. If not, then it seems logical to maintain
the current options list and 'simply' change the internals of the code, and this
cleanup makes sense.

> Attached is an updated patch.  The CF bot should improve with that.

+1

> --
> Michael

Cheers,
//Georgios

Re: Refactoring of compression options in pg_basebackup

[gkokolatos]

Hi,

‐‐‐ Original Message ‐‐‐
On Saturday, December 18th, 2021 at 12:29 PM, Michael Paquier
 wrote:
>Hi all,
>(Added Georgios in CC.)

thank you for the shout out.

>When working on the support of LZ4 for pg_receivewal, walmethods.c has
>gained an extra parameter for the compression method.  It gets used on
>the DirectoryMethod instead of the compression level to decide which
>type of compression is used.  One thing that I left out during this
>previous work is that the TarMethod also gained knowledge of this
>compression method, but we still use the compression level to check if
>tars should be compressed or not.
>
>This is wrong on multiple aspects.  First, this is not consistent with
>the directory method, making walmethods.c harder to figure out.
>Second, this is not extensible if we want to introduce more
>compression methods in pg_basebackup, like LZ4.  This reflects on the
>options used by pg_receivewal and pg_basebackup, that are not
>inconsistent as well.

Agreed with all the above.

>The attached patch refactors the code of pg_basebackup and the
>TarMethod of walmethods.c to use the compression method where it
>should, splitting entirely the logic related the compression level.

Thanks.

>This is one step toward the introduction of LZ4 in pg_basebackup, but
>this refactoring is worth doing on its own, hence a separate thread to
>deal with this problem first.  The options of pg_basebackup are
>reworked to be consistent with pg_receivewal, as follows:
>- --compress ranges now from 1 to 9, instead of 0 to 9.
>- --compression-method={none,gzip} is added, the default is none, same
>as HEAD.
>- --gzip/-z has the same meaning as before, being just a synonym of
>--compression-method=gzip with the default compression level of ZLIB
>assigned if there is no --compress.

Indeed this is consistent with pg_receivewal. It gets my +1.

>One more thing that I have noticed while hacking this stuff is that we
>have no regression tests for gzip with pg_basebackup, so I have added
>some that are skipped when not compiling the code with ZLIB.

As far as the code is concerned, I have a minor nitpick.

+   if (compression_method == COMPRESSION_NONE)
+   streamer = bbstreamer_plain_writer_new(archive_filename,
+  archive_file);
 #ifdef HAVE_LIBZ
-   if (compresslevel != 0)
+   else if (compression_method == COMPRESSION_GZIP)
{
strlcat(archive_filename, ".gz", sizeof(archive_filename));
streamer = bbstreamer_gzip_writer_new(archive_filename,
  archive_file,
  compresslevel);
}
-   else
 #endif
-   streamer = bbstreamer_plain_writer_new(archive_filename,
-  archive_file);
-
+   else
+   {
+   Assert(false);  /* not reachable */
+   }

The above block moves the initialization of 'streamer' within two conditional
blocks. Despite this being correct, it is possible that some compilers will
complain for lack of initialization of 'streamer' when it is eventually used a
bit further ahead in:
if (must_parse_archive)
streamer = bbstreamer_tar_archiver_new(streamer);

I propose to initialize streamer to NULL when declared at the top of
CreateBackupStreamer().

As far as the tests are concerned, I think that 2 too many tests are skipped
when HAVE_LIBZ is not defined to be 1. The patch reads:

+Check ZLIB compression if available.
+SKIP:
+{
+   skip "postgres was not built with ZLIB support", 5
+ if (!check_pg_config("#define HAVE_LIBZ 1"));
+
+   $node->command_ok(
+   [
+   'pg_basebackup','-D',
+   "$tempdir/backup_gzip", '--compression-method',
+   'gzip', '--compress', '1', '--no-sync', '--format', 't'
+   ],
+   'pg_basebackup with --compress and --compression-method=gzip');
+
+   # Verify that the stored files are generated with their expected
+   # names.
+   my @zlib_files = glob "$tempdir/backup_gzip/*.tar.gz";
+   is(scalar(@zlib_files), 2,
+   "two files created with gzip (base.tar.gz and pg_wal.tar.gz)");
+
+   # Check the integrity of the files generated.
+   my $gzip = $ENV{GZIP_PROGRAM};
+   skip "program gzip is not found in your system", 1
+ if ( !defined $gzip
+   || $gzip eq ''
+   || system_log($gzip, '--version') != 0);
+
+   my $gzip_is_valid = system_log($gzip, '--test', @zlib_files);
+   is($gzip_is_valid, 0, "gzip verified the integrity of compressed data");
+   rmtree("$tempdir/backup_gzip");
+}

You can see that after the check_pg_config() test, only 3 tests follow,
namely:
 *  $node->command_ok()
 *  is(scalar(), ...)
 *  is($gzip_is_valid, ...)

>Opinions?

Other than the minor issues above, I think this is a solid improvement. +1

>--
>Michael

Cheers,
//Georgios

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Friday, November 19th, 2021 at 3:07 AM, Michael Paquier 
 wrote:

> On Thu, Nov 18, 2021 at 07:54:37PM +0530, Jeevan Ladhe wrote:
>
> > In dir_open_for_write() I observe that we are writing the header
> > and then calling LZ4F_compressEnd() in case there is an error
> > while writing the buffer to the file, and the output buffer of
> > LZ4F_compressEnd() is not written anywhere. Why should this be
> > necessary? To flush the contents of the internal buffer? But, then we
> > are calling LZ4F_freeCompressionContext() immediately after the
> > LZ4F_compressEnd() call. I might be missing something, will be
> > happy to get more insights.
>
> My concern here was symmetry, where IMO it makes sense to have a
> compressEnd call each time there is a successful compressBegin call
> done for the LZ4 state data, as there is no way to know if in the
> future LZ4 won't change some of its internals to do more than just an
> internal flush.

Agreed.

Although the library does provide an interface for simply flushing contents, it
also assumes that each initializing call will have a finilizing call. If my
memory serves me right, earlier versions of the patch, did not have this
summetry, but that got ammended.

Cheers,
//Georgios

> ---
> Michael

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Friday, November 5th, 2021 at 3:47 AM, Michael Paquier  
wrote:

>
> I have spent an extra couple of hours staring at the code, and the
> whole looked fine, so applied. While on it, I have tested the new TAP
> tests with all the possible combinations of --without-zlib and
> --with-lz4.

Great news. Thank you very much.

Cheers,
//Georgios

> --
> Michael

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Thursday, November 4th, 2021 at 9:21 AM, Michael Paquier 
 wrote:
> > +#ifdef HAVE_LIBLZ4
> > +while (readp < readend)
> > +{
> > +size_tread_size = 1;
> > +size_tout_size = 1;
> > +
> > +status = LZ4F_decompress(ctx, outbuf, _size,
> > + readbuf, _size, NULL);
>
> And...  It happens that the error from v9 is here, where we need to
> read the amount of remaining data from "readp", and not "readbuf" :)
>
> It is already late here, I'll continue on this stuff tomorrow, but
> this looks rather committable overall.

Thank you for v11 of the patch. Please find attached v12 which addresses a few
minor points.

Added an Oxford comma since the list now contains three or more terms:
---with-lz4) and none.
+--with-lz4), and none.

Removed an extra condinional check while switching over compression_method.
Instead of:
+   case COMPRESSION_LZ4:
+#ifdef HAVE_LIBLZ4
+   if (compresslevel != 0)
+   {
+   pg_log_error("cannot use --compress with
--compression-method=%s",
+"lz4");
+   fprintf(stderr, _("Try \"%s --help\" for more 
information.\n"),
+   progname);
+   exit(1);
+   }
+#else
+   if (compression_method == COMPRESSION_LZ4)
+   {
+   pg_log_error("this build does not support compression 
with %s",
+"LZ4");
+   exit(1);
+   }
+   break;
+#endif

I opted for:
+   case COMPRESSION_LZ4:
+#ifdef HAVE_LIBLZ4
+   if (compresslevel != 0)
+   {
+   pg_log_error("cannot use --compress with
--compression-method=%s",
+"lz4");
+   fprintf(stderr, _("Try \"%s --help\" for more 
information.\n"),
+   progname);
+   exit(1);
+   }
+#else
+   pg_log_error("this build does not support compression with 
%s",
+"LZ4");
+   exit(1);
+ #endif

There was an error while trying to find the streaming start. The code read:
+ else if (!ispartial && compression_method == COMPRESSION_LZ4)

where it should be instead:
+ else if (!ispartial && wal_compression_method == COMPRESSION_LZ4)

because compression_method is the global option exposed to the whereas
wal_compression_method is the local variable used to figure out what kind of
file the function is currently working with. This error was existing at least in
v9-0002 of $subject.

The variables readbuf and outbuf, used in the decompression of LZ4 files, are
now heap allocated.

Last, while the following is correct:
+   /*
+* Once we have read enough data to cover one segment, we are
+* done, there is no need to do more.
+*/
+   while (uncompressed_size <= WalSegSz)

I felt that converting it a do {} while () loop instead, will help with
readability:
+   do
+   {

+   /*
+* No need to continue reading the file when the uncompressed_size
+* exceeds WalSegSz, even if there are still data left to read.
+* However, if uncompressed_size is equal to WalSegSz, it should
+* verify that there is no more data to read.
+*/
+   } while (r > 0 && uncompressed_size <= WalSegSz);

of course the check:
+   /* Done reading the file */
+   if (r == 0)
+   break;
midway the loop is no longer needed and thus removed.

I would like to have a bit more test coverage in the case for 
FindStreamingStart().
Specifically for the case that a lz4-compressed segment larger than WalSegSz 
exists.
The attached patch does not contain such test case. I am not very certain on 
how to
create such a test case reliably as it would be mostly based on a warning 
emitted
during the parsing of such a file.

Cheers,
//Georgios

> --
> MichaelFrom 48720e7c6ba771c45d43dc9f5e6833f8bb6715e6 Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Thu, 4 Nov 2021 16:05:21 +
Subject: [PATCH v12] Teach pg_receivewal to use LZ4 compression

The program pg_receivewal can use gzip compression to store the received
WAL.  This commit teaches it to also be able to use LZ4 compression. It
is required that the binary is build using the -llz4 flag. It is enabled
via the --with-lz4 flag on configuration time.

The option `--compression-method` has been expanded to inlude the value
[LZ4].  The option `--compress` can not be used with LZ4 compression.

Under the hood there is nothing exceptional to be noted. Tar 

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Thursday, November 4th, 2021 at 9:21 AM, Michael Paquier 
 wrote:

> On Thu, Nov 04, 2021 at 04:31:48PM +0900, Michael Paquier wrote:
> Thanks.  I have looked at 0001 today, and applied it after fixing a
> couple of issues.

Great! Thank you very much.

> From memory:
> - zlib.h was missing from pg_receivewal.c, issue that I noticed after
> removing the redefinition of Z_DEFAULT_COMPRESSION because there was
> no need for it (did a run with a --without-zlib as well).

Yeah, I simply wanted to avoid adding a header. Either way works really.

> - Simplified a bit the error handling for incorrect option
> combinations, using a switch/case while on it.

Much cleaner done this way.

> - Renamed the existing variable "compression" in walmethods.c to
> compression_level, to reduce any confusion with the introduction of
> compression_method.  One thing I have noticed is about the tar method,
> where we rely on the compression level to decide if compression should
> be used or not.  There should be some simplifications possible there
> but there is a huge take in receivelog.c where we use COMPRESSION_NONE
> to track down that we still want to zero a new segment when using tar
> method.

Agreed.

> - Use of 'I' as short option name, err...  After applying the first
> batch..

I left that in just to have the two compression related options next to each
other when switching. I assumed it might help with readability for the next
developer looking at it.

Removing it, is cleaner for the option definifion though, thanks.

>
> Based on the work of 0001, there were some conflicts with 0002.  I
> have solved them while reviewing it, and adapted the code to what got
> already applied.

Thank you very much.

>
> +   header_size = LZ4F_compressBegin(ctx, lz4buf, lz4bufsize, NULL);
> +   if (LZ4F_isError(header_size))
> +   {
> +   pg_free(lz4buf);
> +   close(fd);
> +   return NULL;
> +   }
> In dir_open_for_write(), I guess that this one is missing one
> LZ4F_freeCompressionContext().

Agreed.

>
> +   status = LZ4F_freeDecompressionContext(ctx);
> +   if (LZ4F_isError(status))
> +   {
> +   pg_log_error("could not free LZ4 decompression context: %s",
> +LZ4F_getErrorName(status));
> +   exit(1);
> +   }
> +
> +   if (uncompressed_size != WalSegSz)
> +   {
> +   pg_log_warning("compressed segment file \"%s\" has
> incorrect uncompressed size %ld, skipping",
> +  dirent->d_name, uncompressed_size);
> +   (void) LZ4F_freeDecompressionContext(ctx);
> +   continue;
> +   }
> When the uncompressed size does not match out expected size, the
> second LZ4F_freeDecompressionContext() looks unnecessary to me because
> we have already one a couple of lines above.

Agreed.

>
> +   ctx_out = LZ4F_createCompressionContext(, LZ4F_VERSION);
> +   lz4bufsize = LZ4F_compressBound(LZ4_IN_SIZE, NULL);
> +   if (LZ4F_isError(ctx_out))
> +   {
> +   close(fd);
> +   return NULL;
> +   }
> LZ4F_compressBound() can be after the check on ctx_out, here.
>
> +   while (1)
> +   {
> +   char   *readp;
> +   char   *readend;
> Simply looping when decompressing a segment to check its size looks
> rather unsafe to me.  We should leave the loop once uncompressed_size
> is strictly more than WalSegSz.

The loop exits when done reading or when it failed to read:

+   r = read(fd, readbuf, sizeof(readbuf));
+   if (r < 0)
+   {
+   pg_log_error("could not read file \"%s\": %m", fullpath);
+   exit(1);
+   }
+
+   /* Done reading */
+   if (r == 0)
+   break;

Although I do agree that it can exit before that, if the uncompressed size is
greater than WalSegSz.

>
> The amount of TAP tests looks fine, and that's consistent with what we
> do for zlib^D^D^D^Dgzip.  Now, when testing manually pg_receivewal
> with various combinations of gzip, lz4 and none, I can see the
> following failure in the code that calculates the streaming start
> point:
> pg_receivewal: error: could not decompress file
> "wals//00010006.lz4": ERROR_frameType_unknown
>

Hmmm I will look into that.

> In the LZ4 code, this points to lib/lz4frame.c, where we read an
> incorrect header (see the part that does not match LZ4F_MAGICNUMBER).
> The segments written by pg_receivewal are clean.  Please note that
> this shows up as well when manually compressing some segments with a
> simple lz4 command, to simulate for example the case where a user
> compressed some segments by himself/herself before running
> pg_receivewal.
>

Rights, thank you for investigating. I will look further.

> So, tour code does 

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Wednesday, November 3rd, 2021 at 9:05 AM,  wrote:

> ‐‐‐ Original Message ‐‐‐
>
> On Wednesday, November 3rd, 2021 at 12:23 AM, Michael Paquier 
> mich...@paquier.xyz wrote:
> > On Tue, Nov 02, 2021 at 12:31:47PM -0400, Robert Haas wrote:
> > > On Tue, Nov 2, 2021 at 8:17 AM Magnus Hagander mag...@hagander.net wrote:
> > >
> > > > I think for the end user, it is strictly better to name it "gzip",
> > > > and given that the target of this option is the end user we should
> > > > do so. (It'd be different it we were talking about a build-time
> > > > parameter to configure).
> > >
> > > I agree. Also, I think there's actually a file format called "zlib"
> > > which is slightly different from the "gzip" format, and you have to be
> > > careful not to generate the wrong one.
> >
> > Okay, fine by me. It would be better to be also consistent in
> > WalCompressionMethods once we switch to this option value, then.
>
> I will revert to gzip for version 9. Should be out shortly.

Please find v9 attached.

Cheers,
//Georgios
>
> > MichaelFrom 8e33136f81c3197020053cba0f7f070d594f056e Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Wed, 3 Nov 2021 08:59:58 +
Subject: [PATCH v9 2/2] Teach pg_receivewal to use LZ4 compression

The program pg_receivewal can use gzip compression to store the received WAL.
This commit teaches it to also be able to use LZ4 compression. It is required
that the binary is build using the -llz4 flag. It is enabled via the --with-lz4
flag on configuration time.

The option `--compression-method` has been expanded to inlude the value [LZ4].
The option `--compress` can not be used with LZ4 compression.

Under the hood there is nothing exceptional to be noted. Tar based archives have
not yet been taught to use LZ4 compression. If that is felt useful, then it is
easy to be added in the future.

Tests have been added to verify the creation and correctness of the generated
LZ4 files. The later is achieved by the use of LZ4 program, if present in the
installation.
---
 doc/src/sgml/ref/pg_receivewal.sgml  |   5 +-
 src/Makefile.global.in   |   1 +
 src/bin/pg_basebackup/Makefile   |   1 +
 src/bin/pg_basebackup/pg_receivewal.c| 129 +++
 src/bin/pg_basebackup/t/020_pg_receivewal.pl |  72 -
 src/bin/pg_basebackup/walmethods.c   | 159 ++-
 src/bin/pg_basebackup/walmethods.h   |   1 +
 7 files changed, 358 insertions(+), 10 deletions(-)

diff --git a/doc/src/sgml/ref/pg_receivewal.sgml b/doc/src/sgml/ref/pg_receivewal.sgml
index cf2eaa1486..411b275de0 100644
--- a/doc/src/sgml/ref/pg_receivewal.sgml
+++ b/doc/src/sgml/ref/pg_receivewal.sgml
@@ -268,8 +268,11 @@ PostgreSQL documentation
   

 Enables compression of write-ahead logs using the specified method.
-Supported values gzip,
+Supported values are lz4, gzip,
 and none.
+For the LZ4 method to be available,
+PostgreSQL must have been have been compiled
+with --with-lz4.

   
  
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 533c12fef9..05c54b27de 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -350,6 +350,7 @@ XGETTEXT = @XGETTEXT@
 
 GZIP	= gzip
 BZIP2	= bzip2
+LZ4	= lz4
 
 DOWNLOAD = wget -O $@ --no-use-server-timestamps
 #DOWNLOAD = curl -o $@
diff --git a/src/bin/pg_basebackup/Makefile b/src/bin/pg_basebackup/Makefile
index 459d514183..387d728345 100644
--- a/src/bin/pg_basebackup/Makefile
+++ b/src/bin/pg_basebackup/Makefile
@@ -24,6 +24,7 @@ export TAR
 # used by the command "gzip" to pass down options, so stick with a different
 # name.
 export GZIP_PROGRAM=$(GZIP)
+export LZ4
 
 override CPPFLAGS := -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 9449b50868..af3eba8845 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -29,6 +29,10 @@
 #include "receivelog.h"
 #include "streamutil.h"
 
+#ifdef HAVE_LIBLZ4
+#include "lz4frame.h"
+#endif
+
 /* Time to sleep between reconnection attempts */
 #define RECONNECT_SLEEP_TIME 5
 
@@ -137,6 +141,15 @@ is_xlogfilename(const char *filename, bool *ispartial,
 		return true;
 	}
 
+	/* File looks like a complete LZ4 compressed XLOG file */
+	if (fname_len == XLOG_FNAME_LEN + strlen(".lz4") &&
+		strcmp(filename + XLOG_FNAME_LEN, ".lz4") == 0)
+	{
+		*ispartial = false;
+		*wal_compression_method = COMPRESSION_LZ4;
+		return true;
+	}
+
 	/* File looks like a partial uncompressed XLOG file */
 	if (fname_len == XLOG_FNAME_LEN + strlen(".partial") &&
 		strcmp(filename + XLOG_FNAME_LEN, ".partial") == 0)
@@ -155,6 +168,15 @@ is_xlogfilename(const char *filename, bool *ispartial,
 		return true;
 	}
 
+	/* File looks like a 

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Wednesday, November 3rd, 2021 at 12:23 AM, Michael Paquier 
 wrote:

> On Tue, Nov 02, 2021 at 12:31:47PM -0400, Robert Haas wrote:
>> On Tue, Nov 2, 2021 at 8:17 AM Magnus Hagander mag...@hagander.net wrote:
>>> I think for the end user, it is strictly better to name it "gzip",
>>> and given that the target of this option is the end user we should
>>> do so. (It'd be different it we were talking about a build-time
>>> parameter to configure).
>>
>> I agree. Also, I think there's actually a file format called "zlib"
>> which is slightly different from the "gzip" format, and you have to be
>> careful not to generate the wrong one.
>
> Okay, fine by me.  It would be better to be also consistent in
> WalCompressionMethods once we switch to this option value, then.

I will revert to gzip for version 9. Should be out shortly.

Cheers,
//Georgios
>
> Michael

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Tuesday, November 2nd, 2021 at 9:51 AM, Michael Paquier 
 wrote:

> On Tue, Nov 02, 2021 at 07:27:50AM +0900, Michael Paquier wrote:
> > On Mon, Nov 01, 2021 at 08:39:59AM +, gkokola...@pm.me wrote:
> > > Agreed.
> > >
> > > I have already started on v8 of the patch with that technique. I should
> > > be able to update the thread soon.
> >
> > Nice, thanks!
>

A pleasure. Please find it in the attached v8-0002 patch.

> By the way, I was reading the last version of the patch today, and
> it seems to me that we could make the commit history if we split the
> patch into two parts:
> - One that introduces the new option --compression-method and
> is_xlogfilename(), while reworking --compress (including documentation
> changes).
> - One to have LZ4 support.

Agreed.

>
> v7 has been using "gzip" for the option name, but I think that it
> would be more consistent to use "zlib" instead.

Done.

Cheers,
//Georgios

> --
> MichaelFrom dc33f7ea2930dfc5a7bace52eb0086c759a7d86e Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Tue, 2 Nov 2021 10:39:42 +
Subject: [PATCH v8 1/2] Refactor pg_receivewal in preparation for introducing
 lz4 compression

The program pg_receivewal can use gzip compression to store the received WAL.
The option `--compress` with a value [1, 9] was used to denote that gzip
compression was required. When `--compress` with a value of `0` was used, then
no compression would take place.

This commit introduces a new option, `--compression-method`. Valid values are
[none|zlib]. The option `--compress` requires for `--compression-method` with
value other than `none`. Also `--compress=0` now returns an error.

Under the hood, there are no surprising changes. A new enum WalCompressionMethod
has been introduced and is used throughout the relevant codepaths to explicitly
note which compression method to use.

Last, the macros IsXLogFileName and friends, have been replaced by the function
is_xlogfilename(). This will allow for easier expansion of the available
compression methods that can be recognised.
---
 doc/src/sgml/ref/pg_receivewal.sgml  |  24 ++-
 src/bin/pg_basebackup/pg_basebackup.c|   7 +-
 src/bin/pg_basebackup/pg_receivewal.c| 164 +--
 src/bin/pg_basebackup/receivelog.c   |   2 +-
 src/bin/pg_basebackup/t/020_pg_receivewal.pl |  16 +-
 src/bin/pg_basebackup/walmethods.c   |  51 --
 src/bin/pg_basebackup/walmethods.h   |  15 +-
 src/tools/pgindent/typedefs.list |   1 +
 8 files changed, 200 insertions(+), 80 deletions(-)

diff --git a/doc/src/sgml/ref/pg_receivewal.sgml b/doc/src/sgml/ref/pg_receivewal.sgml
index 9fde2fd2ef..f95cffcd5e 100644
--- a/doc/src/sgml/ref/pg_receivewal.sgml
+++ b/doc/src/sgml/ref/pg_receivewal.sgml
@@ -263,15 +263,31 @@ PostgreSQL documentation
   
  
 
+ 
+  --compression-method=level
+  
+   
+Enables compression of write-ahead logs using the specified method.
+Supported values zlib,
+and none.
+   
+  
+ 
+
  
   -Z level
   --compress=level
   

-Enables gzip compression of write-ahead logs, and specifies the
-compression level (0 through 9, 0 being no compression and 9 being best
-compression).  The suffix .gz will
-automatically be added to all filenames.
+Specifies the compression level (1 through
+9, 1 being worst compression
+and 9 being best compression) for
+zlib compressed WAL segments.
+   
+
+   
+This option requires --compression-method to be
+specified with zlib.

   
  
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 27ee6394cf..cdea3711b7 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -555,10 +555,13 @@ LogStreamerMain(logstreamer_param *param)
 	stream.replication_slot = replication_slot;
 
 	if (format == 'p')
-		stream.walmethod = CreateWalDirectoryMethod(param->xlog, 0,
+		stream.walmethod = CreateWalDirectoryMethod(param->xlog,
+	COMPRESSION_NONE, 0,
 	stream.do_sync);
 	else
-		stream.walmethod = CreateWalTarMethod(param->xlog, compresslevel,
+		stream.walmethod = CreateWalTarMethod(param->xlog,
+			  COMPRESSION_NONE, /* ignored */
+			  compresslevel,
 			  stream.do_sync);
 
 	if (!ReceiveXlogStream(param->bgconn, ))
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 04ba20b197..9641f4a2f4 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -32,6 +32,9 @@
 /* Time to sleep between reconnection attempts */
 #define RECONNECT_SLEEP_TIME 5
 
+/* This is just the redefinition of a libz constant */
+#define Z_DEFAULT_COMPRESSION (-1)
+
 /* Global options */
 static char *basedir = NULL;
 

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Monday, November 1st, 2021 at 9:09 AM, Michael Paquier  
wrote:

> On Fri, Oct 29, 2021 at 08:38:33PM +0900, Michael Paquier wrote:
>
> It would be good to test with many segments, but could we think about
> just relying on LZ4F_decompress() with a frame and compute the
> decompressed size by ourselves? At least that will never break, and
> that would work in all the cases aimed by pg_receivewal.

Agreed.

I have already started on v8 of the patch with that technique. I should
be able to update the thread soon.

>
> Michael

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Saturday, September 18th, 2021 at 8:18 AM, Michael Paquier 
 wrote:
> On Fri, Sep 17, 2021 at 08:12:42AM +, gkokola...@pm.me wrote:
>
> I have been digging into the issue I saw in the TAP tests when closing
> a segment, and found the problem.  The way you manipulate
> frameInfo.contentSize by just setting it to WalSegSz when *opening*
> a segment causes problems on LZ4F_compressEnd(), making the code
> throw a ERROR_frameSize_wrong.  In lz4frame.c, the end of
> LZ4F_compressEnd() triggers this check and the error:
> if (cctxPtr->prefs.frameInfo.contentSize) {
> if (cctxPtr->prefs.frameInfo.contentSize != cctxPtr->totalInSize)
> return err0r(LZ4F_ERROR_frameSize_wrong);
> }
>
> We don't really care about contentSize as long as a segment is not
> completed.  Rather than filling contentSize all the time we write
> something, we'd better update frameInfo once the segment is
> completed and closed.  That would also take take of the error as this
> is not checked if contentSize is 0.  It seems to me that we should
> fill in the information when doing a CLOSE_NORMAL.

Thank you for the comment. I think that the opposite should be done. At the time
that the file is closed, the header is already written to disk. We have no way
to know that is not. If we need to go back to refill the information, we will
have to ask for the API to produce a new header. There is little guarantee that
the header size will be the same and as a consequence we will have to shift
the actual data around.

In the attached, the header is rewritten only when closing an incomplete
segment. For all intents and purposes that segment is not usable. However there
might be custom scripts that might want to attempt to parse even an otherwise
unusable file.

A different and easier approach would be to simply prepare the LZ4 context for
future actions and simply ignore the file.

>
> -   if (stream->walmethod->compression() == 0 &&
> +   if (stream->walmethod->compression() == COMPRESSION_NONE &&
> stream->walmethod->existsfile(fn))
> This one was a more serious issue, as the compression() callback would
> return an integer for the compression level but v5 compared it to a
> WalCompressionMethod.  In order to take care of this issue, mainly for
> pg_basebackup, I think that we have to update the compression()
> callback to compression_method(), and it is cleaner to save the
> compression method as well as the compression level for the tar data.
>

Agreed.

> I am attaching a new patch, on which I have done many tweaks and
> adjustments while reviewing it.  The attached patch fixes the second
> issue, and I have done nothing about the first issue yet, but that
> should be simple enough to address as this needs an update of the
> frame info when closing a completed segment.  Could you look at it?
>

Thank you. Find v7 attached, rebased to the current head.

Cheers,
//Georgios

> Thanks,
> --
> MichaeFrom c3c2eca22102cd0186eb1975339248a200e1ceb9 Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos 
Date: Fri, 22 Oct 2021 13:14:15 +
Subject: [PATCH v7] Teach pg_receivewal to use LZ4 compression

The program pg_receivewal can use gzip compression to store the received WAL.
This commit teaches it to also be able to use LZ4 compression. It is required
that the binary is build using the -llz4 flag. It is enabled via the --with-lz4
flag on configuration time.

Previously, the user had to use the option --compress with a value between [0-9]
to denote that gzip compression was required. This specific behaviour has not
maintained. A newly introduced option --compression-method=[LZ4|gzip] can be
used to ask for the logs to be compressed. Compression values can be selected
only when the compression method is gzip. A compression value of 0 now returns
an error.

Under the hood there is nothing exceptional to be noted. Tar based archives have
not yet been taught to use LZ4 compression. If that is felt useful, then it is
easy to be added in the future.

Tests have been added to verify the creation and correctness of the generated
LZ4 files. The later is achieved by the use of LZ4 program, if present in the
installation.
---
 doc/src/sgml/ref/pg_receivewal.sgml  |  28 +-
 src/Makefile.global.in   |   1 +
 src/bin/pg_basebackup/Makefile   |   1 +
 src/bin/pg_basebackup/pg_basebackup.c|   7 +-
 src/bin/pg_basebackup/pg_receivewal.c| 274 +++
 src/bin/pg_basebackup/receivelog.c   |   2 +-
 src/bin/pg_basebackup/t/020_pg_receivewal.pl |  79 +-
 src/bin/pg_basebackup/walmethods.c   | 263 --
 src/bin/pg_basebackup/walmethods.h   |  16 +-
 src/tools/pgindent/typedefs.list |   1 +
 10 files changed, 589 insertions(+), 83 deletions(-)

diff --git a/doc/src/sgml/ref/pg_receivewal.sgml b/doc/src/sgml/ref/pg_receivewal.sgml
index 9fde2fd2ef..bc2710131a 100644

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Friday, September 17th, 2021 at 09:39, Michael Paquier  
wrote:

> On Thu, Sep 16, 2021 at 03:17:15PM +, gkokola...@pm.me wrote:
>
> > Hopefully fixed.
>
> Thanks for the new version. I have put my hands on the patch, and
> began reviewing its internals with LZ4. I am not done with it yet,
> and I have noticed some places that could be improved (error handling,
> some uses of LZ4F_flush() that should be replaced LZ4F_compressEnd(),
> and more tweaks). I'll send an updated version once I complete my
> review, but that looks rather solid overall.

Thanks! Looking forward to seeing it!

> The changes done in close_walfile()@receivelog.c are useful taken
> independently, so I have applied these separately.

Yeah, I was considering it to split them over to a separate commit,
then decided against it. Will do so in the future.

Cheers,
//Georgios

> 
> Michael

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Wednesday, September 15th, 2021 at 08:46, Michael Paquier 
mich...@paquier.xyz wrote:

Hi,

thank you for the review.

> On Fri, Sep 10, 2021 at 08:21:51AM +, gkokola...@pm.me wrote:
>
> > Agreed. A default value of 5, which is in the middle point of options, has 
> > been
> > defined and used.
> > In addition, the tests have been adjusted to mimic the newly added gzip 
> > tests.
>
> Looking at lz4frame.h, there is LZ4F_flush() that allows to compress
> immediately any data buffered in the frame context but not compressed
> yet. It seems to me that dir_sync() should be extended to support
> LZ4.

Agreed. LZ4F_flush() calls have been added where appropriate.

>
> export GZIP_PROGRAM=$(GZIP)
> +export LZ4
> [...]
> +PGAC_PATH_PROGS(LZ4, lz4)
> -
> PGAC_PATH_BISON
>
> The part of the test assigning LZ4 is fine, but I'd rather switch to a
> logic à-la-gzip, where we just save "lz4" in Makefile.global.in,
> saving cycles in ./configure.

Reluctantly agreed.

>
> +static bool
> +is_xlogfilename(const char *filename, bool *ispartial,
> -   WalCompressionMethod *wal_compression_method)
>
>
> I like the set of simplifications you have done here to detection if a
> segment is partial and which compression method applies to it.

Thank you very much.

>
> +   if (compression_method != COMPRESSION_ZLIB && compresslevel != 0)
> +   {
> +   pg_log_error("can only use --compress together with "
> +"--compression-method=gzip");
> +#ifndef HAVE_LIBLZ4
> +   pg_log_error("this build does not support compression via gzip");
> +#endif
>
> s/HAVE_LIBLZ4/HAVE_LIBZ/.
>

Fixed.

> +$primary->command_fails(
> +   [
> + 'pg_receivewal', '-D', $stream_dir, '--compression-method', 'lz4',
> + '--compress', '1'
> +   ],
> +   'failure if --compression-method=lz4 specified with --compress');
> This would fail when the code is not built with LZ4 with a non-zero
> error code but with an error that is not what we expect. I think that
> you should use $primary->command_fails_like() instead. That's quite
> new, as of de1d4fe. The matching error pattern will need to change
> depending on if we build the code with LZ4 or not. A simpler method
> is to use --compression-method=none, to bypass the first round of
> errors and make that build-independent, but that feels incomplete if
> you want to tie that to LZ4.

Fixed. Now a regex has been added to address both builds.

>
> +   pg_log_warning("compressed segment file "%s" has 
> incorrect header size %lu, skipping",
> +  dirent->d_name, consumed_len);
> +   LZ4F_freeDecompressionContext(ctx);
>
> I agree that skipping all those cases when calculating the streaming
> start point is more consistent.

Thanks.

>
> +   if (r < 0)
> +   pg_log_error("could not read compressed file 
> "%s": %m",
> +fullpath);
> +   else
> +   pg_log_error("could not read compressed file 
> "%s": read %d of %lu",
> +fullpath, r, sizeof(buf));
>
> Let's same in translation effort here by just using "could not read",
> etc. by removing the term "compressed".

The string is also present in the gzip compressed case, i.e. prior to this 
patch.
The translation effort will not be reduced by changing this string only.

> +   pg_log_error("can only use --compress together with "
> +"--compression-method=gzip");
>
> Better to keep these in a single line to ease grepping. We don't care
> if error strings are longer than the 72-80 character limit.

Fixed.

> +/* Size of lz4 input chunk for .lz4 */
> +#define LZ4_IN_SIZE 4096
>
> Why this choice? Does it need to use LZ4_COMPRESSBOUND?

This value is used in order to calculate the bound, before any buffer is
received. Then when we receive buffer, we consume them in LZ4_IN_SIZE chunks.
Note the call to LZ4F_compressBound() in dir_open_for_write().

+   ctx_out = LZ4F_createCompressionContext(, LZ4F_VERSION);
+   lz4bufsize = LZ4F_compressBound(LZ4_IN_SIZE, );


> -  if (dir_data->compression > 0)
> +  if (dir_data->compression_method == COMPRESSION_ZLIB)
> gzclose(gzfp);
> else
>
> Hm. The addition of the header in dir_open_for_write() uses
> LZ4F_compressBegin. Shouldn't we use LZ4F_compressEnd() if
> fsync_fname() or fsync_parent_path() fail on top of closing the fd?
> That would be more consistent IMO to do so. The patch does that in
> dir_close(). You should do that additionally if there is a failure
> when writing the header.

Fixed. LZ4_flush() have been added where appropriate.

>
> +   pg_log_error("invalid compression-method \"%s\", 
> optarg);
> +   exit(1);
>
> This could be "invalid value \"%s\" 

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Saturday, September 11th, 2021 at 07:02, Jian Guo  wrote:

Hi,

thank you for looking at the patch.

> - LZ4F_decompressionContext_t ctx = NULL;
> - snprintf(fullpath, sizeof(fullpath), "%s/%s", basedir, 
> dirent->d_name);
> - fd = open(fullpath, O_RDONLY | PG_BINARY, 0);
>
> Should close the fd before exit or abort.

You are correct. Please find version 4 attached.

Cheers,
//Georgios

v4-0001-Teach-pg_receivewal-to-use-lz4-compression.patch
Description: Binary data

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Friday, July 9th, 2021 at 04:49, Michael Paquier  wrote:

Hi,

please find v3 of the patch attached, rebased to the current head.

> > Michael Paquier wrote:
> >
>
> * http://www.zlib.org/rfc-gzip.html.
>
> -   -   For lz4 compressed segments
> */
> This comment is incomplete.

Fixed.

> +#define IsLZ4CompressXLogFileName(fname) \
> -   (strlen(fname) == XLOG_FNAME_LEN + strlen(".lz4") && \
> -   strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN && \
> -   strcmp((fname) + XLOG_FNAME_LEN, ".lz4") == 0)
>
> +#define IsLZ4PartialCompressXLogFileName(fname) \
> -   (strlen(fname) == XLOG_FNAME_LEN + strlen(".lz4.partial") && \
> -   strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN && \
> -   strcmp((fname) + XLOG_FNAME_LEN, ".lz4.partial") == 0)
>
> This is getting complicated. Would it be better to change this stuff
> and switch to a routine that checks if a segment has a valid name, is
> partial, and the type of compression that applied to it? It seems to
> me that we should group iszlibcompress and islz4compress together with
> the options available through compression_method.

Agreed and done.


> -   if (compresslevel != 0)
> -   {
> - if (compression_method == COMPRESSION_NONE)
> - {
> - compression_method = COMPRESSION_ZLIB;
> - }
> - if (compression_method != COMPRESSION_ZLIB)
> - {
> - pg_log_error("cannot use --compress when "
> -  "--compression-method is not gzip");
> - fprintf(stderr, _("Try \\"%s --help\\" for more 
> information.\\n"),
> - progname);
> - exit(1);
> - }
> -   }
>
> For compatibility where --compress enforces the use of zlib that would
> work, but this needs a comment explaining the goal of this block. I
> am wondering if it would be better to break the will and just complain
> when specifying --compress without --compression-method though. That
> would cause compatibility issues, but this would make folks aware of
> the presence of LZ4, which does not sound bad to me either as ZLIB is
> slower than LZ4 on all fronts.

Fair point. In v3 of the patch --compress requires --compression-method. Passing
0 as value errors out.

> -   else if (compression_method == COMPRESSION_ZLIB)
> -   {
> - pg_log_error("cannot use --compression-method gzip when "
> -  "--compression is 0");
> - fprintf(stderr, _("Try \\"%s --help\\" for more information.\\n"),
> - progname);
> - exit(1);
> -   }
>
> Hmm. It would be more natural to enforce a default compression level
> in this case? The user is asking for a compression with zlib here.

Agreed. A default value of 5, which is in the middle point of options, has been
defined and used.

In addition, the tests have been adjusted to mimic the newly added gzip tests.


Cheers,
//Georgios


> ---
>
> Michael

v3-0001-Teach-pg_receivewal-to-use-lz4-compression.patch
Description: Binary data

Re: Showing I/O timings spent reading/writing temp buffers in EXPLAIN

[gkokolatos]

Hi,

‐‐‐ Original Message ‐‐‐
On Tuesday, August 24th, 2021 at 13:20, Ranier Vilela  
wrote:

> Em ter., 24 de ago. de 2021 às 03:11, Masahiko Sawada  
> escreveu:
>
> > On Mon, Aug 23, 2021 at 10:46 AM Masahiko Sawada  
> > wrote:
> >
> > >
> > > On Thu, Aug 19, 2021 at 10:52 PM Ranier Vilela  
> > > wrote:
> > > >
> > > > Em qui., 19 de ago. de 2021 às 09:21, Masahiko Sawada 
> > > >  escreveu:
> > > >>
> > > >> Hi all ,
> > > >>
> > > >> It's reported on pgsql-bugs[1] that I/O timings in EXPLAIN don't show



> >
> > I've attached the updated patch that incorporates the above comment.
>
> The patch looks fine to me.
>

The patch looks good to me too. However I do wonder why the timing is added 
only on
the

   if (es->format == EXPLAIN_FORMAT_TEXT)

block and is not added when, for example, the format is json. The 
instrumentation has
clearly recorded the timings regardless of the output format.

Also, it might be worth while to consider adding some regression tests. To my
understanding, explain.sql provides a function, explain_filter, which helps 
create
a stable result. For example, such a test case can be:

  set track_io_timing = 'on';
  select explain_filter('explain (analyze, buffers) select count(*) from 
generate_series(1,10)');

then it would be enough to verify that the line:

  I/O Timings: temp read=N.N write=N.N

is present. The above would apply on the json output via 
`explain_filter_to_json`
of course.

Thoughts?

Cheers,
//Georgios

Re: psql: \dl+ to list large objects privileges

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Sunday, September 5th, 2021 at 21:47, Pavel Luzanov 
 wrote:

Hi,

> Hi,
>
> On 03.09.2021 15:25, Georgios Kokolatos wrote:
>
> > On a high level I will recommend the addition of tests. There are similar 
> > tests
>
> Tests added.

Thanks! The tests look good. A minor nitpick would be to also add a comment on 
the
large object to verify that it is picked up correctly.

Also:

   +\lo_unlink 42
   +DROP ROLE lo_test;
   +

That last empty line can be removed.

>
> > Applying the patch, generates several whitespace warnings. It will be 
> > helpful
> > if those warnings are removed.
>
> I know this is a silly mistake, and after reading this article[1] I tried to 
> remove the extra spaces.
> Can you tell me, please, how can you get such warnings?

I only mentioned it because I thought you might find it useful.
I apply patches by `git apply` and executing the command on the latest version
of your patch, produces:

  $ git apply lo-list-acl-v2.patch
  lo-list-acl-v2.patch:349: new blank line at EOF.
  +
  warning: 1 line adds whitespace errors

The same can be observed highlighted by executing `git diff --color` as
recommended in the the article you linked.

>
> > The patch contains:
> >
> > case 'l':
> > -   success = do_lo_list();
> > +   success = listLargeObjects(show_verbose);
> >
> >
> > It might be of some interest to consider in the above to check the value of 
> > the
> > next character in command or emit an error if not valid. Such a pattern can 
> > be
> > found in the same switch block as for example:
> >
> > switch (cmd[2])
> > {
> > case '\0':
> > case '+':
> > 
> > success = ...
> > 
> > break;
> > default:
> > status = PSQL_CMD_UNKNOWN;
> > break;
> > }
>
> Check added.

Thanks.

>
> > The patch contains:
> >
> > else if (strcmp(cmd + 3, "list") == 0)
> > -   success = do_lo_list();
> > +   success = listLargeObjects(false);
> > +
> > +   else if (strcmp(cmd + 3, "list+") == 0)
> > +   success = listLargeObjects(true);
> >
> >
> > In a fashion similar to `exec_command_list`, it might be interesting to 
> > consider
> > expressing the above as:
> >
> > show_verbose = strchr(cmd, '+') ? true : false;
> > 
> > else if (strcmp(cmd + 3, "list") == 0
> > success = do_lo_list(show_verbose);
>
> I rewrote this part.

Thank you. It looks good to me.

>
> New version attached.

The new version looks good to me.

I did spend a bit of time considering the addition of the verbose version of
the command. I understand your reasoning explained upstream in the same thread.
However, I am not entirely convinced. The columns in consideration are,
"Description" and "Access Privileges". Within the describe commands there are
four different options, listed and explained bellow:

   commands where description is found in verbose
\d \dA \dc \dd \des \df \dFd \dFt \di \dL \dn \dO \dP \dPt \dt \du \dx \dy \da
\db \dC \dD \det \dew \dFp \dg \dl \dm \do \dPi \dS \dT

   commands where description is not found in verbose (including object
   description)
\dd \dFd \dFt \dL \dx \da \dF \dFp \dl \do \dT

   commands where access privileges is found in verbose
\def \dD \des

   commands where access privileges is not found in verbose (including access
   privilege describing)
\ddp \dp \des \df \dL \dn \db \dD \dew \dl \dT

With the above list, I would argue that it feels more natural to include
the "Access Privileges" column in the non verbose version and be done with
the verbose version all together.

My apologies for the back and forth on this detail.

Thoughts?

Cheers,
//Georgios

>
> [1] https://wiki.postgresql.org/wiki/Creating_Clean_Patches
>
> --
> Pavel Luzanov
> Postgres Professional: https://postgrespro.com
> The Russian Postgres Company

Re: Introduce pg_receivewal gzip compression tests

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Thursday, July 15th, 2021 at 14:35, Michael Paquier  
wrote:

> On Thu, Jul 15, 2021 at 08:35:27PM +0900, Michael Paquier wrote:

>
> > 2.  curculio:
> > Looking at the OpenBSD code (usr.bin/compress/main.c), long options
> > are supported, where --version does exit(0) without printing
> > set_outfile() is doing a discard of the file suffixes it does not
> > recognize, and I think that their implementation bumps on .gz.partial
> > and generates an exit code of 512 to map with WARNING. I still wish
> > to keep this test, and I'd like to think that the contents of
> > @zlib_wals are enough in terms of coverage. What do you think?
>
> After thinking more about this one, I have taken the course to just
> remove the .gz.partial segment from the check, a full segment should
> be enough in terms of coverage. I prefer this simplification over a
> rename of the .partial segment or a tweak of the error code to map
> with WARNING.

Fair enough.

Cheers,
//Georgios

> ---
>
> Michael

Re: Introduce pg_receivewal gzip compression tests

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Thursday, July 15th, 2021 at 09:00, Michael Paquier  
wrote:

> On Wed, Jul 14, 2021 at 02:11:09PM +, gkokola...@pm.me wrote:
>
> > Please find v6 attached.
>
> Thanks. I have spent some time checking this stuff in details, and
> I did some tests on Windows while on it. A run of pgperltidy was
> missing. A second thing is that you added one useless WAL segment
> switch in the ZLIB block, and two at the end, causing the first two in
> the set of three (one in the ZLIB block and one in the final command)
> to be no-ops as they followed a previous WAL switch. The final one
> was not needed as no WAL is generated after that.
>

Thank you for the work and comments.

> And applied. Let's see if the buildfarm has anything to say. Perhaps
> this will even catch some bugs that pre-existed.

Let us hope that it will prevent some bugs from happening.

Cheers,
//Georgios

> ---
>
> Michael

Re: Introduce pg_receivewal gzip compression tests

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Wednesday, July 14th, 2021 at 04:17, Michael Paquier  
wrote:

> On Tue, Jul 13, 2021 at 11:16:06AM +, gkokola...@pm.me wrote:
> > Agreed. For the record that is why I said v6 :)
> Okay, thanks.

Please find v6 attached.

Cheers,
//Georgios

> ---
>
> Michael

v6-0001-Introduce-pg_receivewal-gzip-compression-tests.patch
Description: Binary data

Re: Introduce pg_receivewal gzip compression tests

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Tuesday, July 13th, 2021 at 12:26, Michael Paquier  
wrote:

> On Tue, Jul 13, 2021 at 08:28:44AM +, gkokola...@pm.me wrote:
> > Sounds great. Let me cook up v6 for this.
>
> Thanks. Could you use v5 I posted upthread as a base? There were
> some improvements in the variable names, the comments and the test
> descriptions.

Agreed. For the record that is why I said v6 :)

Cheers,
//Georgios

> -
> Michael

Re: Introduce pg_receivewal gzip compression tests

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Tuesday, July 13th, 2021 at 10:14, Michael Paquier  
wrote:

> On Tue, Jul 13, 2021 at 04:37:53PM +0900, Michael Paquier wrote:
>
> Poking at this problem, I partially take this statement back as this
> requires an initial run of pg_receivewal --endpos to ensure the
> creation of one .gz and one .gz.partial. So I guess that this should
> be structured as:
>
> 1.  Keep the existing pg_receivewal --endpos.
> 2.  Add the ZLIB block, with one pg_receivewal --endpos.
> 3.  Add at the end one extra pg_receivewal --endpos, outside of the ZLIB
> block, which should check the creation of a .partial, non-compressed
> segment. This should mention that we place this command at the end of
> the test for the start streaming point computation.
> LZ4 tests would be then between 2) and 3), or 1) and 2).

Sounds great. Let me cook up v6 for this.

>
> --
>
> Michael

Re: Introduce pg_receivewal gzip compression tests

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Tuesday, July 13th, 2021 at 09:37, Michael Paquier  
wrote:

> On Tue, Jul 13, 2021 at 06:36:59AM +, gkokola...@pm.me wrote:
>
> > I am sorry this was not so clear. It is indeed running twice the binary
> > with different flags. However the goal is not to check the flags, but
> > to make certain that the partial file has now been completed. That is
> > why there was code asserting that the previous FILENAME.gz.partial file
> > after the second invocation is converted to FILENAME.gz.
>
> The first run you are adding checks the same thing thanks to
> pg_switch_wal(), where pg_receivewal completes the generation of
> 00010002.gz and finishes with
> 00010003.gz.partial.

This is correct. It is the 00010003 awl that the rest
of the tests are targeting.

>
> > Additionally the second invocation of pg_receivewal is extending the
> > coverage of FindStreamingStart().
>
> Hmm. It looks like a waste in runtime once we mix LZ4 in that as that
> would mean 5 runs of pg_receivewal, but we really need only three of
> them with --endpos:
> -   One with ZLIB compression.
> -   One with LZ4 compression.
> -   One without compression.
>
> Do you think that we could take advantage of what is now the only run
> of pg_receivewal --endpos for that? We could make the ZLIB checks run
> first, conditionally, and then let the last command with --endpos
> perform a full scan of the contents in $stream_dir with the .gz files
> already in place. The addition of LZ4 would be an extra conditional
> block similar to what's introduced in ZLIB, running before the last
> command without compression.

I will admit that for the current patch I am not taking lz4 into account as
at the moment I have little idea as to how the lz4 patch will advance with the
review rounds. I simply accepted that it will be rebased on top of the patch
in the current thread and probably need to modify the current then.

But I digress. I would like have some combination of .gz and .gz.parial but
I will not take too strong of a stance. I am happy to go with your suggestion.

Cheers,
//Georgios

>
> --
>
> Michael

Re: Introduce pg_receivewal gzip compression tests

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Tuesday, July 13th, 2021 at 03:53, Michael Paquier  
wrote:

> On Mon, Jul 12, 2021 at 04:46:29PM +, gkokola...@pm.me wrote:
>
> > On Monday, July 12th, 2021 at 17:07, gkokola...@pm.me wrote:
> >
> > > ‐‐‐ Original Message ‐‐‐
>
> Are you using outlook? The format of your messages gets blurry on the
>
> PG website, so does it for me.

I am using protonmail's web page. I was not aware of the issue. Thank you
for bringing it up to my attention. I shall try to address it.

>
> > > I am admittedly not so well versed on Windows systems. Thank you for
> > >
> > > informing me.
> > >
> > > Please find attached v3 of the patch where $ENV{GZIP_PROGRAM} is used
> > >
> > > instead. To the best of my knowledge one should avoid using $ENV{GZIP}
> > >
> > > because that would translate to the obsolete, yet used environment
> > >
> > > variable GZIP which holds a set of default options for gzip. In essence
> > >
> > > it would be equivalent to executing:
> > >
> > > GZIP=gzip gzip --test 
> > >
> > > which can result to errors similar to:
> > >
> > > gzip: gzip: non-option in GZIP environment variable
>
> -# make this available to TAP test scripts
>
> +# make these available to TAP test scripts
>
> export TAR
>
> +export GZIP_PROGRAM=$(GZIP)
>
> Wow. So this comes from the fact that the command gzip can feed on
>
> the environment variable from the same name. I was not aware of
>
> that, and a comment would be in place here. That means complicating a
>
> bit the test flow for people on Windows, but I am fine to live with
>
> that as long as this does not fail hard. One extra thing we could do
>
> is drop this part of the test, but I agree that this is useful to have
>
> around as a validity check.

Great.

>
> > After a bit more thinking, I went ahead and added on top of v3 a test
> >
> > verifying that the gzip program can actually be called.
>
> - system_log($gzip, '--version') != 0);
>
>
>
> Checking after that does not hurt, indeed. I am wondering if we
>
> should do that for TAR.

I do not think that this will be a necessity for TAR. TAR after all
is discovered by autoconf, which gzip is not.

>
> Another thing I find unnecessary is the number of the tests. This
>
> does two rounds of pg_receivewal just to test the long and short
>
> options of -Z/-compress, which brings only coverage to make sure that
>
> both option names are handled. That's a high cost for a low amound of
>
> extra coverage, so let's cut the runtime in half and just use the
>
> round with --compress.

I am sorry this was not so clear. It is indeed running twice the binary
with different flags. However the goal is not to check the flags, but
to make certain that the partial file has now been completed. That is
why there was code asserting that the previous FILENAME.gz.partial file
after the second invocation is converted to FILENAME.gz.

Additionally the second invocation of pg_receivewal is extending the
coverage of FindStreamingStart().

The different flags was an added bonus.

>
> There was also a bit of confusion with ZLIB and gzip in the variable
>
> names and the comments, the latter being only the command while the
>
> compression happens with zlib. With a round of indentation on top of
>
> all that, I ge tthe attached.
>
> What do you think?

Thank you very much for the patch. I would prefer to keep the parts that
tests that .gz.partial are completed on a subsequent run if you agree.

Cheers,
//Georgios

> --
>
> Michael

Re: Introduce pg_receivewal gzip compression tests

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Monday, July 12th, 2021 at 17:07,  wrote:

> ‐‐‐ Original Message ‐‐‐
>
> On Monday, July 12th, 2021 at 13:04, Michael Paquier mich...@paquier.xyz 
> wrote:
>
> > On Mon, Jul 12, 2021 at 09:42:32AM +, gkokola...@pm.me wrote:
> >
> > > This to my understanding means that gzip is expected to exist.
> > >
> > > If this is correct, then simply checking for the headers should
> > >
> > > suffice, since that is the only dependency for the files to be
> > >
> > > created.
> >
> > You cannot expect this to work on Windows when it comes to MSVC for
> >
> > example, as gzip may not be in the environment PATH so the test would
> >
> > fail hard. Let's just rely on $ENV{GZIP} instead, and skip the test
> >
> > if it is not defined.
>
> I am admittedly not so well versed on Windows systems. Thank you for
>
> informing me.
>
> Please find attached v3 of the patch where $ENV{GZIP_PROGRAM} is used
>
> instead. To the best of my knowledge one should avoid using $ENV{GZIP}
>
> because that would translate to the obsolete, yet used environment
>
> variable GZIP which holds a set of default options for gzip. In essence
>
> it would be equivalent to executing:
>
> GZIP=gzip gzip --test 
>
> which can result to errors similar to:
>
> gzip: gzip: non-option in GZIP environment variable
>

After a bit more thinking, I went ahead and added on top of v3 a test
verifying that the gzip program can actually be called.

Please find v4 attached.

Cheers,
//Georgios

>
> > Michael

v4-0001-Introduce-pg_receivewal-gzip-compression-tests.patch
Description: Binary data

Re: Introduce pg_receivewal gzip compression tests

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Monday, July 12th, 2021 at 13:04, Michael Paquier  
wrote:

> On Mon, Jul 12, 2021 at 09:42:32AM +, gkokola...@pm.me wrote:
>
> > This to my understanding means that gzip is expected to exist.
> >
> > If this is correct, then simply checking for the headers should
> >
> > suffice, since that is the only dependency for the files to be
> >
> > created.
>
> You cannot expect this to work on Windows when it comes to MSVC for
>
> example, as gzip may not be in the environment PATH so the test would
>
> fail hard. Let's just rely on $ENV{GZIP} instead, and skip the test
>
> if it is not defined.

I am admittedly not so well versed on Windows systems. Thank you for
informing me.

Please find attached v3 of the patch where $ENV{GZIP_PROGRAM} is used
instead. To the best of my knowledge one should avoid using $ENV{GZIP}
because that would translate to the obsolete, yet used environment
variable GZIP which holds a set of default options for gzip. In essence
it would be equivalent to executing:
   GZIP=gzip gzip --test 
which can result to errors similar to:
   gzip: gzip: non-option in GZIP environment variable

Cheers,
//Georgios

> 
>
> Michael

v3-0001-Introduce-pg_receivewal-gzip-compression-tests.patch
Description: Binary data

Re: Introduce pg_receivewal gzip compression tests

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Monday, July 12th, 2021 at 13:00, Gilles Darold  wrote:

> Le 12/07/2021 à 12:27, gkokola...@pm.me a écrit :
>
> > > > > Shouldn't this be coded as a loop going through @gzip_wals?
> > > > >
> > > > > I would hope that there is only one gz file created. There is a line
> > > >
> > > > further up that tests exactly that.
> > > >
> > > > -   is (scalar(@gzip_wals), 1, "one gzip compressed WAL was created");
> > > >
> > > > Let me amend that. The line should be instead:
> > >
> > > is (scalar(keys @gzip_wals), 1, "one gzip compressed WAL was created");
> > >
> > > To properly test that there is one entry.
> > >
> > > Let me provide with v2 to fix this.
>
> The following tests are not correct in Perl even if Perl returns the
>
> right value.
>
> +    is (scalar(keys @gzip_wals), 1, "one gzip compressed WAL was created");
>
> +    is (scalar(keys @gzip_partial_wals), 1,
>
> +        "one partial gzip compressed WAL was created");
>
> Function keys or values are used only with hashes but here you are using
>
> arrays. To obtain the length of the array you can just use the scalar
>
> function as Perl returns the length of the array when it is called in a
>
> scalar context. Please use the following instead:
>
> +    is (scalar(@gzip_wals), 1, "one gzip compressed WAL was created");
>
> +    is (scalar(@gzip_partial_wals), 1,
>
> +        "one partial gzip compressed WAL was created");

You are absolutely correct. I had used that in v1, yet since it got called out
I doubted myself, assumed I was wrong and the rest is history. I shall ament the
amendment for v3 of the patch.

Cheers,
//Georgios

>
>
> -
>
> Gilles Darold
>
> http://www.darold.net/

Re: Introduce pg_receivewal gzip compression tests

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Monday, July 12th, 2021 at 11:56,  wrote:

> ‐‐‐ Original Message ‐‐‐
>
> On Monday, July 12th, 2021 at 11:42, gkokola...@pm.me wrote:
>
> > ‐‐‐ Original Message ‐‐‐
> >
> > On Monday, July 12th, 2021 at 08:42, Michael Paquier mich...@paquier.xyz 
> > wrote:
> >
> > > On Fri, Jul 09, 2021 at 11:26:58AM +, Georgios wrote:
> > >
> > > > As suggested on a different thread [1], pg_receivewal can increase it's 
> > > > test
> > > >
> > > > coverage. There exists a non trivial amount of code that handles gzip
> > > >
> > > > compression. The current patch introduces tests that cover creation of 
> > > > gzip
> > > >
> > > > compressed WAL files and the handling of gzip partial segments. Finally 
> > > > the
> > > >
> > > > integrity of the compressed files is verified.
> > >
> > > - # Verify compressed file's integrity
> > >
> > >
> > > - my $gzip_is_valid = system_log('gzip', '--test', 
> > > $gzip_wals[0]);
> > >
> > >
> > > - is($gzip_is_valid, 0, "program gzip verified file's 
> > > integrity");
> > >
> > >
> > >
> > > libz and gzip are usually split across different packages, hence there
> > >
> > > is no guarantee that this command is always available (same comment as
> > >
> > > for LZ4 from a couple of days ago).
> >
> > Of course. Though while going for it, I did find in Makefile.global.in:
> >
> > TAR = @TAR@
> >
> > XGETTEXT = @XGETTEXT@
> >
> > GZIP = gzip
> >
> > BZIP2 = bzip2
> >
> > DOWNLOAD = wget -O $@ --no-use-server-timestamps
> >
> > Which is also used by GNUmakefile.in
> >
> > distcheck: dist
> >
> > rm -rf $(dummy)
> >
> > mkdir $(dummy)
> >
> > $(GZIP) -d -c $(distdir).tar.gz | $(TAR) xf -
> >
> > install_prefix=`cd $(dummy) && pwd`; \
> >
> > This to my understanding means that gzip is expected to exist.
> >
> > If this is correct, then simply checking for the headers should
> >
> > suffice, since that is the only dependency for the files to be
> >
> > created.
> >
> > If this is wrong, then I will add the discovery code as in the
> >
> > other patch.
> >
> > > - [
> > >
> > >
> > > - 'pg_receivewal', '-D', $stream_dir, 
> > > '--verbose',
> > >
> > >
> > > - '--endpos',  $nextlsn, '-Z', '5'
> > >
> > >
> > > - ],
> > >
> > >
> > >
> > > I would keep the compression level to a minimum here, to limit CPU
> > >
> > > usage but still compress something faster.
> > >
> > > - # Verify compressed file's integrity
> > >
> > >
> > > - my $gzip_is_valid = system_log('gzip', '--test', 
> > > $gzip_wals[0]);
> > >
> > >
> > > - is($gzip_is_valid, 0, "program gzip verified file's 
> > > integrity");
> > >
> > >
> > >
> > > Shouldn't this be coded as a loop going through @gzip_wals?
> >
> > I would hope that there is only one gz file created. There is a line
> >
> > further up that tests exactly that.
> >
> > -   is (scalar(@gzip_wals), 1, "one gzip compressed WAL was created");
>
> Let me amend that. The line should be instead:
>
> is (scalar(keys @gzip_wals), 1, "one gzip compressed WAL was created");
>
> To properly test that there is one entry.
>
> Let me provide with v2 to fix this.


Please find v2 attached with the above.

Cheers,
//Georgios

>
> Cheers,
>
> //Georgios
>
> > Then there should also be a partial gz file which is tested further 
> > ahead.
> >
> > Cheers,
> >
> > //Georgios
> >
> >
> > > Michael

v2-0001-Introduce-pg_receivewal-gzip-compression-tests.patch
Description: Binary data

Re: Introduce pg_receivewal gzip compression tests

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Monday, July 12th, 2021 at 11:42,  wrote:

> ‐‐‐ Original Message ‐‐‐
>
> On Monday, July 12th, 2021 at 08:42, Michael Paquier mich...@paquier.xyz 
> wrote:
>
> > On Fri, Jul 09, 2021 at 11:26:58AM +, Georgios wrote:
> >
> > > As suggested on a different thread [1], pg_receivewal can increase it's 
> > > test
> > >
> > > coverage. There exists a non trivial amount of code that handles gzip
> > >
> > > compression. The current patch introduces tests that cover creation of 
> > > gzip
> > >
> > > compressed WAL files and the handling of gzip partial segments. Finally 
> > > the
> > >
> > > integrity of the compressed files is verified.
> >
> > -   # Verify compressed file's integrity
> >
> >
> > -   my $gzip_is_valid = system_log('gzip', '--test', $gzip_wals[0]);
> >
> >
> > -   is($gzip_is_valid, 0, "program gzip verified file's integrity");
> >
> >
> >
> > libz and gzip are usually split across different packages, hence there
> >
> > is no guarantee that this command is always available (same comment as
> >
> > for LZ4 from a couple of days ago).
>
> Of course. Though while going for it, I did find in Makefile.global.in:
>
> TAR = @TAR@
>
> XGETTEXT = @XGETTEXT@
>
> GZIP = gzip
>
> BZIP2 = bzip2
>
> DOWNLOAD = wget -O $@ --no-use-server-timestamps
>
> Which is also used by GNUmakefile.in
>
> distcheck: dist
>
> rm -rf $(dummy)
>
> mkdir $(dummy)
>
> $(GZIP) -d -c $(distdir).tar.gz | $(TAR) xf -
>
> install_prefix=`cd $(dummy) && pwd`; \
>
> This to my understanding means that gzip is expected to exist.
>
> If this is correct, then simply checking for the headers should
>
> suffice, since that is the only dependency for the files to be
>
> created.
>
> If this is wrong, then I will add the discovery code as in the
>
> other patch.
>
> > -   [
> >
> >
> > -   'pg_receivewal', '-D', $stream_dir, 
> > '--verbose',
> >
> >
> > -   '--endpos',  $nextlsn, '-Z', '5'
> >
> >
> > -   ],
> >
> >
> >
> > I would keep the compression level to a minimum here, to limit CPU
> >
> > usage but still compress something faster.
> >
> > -   # Verify compressed file's integrity
> >
> >
> > -   my $gzip_is_valid = system_log('gzip', '--test', $gzip_wals[0]);
> >
> >
> > -   is($gzip_is_valid, 0, "program gzip verified file's integrity");
> >
> >
> >
> > Shouldn't this be coded as a loop going through @gzip_wals?
>
> I would hope that there is only one gz file created. There is a line
>
> further up that tests exactly that.
>
> -   is (scalar(@gzip_wals), 1, "one gzip compressed WAL was created");


Let me amend that. The line should be instead:

  is (scalar(keys @gzip_wals), 1, "one gzip compressed WAL was created");

To properly test that there is one entry.

Let me provide with v2 to fix this.

Cheers,
//Georgios

>
> Then there should also be a partial gz file which is tested further ahead.
>
> Cheers,
>
> //Georgios
>
> > Michael

Re: Introduce pg_receivewal gzip compression tests

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Monday, July 12th, 2021 at 08:42, Michael Paquier  
wrote:

> On Fri, Jul 09, 2021 at 11:26:58AM +, Georgios wrote:
>
> > As suggested on a different thread [1], pg_receivewal can increase it's test
> >
> > coverage. There exists a non trivial amount of code that handles gzip
> >
> > compression. The current patch introduces tests that cover creation of gzip
> >
> > compressed WAL files and the handling of gzip partial segments. Finally the
> >
> > integrity of the compressed files is verified.
>
> - # Verify compressed file's integrity
>
>
> - my $gzip_is_valid = system_log('gzip', '--test', $gzip_wals[0]);
>
>
> - is($gzip_is_valid, 0, "program gzip verified file's integrity");
>
>
>
> libz and gzip are usually split across different packages, hence there
>
> is no guarantee that this command is always available (same comment as
>
> for LZ4 from a couple of days ago).


Of course. Though while going for it, I did find in Makefile.global.in:

  TAR = @TAR@
  XGETTEXT = @XGETTEXT@

  GZIP= gzip
  BZIP2   = bzip2

  DOWNLOAD = wget -O $@ --no-use-server-timestamps

Which is also used by GNUmakefile.in

  distcheck: dist
  rm -rf $(dummy)
  mkdir $(dummy)
  $(GZIP) -d -c $(distdir).tar.gz | $(TAR) xf -
  install_prefix=`cd $(dummy) && pwd`; \


This to my understanding means that gzip is expected to exist.
If this is correct, then simply checking for the headers should
suffice, since that is the only dependency for the files to be
created.

If this is wrong, then I will add the discovery code as in the
other patch.

>
> - [
>
>
> - 'pg_receivewal', '-D', $stream_dir, '--verbose',
>
>
> - '--endpos',  $nextlsn, '-Z', '5'
>
>
> - ],
>
>
>
> I would keep the compression level to a minimum here, to limit CPU
>
> usage but still compress something faster.
>
> - # Verify compressed file's integrity
>
>
> - my $gzip_is_valid = system_log('gzip', '--test', $gzip_wals[0]);
>
>
> - is($gzip_is_valid, 0, "program gzip verified file's integrity");
>
>
>
> Shouldn't this be coded as a loop going through @gzip_wals?

I would hope that there is only one gz file created. There is a line
further up that tests exactly that.

+   is (scalar(@gzip_wals), 1, "one gzip compressed WAL was created");


Then there should also be a partial gz file which is tested further ahead.

Cheers,
//Georgios

> ---
>
> Michael

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Monday, July 12th, 2021 at 07:56, Michael Paquier  
wrote:

> On Mon, Jul 12, 2021 at 11:10:24AM +0530, Dilip Kumar wrote:
>
> > On Thu, Jul 8, 2021 at 7:48 PM gkokola...@pm.me wrote:
> >
> > > We can, though I am not in favour of doing so. There is seemingly
> > >
> > > little benefit for added complexity.
> >
> > I am really not sure what complexity you are talking about, do you
> >
> > mean since with pglz we were already providing the compression level
> >
> > so let it be as it is but with the new compression method you don't
> >
> > see much benefit of providing compression level or speed?
>
> You mean s/pglz/zlib/ here perhaps? I am not sure what Georgios has
>
> in mind, but my opinion stands on the latter: there is little benefit
>
> in making lz4 faster than the default and reduce compression, as the
>
> default is already a rather low CPU user.

Thank you all for your comments. I am sitting on the same side as Micheal
on this one. The complexity is not huge, yet there will need to be code to
pass this option to the lz4 api and various test cases to verify for
correctness and integrity. The burden of maintenance of this code vs the
benefit of the option, tilt the scale towards not including the option.

Of course, I will happily provide whatever the community finds beneficial.

Cheers,
//Georgios


> -
>
> Michael

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Friday, July 9th, 2021 at 04:49, Michael Paquier  wrote:

> On Thu, Jul 08, 2021 at 02:18:40PM +, gkokola...@pm.me wrote:
>
> > please find v2 of the patch which tries to address the commends
> >
> > received so far.
>
> Thanks!
>
> > Michael Paquier wrote:
> >
> > > -   system_or_bail('lz4', '-t', $lz4_wals[0]);
> > >
> > > I think that you should just drop this part of the test. The only
> > >
> > > part of LZ4 that we require to be present when Postgres is built with
> > >
> > > --with-lz4 is its library liblz4. Commands associated to it may not
> > >
> > > be around, causing this test to fail. The test checking that one .lz4
> > >
> > > file has been created is good to have. It may be worth adding a test
> > >
> > > with a .lz4.partial segment generated and --endpos pointing to a LSN
> > >
> > > that does not finish the segment that gets switched.
> >
> > I humbly disagree with the need for the test. It is rather easily possible
> >
> > to generate a file that can not be decoded, thus becoming useless. Having 
> > the
> >
> > test will provide some guarantee for the fact. In the current patch, there
> >
> > is code to find out if the program lz4 is available in the system. If it is
> >
> > not available, then that specific test is skipped. The rest remains as it
> >
> > were. Also `system_or_bail` is not used anymore in favour of the 
> > `system_log`
> >
> > so that the test counted and the execution of tests continues upon failure.
>
> Check. I can see what you are using in the new patch. We could live
>
> with that.

Great. Thank you.

>
> > > It seems to me that you are missing some logic in
> > >
> > > FindStreamingStart() to handle LZ4-compressed segments, in relation
> > >
> > > with IsCompressXLogFileName() and IsPartialCompressXLogFileName().
> >
> > Very correct. The logic is now added. Given the lz4 api, one has to fill
> >
> > in the uncompressed size during creation time. Then one can read the
> >
> > headers and verify the expectations.
>
> Yeah, I read that as well with lz4 --list and the kind. That's weird
>
> compared to how ZLIB gives an easy access to it. We may want to do an
>
> effort and tell about more lz4 --content-size/--list, telling that we
>
> add the size in the segment compressed because we have to and LZ4 does
>
> not do it by default?

I am afraid I do not follow. In the patch we do add the uncompressed size
and then, the uncompressed size is checked against a known value WalSegSz.
What the compressed size will be checked against?

>
> > > Should we have more tests for ZLIB, while on it? That seems like a
> > >
> > > good addition as long as we can skip the tests conditionally when
> > >
> > > that's not supported.
> >
> > Agreed. Please allow me to provide a distinct patch for this code.
>
> Thanks. Looking forward to seeing it. That may be better on a
>
> separate thread, even if I digressed in this thread :)

Thank you for the comments. I will sent in a separate thread.

>
> > > I think we can somehow use "acceleration" parameter of lz4 compression
> > >
> > > to map on compression level, It is not direct mapping but
> > >
> > > can't we create some internal mapping instead of completely ignoring
> > >
> > > this option for lz4, or we can provide another option for lz4?
> >
> > We can, though I am not in favour of doing so. There is seemingly
> >
> > little benefit for added complexity.
>
> Agreed.
>
> > > What I think is important for the user when it comes to this
> > >
> > > option is the consistency of its naming across all the tools
> > >
> > > supporting it. pg_dump and pg_basebackup, where we could plug LZ4,
> > >
> > > already use most of the short options you could use for pg_receivewal,
> > >
> > > having only a long one gives a bit more flexibility.
> >
> > Done.
>
> * http://www.zlib.org/rfc-gzip.html.
>
> -   -   For lz4 compressed segments
>
> */
>
> This comment is incomplete.

It is. I will fix.

>
> +#define IsLZ4CompressXLogFileName(fname) \
> -   (strlen(fname) == XLOG_FNAME_LEN + strlen(".lz4") && \
> -   strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN && \
> -   strcmp((fname) + XLOG_FNAME_LEN, ".lz4") == 0)
>
> +#define IsLZ4PartialCompressXLogFileName(fname) \
> -   (strlen(fname) == XLOG_FNAME_LEN + strlen(".lz4.partial") && \
> -   strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN && \
> -   strcmp((fname) + XLOG_FNAME_LEN, ".lz4.partial") == 0)
>
> This is getting complicated. Would it be better to change this stuff
>
> and switch to a routine that checks if a segment has a valid name, is
>
> partial, and the type of compression that applied to it? It seems to
>
> me that we should group iszlibcompress and islz4compress together with
>
> the options available through compression_method.

I agree with you. I will refactor.


> -   if (compresslevel != 0)
> -   {
> - if (compression_method == COMPRESSION_NONE)
>

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

Hi,

please find v2 of the patch which tries to address the commends received so far.

Thank you all for your comments.

Michael Paquier wrote:

> Documentation is missing from the patch.
>
It has now been added.

> +   LZ4F_compressionContext_t ctx;
> +   size_t  outbufCapacity;
> +   void   *outbuf;
> It may be cleaner to refer to lz4 in the name of those variables?

Agreed and done

> +   ctx_out = LZ4F_createCompressionContext(, LZ4F_VERSION);
> +   outbufCapacity = LZ4F_compressBound(LZ4_IN_SIZE, NULL /* default 
> preferences */);
> Interesting.  So this cannot be done at compilation time because of
> the auto-flush mode looking at the LZ4 code.  That looks about right.

This is also my understanding.

> +   system_or_bail('lz4', '-t', $lz4_wals[0]);
> I think that you should just drop this part of the test.  The only
> part of LZ4 that we require to be present when Postgres is built with
> --with-lz4 is its library liblz4.  Commands associated to it may not
> be around, causing this test to fail.  The test checking that one .lz4
> file has been created is good to have.  It may be worth adding a test
> with a .lz4.partial segment generated and --endpos pointing to a LSN
> that does not finish the segment that gets switched.

I humbly disagree with the need for the test. It is rather easily possible
to generate a file that can not be decoded, thus becoming useless. Having the
test will provide some guarantee for the fact. In the current patch, there
is code to find out if the program lz4 is available in the system. If it is
not available, then that specific test is skipped. The rest remains as it
were. Also `system_or_bail` is not used anymore in favour of the `system_log`
so that the test counted and the execution of tests continues upon failure.


> It seems to me that you are missing some logic in
> FindStreamingStart() to handle LZ4-compressed segments, in relation
> with IsCompressXLogFileName() and IsPartialCompressXLogFileName().

Very correct. The logic is now added. Given the lz4 api, one has to fill
in the uncompressed size during creation time. Then one can read the
headers and verify the expectations.


> Should we have more tests for ZLIB, while on it?  That seems like a
> good addition as long as we can skip the tests conditionally when
> that's not supported.

Agreed. Please allow me to provide a distinct patch for this code.


Dilip Kumar wrote:

> Wouldn't it be better to call it compression method instead of
> compression program?

Agreed. This is inline with the suggestions of other reviewers.
Find the change in the attached patch.

> I think we can somehow use "acceleration" parameter of lz4 compression
> to map on compression level, It is not direct mapping but
> can't we create some internal mapping instead of completely ignoring
> this option for lz4, or we can provide another option for lz4?

We can, though I am not in favour of doing so. There is seemingly
little benefit for added complexity.

> Should we also support LZ4 compression using dictionary?

I would we should not do that. If my understanding is correct,
decompression would require the dictionary to be passed along.
The algorithm seems to be very competitive to the current compression
as is.

Magnus Hagander wrote:

> I came here to say exactly that, just had to think up what I thought
> was the better name first. Either method or algorithm, but method
> seems like the much simpler choice and therefore better in this case.
>
> Should is also then not be --compression-method, rather than 
> --compress-method?

Agreed and changed throughout.


Michael Paquier wrote:

> What I think is important for the user when it comes to this
> option is the consistency of its naming across all the tools
> supporting it.  pg_dump and pg_basebackup, where we could plug LZ4,
> already use most of the short options you could use for pg_receivewal,
> having only a long one gives a bit more flexibility.

Done.

Cheers,
//Georgios

v2-0001-Teach-pg_receivewal-to-use-lz4-compression.patch
Description: Binary data

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Friday, July 2nd, 2021 at 03:10, Michael Paquier  wrote:

> On Thu, Jul 01, 2021 at 02:10:17PM +, gkokola...@pm.me wrote:
>
> > Micheal suggested on the same thread to move my entry in the help output so 
> > that
> >
> > the output remains ordered. I would like the options for the compression 
> > method and
> >
> > the already existing compression level to next to each other if possible. 
> > Then it
> >
> > should be either 'X' or 'Y'.
>
> Hmm. Grouping these together makes sense for the user. One choice
>
> that we have here is to drop the short option, and just use a long
>
> one. What I think is important for the user when it comes to this
>
> option is the consistency of its naming across all the tools
>
> supporting it. pg_dump and pg_basebackup, where we could plug LZ4,
>
> already use most of the short options you could use for pg_receivewal,
>
> having only a long one gives a bit more flexibility.


Good point. I am going with that one.


> --
>
> Michael

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Thursday, July 1st, 2021 at 15:58, Magnus Hagander  
wrote:

> On Thu, Jul 1, 2021 at 3:39 PM gkokola...@pm.me wrote:
>
> > ‐‐‐ Original Message ‐‐‐
> >
> > On Thursday, July 1st, 2021 at 12:28, Magnus Hagander mag...@hagander.net 
> > wrote:
> >
> > > On Wed, Jun 30, 2021 at 8:34 AM Dilip Kumar dilipbal...@gmail.com wrote:
> > >
> > > > On Tue, Jun 29, 2021 at 8:15 PM gkokola...@pm.me wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > The program pg_receivewal can use gzip compression to store the 
> > > > > received WAL.
> > > > >
> > > > > This patch teaches it to be able to use lz4 compression if the binary 
> > > > > is build
> > > > >
> > > > > using the -llz4 flag.
> > > >
> > > > +1 for the idea
> > > >
> > > > Some comments/suggestions on the patch
> > > >
> > > > @@ -90,7 +91,8 @@ usage(void)
> > > >
> > > > printf((" --synchronous flush write-ahead log immediately
> > > >
> > > > after writing\n"));
> > > >
> > > > printf((" -v, --verbose output verbose messages\n"));
> > > >
> > > > printf(_(" -V, --version output version information, then exit\n"));
> > > >
> > > > -   printf(_(" -Z, --compress=0-9 compress logs with given
> > > >
> > > > compression level\n"));
> > > >
> > > > -   printf(_(" -I, --compress-program use this program for 
> > > > compression\n"));
> > > >
> > > >
> > > > Wouldn't it be better to call it compression method instead of
> > > >
> > > > compression program?
> > >
> > > I came here to say exactly that, just had to think up what I thought
> > >
> > > was the better name first. Either method or algorithm, but method
> > >
> > > seems like the much simpler choice and therefore better in this case.
> > >
> > > Should is also then not be --compression-method, rather than 
> > > --compress-method?
> >
> > Not a problem. To be very transparent, I first looked what was already out 
> > there.
> >
> > For example `tar` is using
> >
> > -I, --use-compress-program=PROG
> >
> > yet the 'use-' bit would push the alignment of the --help output, so I 
> > removed it.
>
> I think the difference there is that tar actually calls an external
>
> program to do the work... And we are using the built-in library,
>
> right?

You are very correct :) I am not objecting the change at all. Just let you know
how I chose that. You know, naming is dead easy and all...

On a more serious note, what about the `-I` short flag? Should we keep it or
is there a better one to be used?

Micheal suggested on the same thread to move my entry in the help output so that
the output remains ordered. I would like the options for the compression method 
and
the already existing compression level to next to each other if possible. Then 
it
should be either 'X' or 'Y'.

Thoughts?



>
> 
>
> Magnus Hagander
>
> Me: https://www.hagander.net/
>
> Work: https://www.redpill-linpro.com/

Re: Teach pg_receivewal to use lz4 compression

[gkokolatos]

‐‐‐ Original Message ‐‐‐

On Thursday, July 1st, 2021 at 12:28, Magnus Hagander  
wrote:

> On Wed, Jun 30, 2021 at 8:34 AM Dilip Kumar dilipbal...@gmail.com wrote:
>
> > On Tue, Jun 29, 2021 at 8:15 PM gkokola...@pm.me wrote:
> >
> > > Hi,
> > >
> > > The program pg_receivewal can use gzip compression to store the received 
> > > WAL.
> > >
> > > This patch teaches it to be able to use lz4 compression if the binary is 
> > > build
> > >
> > > using the -llz4 flag.
> >
> > +1 for the idea
> >
> > Some comments/suggestions on the patch
> >
> > @@ -90,7 +91,8 @@ usage(void)
> >
> > printf((" --synchronous flush write-ahead log immediately
> >
> > after writing\n"));
> >
> > printf((" -v, --verbose output verbose messages\n"));
> >
> > printf(_(" -V, --version output version information, then exit\n"));
> >
> > -   printf(_(" -Z, --compress=0-9 compress logs with given
> >
> > compression level\n"));
> >
> > -   printf(_(" -I, --compress-program use this program for compression\n"));
> >
> > Wouldn't it be better to call it compression method instead of
> >
> > compression program?
>
> I came here to say exactly that, just had to think up what I thought
>
> was the better name first. Either method or algorithm, but method
>
> seems like the much simpler choice and therefore better in this case.
>
> Should is also then not be --compression-method, rather than 
> --compress-method?

Not a problem. To be very transparent, I first looked what was already out 
there.
For example `tar` is using
-I, --use-compress-program=PROG
yet the 'use-' bit would push the alignment of the --help output, so I removed 
it.

To me, as a non native English speaker, `--compression-method` does sound 
better.
I can just re-align the rest of the help output.

Updated patch is on the making.

>
> --
>
> Magnus Hagander
>
> Me: https://www.hagander.net/
>
> Work: https://www.redpill-linpro.com/

Teach pg_receivewal to use lz4 compression

[gkokolatos]

Hi,

The program pg_receivewal can use gzip compression to store the received WAL.
This patch teaches it to be able to use lz4 compression if the binary is build
using the -llz4 flag.

Previously, the user had to use the option --compress with a value between [0-9]
to denote that gzip compression was requested. This specific behaviour is
maintained. A newly introduced option --compress-program=lz4 can be used to ask
for the logs to be compressed using lz4 instead. In that case, no compression
values can be selected as it does not seem too useful.

Under the hood there is nothing exceptional to be noted. Tar based archives have
not yet been taught to use lz4 compression. Those are used by pg_basebackup. If
is is felt useful, then it is easy to be added in a new patch.

Cheers,
//Georgios

v1-0001-Teach-pg_receivewal-to-use-lz4-compression.patch
Description: Binary data

Re: PATCH: Attempt to make dbsize a bit more consistent

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Wednesday, March 17, 2021 6:35 AM, Michael Paquier  
wrote:

> On Mon, Mar 15, 2021 at 03:10:59PM +0900, Michael Paquier wrote:
>
> > Anyway, as mentioned by other people upthread, I am not really
> > convinced either that we should have more flavors of size functions,
> > particularly depending on the relkind as this would be confusing for
> > the end-user. pg_relation_size() can already do this job for all
> > relkinds that use segment files, so it should still be able to hold
> > its ground and maintain a consistent behavior with what it does
> > currently.
>
> On top of the rest of my notes, there are two more things that would
> face a behavior change if making the existing functions go through
> table AMs, which would scan the data in the smgr:

I am not certain if you are referring to v5 (sent earlier than your mail)
or v4. Can you please verify?

>
> -   After a VACUUM, the relation would be reported with a size of 0,
> while that may not be the case of on-disk files yet.

I am not really following. Apologies for it. The table AM may or may not
choose to go through smgr, depending on the implementation. The only
currently known implementation, heap, does invalidate smgr, based on
what I can see, after a VACUUM. I have not been able to create a test
case both with or without v5 of the patch where not the same result
would be returned.

What have I missed?

>
> -   Temporary tables of other sessions would be accessible.

I am not really certain I am following. Commit 6919b7e3294 from 2012
notes that calculate_relation_size can be safely applied to temp tables
of other sessions. v5 of the patch does not change that behaviour. Nor
did previous versions, but those are already obsolete.

>
> So we would likely want a separate function. Another possibility,
> which I find tempting, would be to push down the calculation logic
> relying on physical files down to the table AM themselves with a new
> dedicated callback (relation_size_physical?), as it seems to me that
> the most important thing users want to know with this set of functions
> is how much physical space is being consumed at one given point in
> time. Attached is a small prototype I was just playing with.
> --
> Michael
>

Re: psql tab completion for \h with IMPORT FOREIGN SCHEMA

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Thursday, March 18, 2021 8:13 AM, Julien Rouhaud  wrote:

> On Thu, Mar 18, 2021 at 03:58:46PM +0900, Michael Paquier wrote:
>
> > Well, as $subject tells, I just found confusing that \h does not
> > complete the so-said command, the only one using IMPORT as keyword,
> > so I'd like to do the following:
> > --- a/src/bin/psql/tab-complete.c
> > +++ b/src/bin/psql/tab-complete.c
> > @@ -1493,7 +1493,7 @@ psql_completion(const char *text, int start, int end)
> > "ABORT", "ALTER", "ANALYZE", "BEGIN", "CALL", "CHECKPOINT", "CLOSE", 
> > "CLUSTER",
> > "COMMENT", "COMMIT", "COPY", "CREATE", "DEALLOCATE", "DECLARE",
> > "DELETE FROM", "DISCARD", "DO", "DROP", "END", "EXECUTE", "EXPLAIN",
> >
> > -  "FETCH", "GRANT", "IMPORT", "INSERT", "LISTEN", "LOAD", "LOCK",
> >
> >
> >
> > -  "FETCH", "GRANT", "IMPORT FOREIGN SCHEMA", "INSERT", "LISTEN", 
> > "LOAD", "LOCK",
> >"MOVE", "NOTIFY", "PREPARE",
> >"REASSIGN", "REFRESH MATERIALIZED VIEW", "REINDEX", "RELEASE",
> >"RESET", "REVOKE", "ROLLBACK",
> >
> >
>
> Looks sensible to me.


It seems helpful. Thank you.

Re: Allow batched insert during cross-partition updates

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Tuesday, March 16, 2021 9:59 AM, Amit Langote  
wrote:

> Hi Georgios,
>
> On Tue, Mar 16, 2021 at 5:12 PM gkokola...@pm.me wrote:
>
> > On Tuesday, March 16, 2021 6:13 AM, Amit Langote amitlangot...@gmail.com 
> > wrote:
> >
> > > On Fri, Mar 12, 2021 at 7:59 PM gkokola...@pm.me wrote:
> > >
> > > > On Friday, March 12, 2021 3:45 AM, Amit Langote amitlangot...@gmail.com 
> > > > wrote:
> > > >
> > > > > By the way, the test case added by commit 927f453a94106 does exercise
> > > > > the code added by this patch, but as I said in the last paragraph, we
> > > > > can't verify that by inspecting EXPLAIN output.
> > > >
> > > > I never doubted that. However, there is a difference. The current patch
> > > > changes the query to be executed in the remote from:
> > > > INSERT INTO  VALUES ($1);
> > > > to:
> > > > INSERT INTO  VALUES ($1), ($2) ... ($n);
> > > > When this patch gets in, it would be very helpful to know that 
> > > > subsequent
> > > > code changes will not cause regressions. So I was wondering if there is
> > > > a way to craft a test case that would break for the code in 
> > > > 927f453a94106
> > > > yet succeed with the current patch.
> > >
> > > The test case "works" both before and after the patch, with the
> > > difference being in the form of the remote query. It seems to me
> > > though that you do get that.
> > >
> > > > I attach version 2 of my small reproduction. I am under the impression 
> > > > that
> > > > in this version, examining the value of cmin in the remote table should
> > > > give an indication of whether the remote received a multiple insert 
> > > > queries
> > > > with a single value, or a single insert query with multiple values.
> > > > Or is this a wrong assumption of mine?
> > >
> > > No, I think you have a good idea here.
> > > I've adjusted that test case to confirm that the batching indeed works
> > > by checking cmin of the moved rows, as you suggest. Please check the
> > > attached updated patch.
> >
> > Excellent. The patch in the current version with the added test seems
> > ready to me.
>
> Thanks for quickly checking that.

A pleasure.

>
> > I would still vote to have accessor functions instead of exposing the
> > whole PartitionTupleRouting struct, but I am not going to hold a too
> > strong stance about it.
>
> I as well, although I would wait for others to chime in before
> updating the patch that way.

Fair enough.

Status updated to RfC in the commitfest app.

>
> --
>
> Amit Langote
> EDB: http://www.enterprisedb.com

Re: Allow batched insert during cross-partition updates

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Tuesday, March 16, 2021 6:13 AM, Amit Langote  
wrote:

> Hi Georgios,
>
> On Fri, Mar 12, 2021 at 7:59 PM gkokola...@pm.me wrote:
>
> > On Friday, March 12, 2021 3:45 AM, Amit Langote amitlangot...@gmail.com 
> > wrote:
> >
> > > On Thu, Mar 11, 2021 at 8:36 PM gkokola...@pm.me wrote:
> > >
> > > > On Thursday, March 11, 2021 9:42 AM, Amit Langote 
> > > > amitlangot...@gmail.com wrote:
> > > >
> > > > > What we do support however is moving rows from a local partition to a
> > > > > remote partition and that involves performing an INSERT on the latter.
> > > > > This patch is for teaching those INSERTs to use batched mode if
> > > > > allowed, which is currently prohibited. So with this patch, if an
> > > > > UPDATE moves 10 rows from a local partition to a remote partition,
> > > > > then they will be inserted with a single INSERT command containing all
> > > > > 10 rows, instead of 10 separate INSERT commands.
> > > >
> > > > So, if I understand correctly then in my previously attached repro I
> > > > should have written instead:
> > > >
> > > > CREATE TABLE local_root_remote_partitions (a int) PARTITION BY LIST 
> > > > ( a );
> > > > CREATE TABLE
> > > > local_root_local_partition_1
> > > > PARTITION OF
> > > > local_root_remote_partitions FOR VALUES IN (1);
> > > >
> > > > CREATE FOREIGN TABLE
> > > > local_root_remote_partition_2
> > > > PARTITION OF
> > > > local_root_remote_partitions FOR VALUES IN (2)
> > > > SERVER
> > > > remote_server
> > > > OPTIONS (
> > > > table_name 'remote_partition_2',
> > > > batch_size '10'
> > > > );
> > > >
> > > > INSERT INTO local_root_remote_partitions VALUES (1), (1);
> > > > -- Everything should be on local_root_local_partition_1 and on the 
> > > > same transaction
> > > > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM 
> > > > local_root_remote_partitions;
> > > >
> > > > UPDATE local_root_remote_partitions SET a = 2;
> > > > -- Everything should be on remote_partition_2 and on the same 
> > > > transaction
> > > > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM 
> > > > local_root_remote_partitions;
> > > >
> > > >
> > > > I am guessing that I am still wrong because the UPDATE operation above 
> > > > will
> > > > fail due to the restrictions imposed in postgresBeginForeignInsert 
> > > > regarding
> > > > UPDATES.
> > >
> > > Yeah, for the move to work without hitting the restriction you
> > > mention, you will need to write the UPDATE query such that
> > > local_root_remote_partition_2 is not updated. For example, as
> > > follows:
> > > UPDATE local_root_remote_partitions SET a = 2 WHERE a <> 2;
> > > With this query, the remote partition is not one of the result
> > > relations to be updated, so is able to escape that restriction.
> >
> > Excellent. Thank you for the explanation and patience.
> >
> > > > Would it be too much to ask for the addition of a test case that will
> > > > demonstrate the change of behaviour found in patch.
> > >
> > > Hmm, I don't think there's a way to display whether the INSERT done on
> > > the remote partition as a part of an (tuple-moving) UPDATE used
> > > batching or not. That's because that INSERT's state is hidden from
> > > EXPLAIN. Maybe we should change EXPLAIN to make it show such hidden
> > > INSERT's state (especially its batch size) under the original UPDATE
> > > node, but I am not sure.
> >
> > Yeah, there does not seem to be a way for explain to do show that 
> > information
> > with the current code.
> >
> > > By the way, the test case added by commit 927f453a94106 does exercise
> > > the code added by this patch, but as I said in the last paragraph, we
> > > can't verify that by inspecting EXPLAIN output.
> >
> > I never doubted that. However, there is a difference. The current patch
> > changes the query to be executed in the remote from:
> > INSERT INTO  VALUES ($1);
> > to:
> > INSERT INTO  VALUES ($1), ($2) ... ($n);
> > When this patch gets in, it would be very helpful to know that subsequent
> > code changes will not cause regressions. So I was wondering if there is
> > a way to craft a test case that would break for the code in 927f453a94106
> > yet succeed with the current patch.
>
> The test case "works" both before and after the patch, with the
> difference being in the form of the remote query. It seems to me
> though that you do get that.
>
> > I attach version 2 of my small reproduction. I am under the impression that
> > in this version, examining the value of cmin in the remote table should
> > give an indication of whether the remote received a multiple insert queries
> > with a single value, or a single insert query with multiple values.
> > Or is this a wrong assumption of mine?
>
> No, I think you have a good idea here.

Thank you.

>
> I've adjusted that test case to confirm that the batching indeed works
> by 

Re: PATCH: Attempt to make dbsize a bit more consistent

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Monday, March 15, 2021 7:10 AM, Michael Paquier  wrote:

 On Wed, Feb 24, 2021 at 02:35:51PM +, gkokola...@pm.me wrote:

  Now with attachment. Apologies for the chatter.

 The patch has no documentation for the two new functions, so it is a
 bit hard to understand what is the value brought here, and what is the
 goal wanted just by reading the patch, except that this switches the
 size reported for views to NULL instead of zero bytes by reading the
 regression tests.

Understood and agreed. Thank you very much for looking.


 Please note that reporting zero is not wrong for views IMO as these
 have no physical storage, so, while it is possible to argue that a
 size of zero could imply that this relation size could not be zero, it
 will never change, so I'd rather let this behavior as it is. I
 would bet, additionally, that this could break existing use cases.
 Reporting NULL, on the contrary, as your patch does, makes things
 worse in some ways because that's what pg_relation_size would report
 when a relation does not exist anymore. Imagine for example the case
 of a DROP TABLE run in parallel of a scan of pg_class using
 pg_relation_size(). So it becomes impossible to know if a relation
 has been removed or not. This joins some points raised by Soumyadeep
 upthread.

Yeah, I would have agreed with you before actually looking a bit closer.
I think that the gist of it is that the intention of the functions and
the actual usage/documentation of those have diverged. I honestly thought
that pg_relation_size() was a general function intended for any kind of
relation, whereas pg_table_size() was a function intended only for tables
(toast and mat views included).

Then I read the comment describing pg_relation_size() which reads
'disk space usage for the main fork of the specified table or index' and
'disk space usage for the specified fork of a table or index', found
initially in commit 358a897fa1d and maintained since.

But I digress.



 Anyway, as mentioned by other people upthread, I am not really
 convinced either that we should have more flavors of size functions,
 particularly depending on the relkind as this would be confusing for
 the end-user. pg_relation_size() can already do this job for all
 relkinds that use segment files, so it should still be able to hold
 its ground and maintain a consistent behavior with what it does
 currently.

I must have missunderstood the other people upthread and I thought
that new flavours were requested. Thank you for clarifying and
correcting me.


 +static int64
 +calculate_table_fork_size(Relation rel, ForkNumber forkNum)
 +{

 -   return (int64)table_relation_size(rel, forkNum);
 +}
 Why isn't that just unsigned, like table_relation_size()? This is an
 internal function so it does not matter to changes its signature, but
 I think that you had better do a cast at a higher level instead.

 for (int i = 0; i  MAX_FORKNUM; i++)


 - nblocks += smgrnblocks(rel-rd_smgr, i);



 - nblocks += smgrexists(rel-rd_smgr, i)


 - ? smgrnblocks(rel-rd_smgr, i)


 - : 0;



 Is that actually the part for views? Why is that done this way?

No, it is not for views. The codebase should never reach this function
for a view or it would be a serious bug elsewhere.

This is addressing a bug in table_relation_size(). This function is correctly
not requiring for its caller to know any specifics about the forks. A heap
table is not required to have created any fsm, or vm, forks neither. Finally
smgrnblocks() assumes that the fork actually exists or errors out.

This change, makes certain that calling table_relation_size() with a
non existing fork will not generate an error.


 -   if (rel-rd_rel-relkind == RELKIND_RELATION ||

 - rel-rd_rel-relkind == RELKIND_TOASTVALUE ||


 - rel-rd_rel-relkind == RELKIND_MATVIEW)


 - size = calculate_table_fork_size(rel,


 -  forkname_to_number(text_to_cstring(forkName)));


 -   else if (rel-rd_rel-relkind == RELKIND_INDEX)

 - size = calculate_relation_size((rel-rd_node), 
rel-rd_backend,


 -  forkname_to_number(text_to_cstring(forkName)));


 -   else

 -   {

 - relation_close(rel, AccessShareLock);


 - PG_RETURN_NULL();


 -   }
 The approach you are taking to bring some consistency in all that by
 making the size estimations go through table AMs via
 table_relation_size() is promising. However, this code breaks the
 size estimation for sequences, which is not good. If attempting to
 use an evaluation that's based on a table AM, shouldn't this code use
 a check based on rd_tableam rather than the relkind then? We assume
 that rd_tableam is set depending on the relkind in relcache.c, for
 one.


Thank you for the remarks. Please find v5 attached which is a minimal
patch to try to use the table am api 

Re: Allow batched insert during cross-partition updates

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Friday, March 12, 2021 3:45 AM, Amit Langote  wrote:

> On Thu, Mar 11, 2021 at 8:36 PM gkokola...@pm.me wrote:
>
> > On Thursday, March 11, 2021 9:42 AM, Amit Langote amitlangot...@gmail.com 
> > wrote:
> >
> > > On Wed, Mar 10, 2021 at 9:30 PM Georgios gkokola...@protonmail.com wrote:
> > >
> > > > I continued looking a bit at the patch, yet I am either failing to see 
> > > > fix or I am
> > > > looking at the wrong thing. Please find attached a small repro of what 
> > > > my expectetions
> > > > were.
> > > > As you can see in the repro, I would expect the
> > > > UPDATE local_root_remote_partitions SET a = 2;
> > > > to move the tuples to remote_partition_2 on the same transaction.
> > > > However this is not the case, with or without the patch.
> > > > Is my expectation of this patch wrong?
> > >
> > > I think yes. We currently don't have the feature you are looking for
> > > -- moving tuples from one remote partition to another remote
> > > partition. This patch is not for adding that feature.
> >
> > Thank you for correcting me.
> >
> > > What we do support however is moving rows from a local partition to a
> > > remote partition and that involves performing an INSERT on the latter.
> > > This patch is for teaching those INSERTs to use batched mode if
> > > allowed, which is currently prohibited. So with this patch, if an
> > > UPDATE moves 10 rows from a local partition to a remote partition,
> > > then they will be inserted with a single INSERT command containing all
> > > 10 rows, instead of 10 separate INSERT commands.
> >
> > So, if I understand correctly then in my previously attached repro I
> > should have written instead:
> >
> > CREATE TABLE local_root_remote_partitions (a int) PARTITION BY LIST ( a 
> > );
> > CREATE TABLE
> > local_root_local_partition_1
> > PARTITION OF
> > local_root_remote_partitions FOR VALUES IN (1);
> >
> > CREATE FOREIGN TABLE
> > local_root_remote_partition_2
> > PARTITION OF
> > local_root_remote_partitions FOR VALUES IN (2)
> > SERVER
> > remote_server
> > OPTIONS (
> > table_name 'remote_partition_2',
> > batch_size '10'
> > );
> >
> > INSERT INTO local_root_remote_partitions VALUES (1), (1);
> > -- Everything should be on local_root_local_partition_1 and on the same 
> > transaction
> > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM 
> > local_root_remote_partitions;
> >
> > UPDATE local_root_remote_partitions SET a = 2;
> > -- Everything should be on remote_partition_2 and on the same 
> > transaction
> > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM 
> > local_root_remote_partitions;
> >
> >
> > I am guessing that I am still wrong because the UPDATE operation above will
> > fail due to the restrictions imposed in postgresBeginForeignInsert regarding
> > UPDATES.
>
> Yeah, for the move to work without hitting the restriction you
> mention, you will need to write the UPDATE query such that
> local_root_remote_partition_2 is not updated. For example, as
> follows:
>
> UPDATE local_root_remote_partitions SET a = 2 WHERE a <> 2;
>
> With this query, the remote partition is not one of the result
> relations to be updated, so is able to escape that restriction.

Excellent. Thank you for the explanation and patience.

>
> > Would it be too much to ask for the addition of a test case that will
> > demonstrate the change of behaviour found in patch.
>
> Hmm, I don't think there's a way to display whether the INSERT done on
> the remote partition as a part of an (tuple-moving) UPDATE used
> batching or not. That's because that INSERT's state is hidden from
> EXPLAIN. Maybe we should change EXPLAIN to make it show such hidden
> INSERT's state (especially its batch size) under the original UPDATE
> node, but I am not sure.

Yeah, there does not seem to be a way for explain to do show that information
with the current code.

>
> By the way, the test case added by commit 927f453a94106 does exercise
> the code added by this patch, but as I said in the last paragraph, we
> can't verify that by inspecting EXPLAIN output.

I never doubted that. However, there is a difference. The current patch
changes the query to be executed in the remote from:

   INSERT INTO  VALUES ($1);
to:
   INSERT INTO  VALUES ($1), ($2) ... ($n);

When this patch gets in, it would be very helpful to know that subsequent
code changes will not cause regressions. So I was wondering if there is
a way to craft a test case that would break for the code in 927f453a94106
yet succeed with the current patch.

I attach version 2 of my small reproduction. I am under the impression that
in this version, examining the value of cmin in the remote table should
give an indication of whether the remote received a multiple insert queries
with a single value, or a single insert query with multiple values.

Or is this a wrong assumption of mine?

Re: Allow batched insert during cross-partition updates

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Thursday, March 11, 2021 9:42 AM, Amit Langote  
wrote:

> Hi Georgios,
>
> On Wed, Mar 10, 2021 at 9:30 PM Georgios gkokola...@protonmail.com wrote:
>
> > I continued looking a bit at the patch, yet I am either failing to see fix 
> > or I am
> > looking at the wrong thing. Please find attached a small repro of what my 
> > expectetions
> > were.
> > As you can see in the repro, I would expect the
> > UPDATE local_root_remote_partitions SET a = 2;
> > to move the tuples to remote_partition_2 on the same transaction.
> > However this is not the case, with or without the patch.
> > Is my expectation of this patch wrong?
>
> I think yes. We currently don't have the feature you are looking for
> -- moving tuples from one remote partition to another remote
> partition. This patch is not for adding that feature.

Thank you for correcting me.

>
> What we do support however is moving rows from a local partition to a
> remote partition and that involves performing an INSERT on the latter.
> This patch is for teaching those INSERTs to use batched mode if
> allowed, which is currently prohibited. So with this patch, if an
> UPDATE moves 10 rows from a local partition to a remote partition,
> then they will be inserted with a single INSERT command containing all
> 10 rows, instead of 10 separate INSERT commands.

So, if I understand correctly then in my previously attached repro I
should have written instead:

CREATE TABLE local_root_remote_partitions (a int) PARTITION BY LIST ( a );
CREATE TABLE
local_root_local_partition_1
PARTITION OF
local_root_remote_partitions FOR VALUES IN (1);

CREATE FOREIGN TABLE
local_root_remote_partition_2
PARTITION OF
local_root_remote_partitions FOR VALUES IN (2)
SERVER
remote_server
OPTIONS (
table_name 'remote_partition_2',
batch_size '10'
);

INSERT INTO local_root_remote_partitions VALUES (1), (1);
-- Everything should be on local_root_local_partition_1 and on the same 
transaction
SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM 
local_root_remote_partitions;

UPDATE local_root_remote_partitions SET a = 2;
-- Everything should be on remote_partition_2 and on the same transaction
SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM 
local_root_remote_partitions;


I am guessing that I am still wrong because the UPDATE operation above will
fail due to the restrictions imposed in postgresBeginForeignInsert regarding
UPDATES.

Would it be too much to ask for the addition of a test case that will
demonstrate the change of behaviour found in patch?

Cheers,
//Georgios

>
> -
>
> Amit Langote
> EDB: http://www.enterprisedb.com

Re: New default role- 'pg_read_all_data'

[gkokolatos]

Hi,




‐‐‐ Original Message ‐‐‐
On Monday, November 23, 2020 11:31 PM, Stephen Frost  wrote:

> Greetings,
>
> -   Anastasia Lubennikova (a.lubennik...@postgrespro.ru) wrote:
>
> > On 29.10.2020 17:19, Stephen Frost wrote:
> >
> > > -   Georgios Kokolatos (gkokola...@protonmail.com) wrote:
> > >
> > > > this patch is in "Ready for committer" state and the Cfbot is happy.
> > > > Glad that's still the case. :)
> > >
> > > > Is there any committer that is available for taking a look at it?
> > > > If there aren't any objections or further comments, I'll take another
> > > > look through it and will commit it during the upcoming CF.
> > >
> > > Thanks!
> > > Stephen
> >
> > CFM reminder. Just in case you forgot about this thread)
> > The commitfest is heading to the end. And there was a plenty of time for
> > anyone to object.
>
> Thanks, I've not forgotten, but it's a bit complicated given that I've
> another patch in progress to rename default roles to be predefined
> roles which conflicts with this one. Hopefully we'll have a few
> comments on that and I can get it committed and this one updated with
> the new naming. I'd rather not commit this one and then immediately
> commit changes over top of it.

May I enquire about the status of the current?


//Georgios

>
> Thanks again!
>
> Stephen

Re: PATCH: Attempt to make dbsize a bit more consistent

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Wednesday, February 24, 2021 3:34 PM,  wrote:

>
>
> ‐‐‐ Original Message ‐‐‐
> On Friday, February 19, 2021 4:57 PM, gkokola...@pm.me wrote:
>
> > ‐‐‐ Original Message ‐‐‐
> > On Monday, February 1, 2021 1:18 PM, Masahiko Sawada sawada.m...@gmail.com 
> > wrote:
> >
> > > On Thu, Nov 12, 2020 at 2:54 AM Soumyadeep Chakraborty
> > > soumyadeep2...@gmail.com wrote:
> > >
> > > > Hey Georgios,
> > > > On Tue, Nov 10, 2020 at 6:20 AM gkokola...@pm.me wrote:
> > > >
> > > > > ‐‐‐ Original Message ‐‐‐
> > > > > On Monday, November 9, 2020 7:50 PM, Soumyadeep Chakraborty 
> > > > > soumyadeep2...@gmail.com wrote:
> > > > >
> > > > > > Hey Georgios,
> > > > > > Thanks for looking for more avenues to invoke tableAM APIS! Please 
> > > > > > find
> > > > > > my review below:
> > > > >
> > > > > A great review Soumyadeep, it is much appreciated.
> > > > > Please remember to add yourself as a reviewer in the commitfest
> > > > > [https://commitfest.postgresql.org/30/2701/]
> > > >
> > > > Ah yes. Sorry, I forgot that!
> > > >
> > > > > > On Tue, Oct 13, 2020 at 6:28 AM gkokola...@pm.me wrote:
> > > > > >
> > > > > > 1.
> > > > > >
> > > > > > > /*
> > > > > > >
> > > > > > > -   -   heap size, including FSM and VM
> > > > > > > -   -   table size, including FSM and VM
> > > > > > > */
> > > > > > >
> > > > > >
> > > > > > We should not mention FSM and VM in dbsize.c at all as these are
> > > > > > heap AM specific. We can say:
> > > > > > table size, excluding TOAST relation
> > > > >
> > > > > Yeah, I was thinking that the notion that FSM and VM are still taken
> > > > > into account should be stated. We are iterating over ForkNumber
> > > > > after all.
> > > > > How about phrasing it as:
> > > > >
> > > > > -   table size, including all implemented forks from the AM (e.g. 
> > > > > FSM, VM)
> > > > > -   excluding TOAST relations
> > > > >
> > > > > Thoughts?
> > > >
> > > > Yes, I was thinking along the same lines. The concept of a "fork" forced
> > > > should not be forced down into the tableAM. But that is a discussion for
> > > > another day. We can probably say:
> > > >
> > > > -   table size, including all implemented forks from the AM (e.g. FSM, 
> > > > VM
> > > > -   for the heap AM) excluding TOAST relations
> > > >
> > > > > > 2.
> > > > > >
> > > > > > > /*
> > > > > > >
> > > > > > > -   Size of toast relation
> > > > > > > */
> > > > > > > if (OidIsValid(rel->rd_rel->reltoastrelid))
> > > > > > >
> > > > > > > -   size += 
> > > > > > > calculate_toast_table_size(rel->rd_rel->reltoastrelid);
> > > > > > >
> > > > > > > -   {
> > > > > > >
> > > > > > > -   Relation toastRel;
> > > > > > >
> > > > > > > -
> > > > > > > -   toastRel = relation_open(rel->rd_rel->reltoastrelid, 
> > > > > > > AccessShareLock);
> > > > > > >
> > > > > >
> > > > > > We can replace the OidIsValid check with 
> > > > > > relation_needs_toast_table()
> > > > > > and then have the OidIsValid() check in an Assert. Perhaps, that 
> > > > > > will
> > > > > > make things more readable.
> > > > >
> > > > > Please, allow me to kindly disagree.
> > > > > Relation is already open at this stage. Even create_toast_table(), the
> > > > > internal workhorse for creating toast relations, does check 
> > > > > reltoastrelid
> > > > > to test if the relation is already toasted.
> > > > > Furthermore, we do call:
> > > > >
> > > > > -   toastRel = relation_open(rel->rd_rel->reltoastrelid, 
> > > > > AccessShareLock);
> > > > >
> > > > > and in order to avoid elog() errors underneath us, we ought to have
> > > > > verified the validity of reltoastrelid.
> > > > > In short, I think that the code in the proposal is not too unreadable
> > > > > nor that it breaks the coding patterns throughout the codebase.
> > > > > Am I too wrong?
> > > >
> > > > No not at all. The code in the proposal is indeed adhering to the
> > > > codebase. What I was going for here was to increase the usage of
> > > > relation_needs_toast_table(). What I meant was:
> > > > if (table_relation_needs_toast_table(rel))
> > > > {
> > > > if (!OidIsValid(rel->rd_rel->reltoastrelid))
> > > > {
> > > > elog(ERROR, );
> > > > }
> > > > //size calculation here..
> > > > }
> > > > We want to error out if the toast table can't be found and the relation
> > > > is expected to have one, which the existing code doesn't handle.
> > > >
> > > > > > 3.
> > > > > >
> > > > > > > -   if (rel->rd_rel->relkind == RELKIND_RELATION ||
> > > > > > > -   rel->rd_rel->relkind == RELKIND_TOASTVALUE ||
> > > > > > > -   rel->rd_rel->relkind == RELKIND_MATVIEW)
> > > > > > > -   size = calculate_table_size(rel);
> > > > > > > -   else
> > > > > > > -   {
> > > > > > > -   relation_close(rel, AccessShareLock);
> > > > > > > -   PG_RETURN_NULL();
> > > > > > > -   }
> > > > > >
> > > > > > This leads to behavioral changes:
> > > > > > I am talking about the case where one calls pg_table_size() on an 
> > > > > > index.
> 

Re: PATCH: Attempt to make dbsize a bit more consistent

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Friday, February 19, 2021 4:57 PM,  wrote:

>
>
> ‐‐‐ Original Message ‐‐‐
> On Monday, February 1, 2021 1:18 PM, Masahiko Sawada sawada.m...@gmail.com 
> wrote:
>
> > On Thu, Nov 12, 2020 at 2:54 AM Soumyadeep Chakraborty
> > soumyadeep2...@gmail.com wrote:
> >
> > > Hey Georgios,
> > > On Tue, Nov 10, 2020 at 6:20 AM gkokola...@pm.me wrote:
> > >
> > > > ‐‐‐ Original Message ‐‐‐
> > > > On Monday, November 9, 2020 7:50 PM, Soumyadeep Chakraborty 
> > > > soumyadeep2...@gmail.com wrote:
> > > >
> > > > > Hey Georgios,
> > > > > Thanks for looking for more avenues to invoke tableAM APIS! Please 
> > > > > find
> > > > > my review below:
> > > >
> > > > A great review Soumyadeep, it is much appreciated.
> > > > Please remember to add yourself as a reviewer in the commitfest
> > > > [https://commitfest.postgresql.org/30/2701/]
> > >
> > > Ah yes. Sorry, I forgot that!
> > >
> > > > > On Tue, Oct 13, 2020 at 6:28 AM gkokola...@pm.me wrote:
> > > > >
> > > > > 1.
> > > > >
> > > > > > /*
> > > > > >
> > > > > > -   -   heap size, including FSM and VM
> > > > > > -   -   table size, including FSM and VM
> > > > > > */
> > > > > >
> > > > >
> > > > > We should not mention FSM and VM in dbsize.c at all as these are
> > > > > heap AM specific. We can say:
> > > > > table size, excluding TOAST relation
> > > >
> > > > Yeah, I was thinking that the notion that FSM and VM are still taken
> > > > into account should be stated. We are iterating over ForkNumber
> > > > after all.
> > > > How about phrasing it as:
> > > >
> > > > -   table size, including all implemented forks from the AM (e.g. FSM, 
> > > > VM)
> > > > -   excluding TOAST relations
> > > >
> > > > Thoughts?
> > >
> > > Yes, I was thinking along the same lines. The concept of a "fork" forced
> > > should not be forced down into the tableAM. But that is a discussion for
> > > another day. We can probably say:
> > >
> > > -   table size, including all implemented forks from the AM (e.g. FSM, VM
> > > -   for the heap AM) excluding TOAST relations
> > >
> > > > > 2.
> > > > >
> > > > > > /*
> > > > > >
> > > > > > -   Size of toast relation
> > > > > > */
> > > > > > if (OidIsValid(rel->rd_rel->reltoastrelid))
> > > > > >
> > > > > > -   size += calculate_toast_table_size(rel->rd_rel->reltoastrelid);
> > > > > >
> > > > > > -   {
> > > > > >
> > > > > > -   Relation toastRel;
> > > > > >
> > > > > > -
> > > > > > -   toastRel = relation_open(rel->rd_rel->reltoastrelid, 
> > > > > > AccessShareLock);
> > > > > >
> > > > >
> > > > > We can replace the OidIsValid check with relation_needs_toast_table()
> > > > > and then have the OidIsValid() check in an Assert. Perhaps, that will
> > > > > make things more readable.
> > > >
> > > > Please, allow me to kindly disagree.
> > > > Relation is already open at this stage. Even create_toast_table(), the
> > > > internal workhorse for creating toast relations, does check 
> > > > reltoastrelid
> > > > to test if the relation is already toasted.
> > > > Furthermore, we do call:
> > > >
> > > > -   toastRel = relation_open(rel->rd_rel->reltoastrelid, 
> > > > AccessShareLock);
> > > >
> > > > and in order to avoid elog() errors underneath us, we ought to have
> > > > verified the validity of reltoastrelid.
> > > > In short, I think that the code in the proposal is not too unreadable
> > > > nor that it breaks the coding patterns throughout the codebase.
> > > > Am I too wrong?
> > >
> > > No not at all. The code in the proposal is indeed adhering to the
> > > codebase. What I was going for here was to increase the usage of
> > > relation_needs_toast_table(). What I meant was:
> > > if (table_relation_needs_toast_table(rel))
> > > {
> > > if (!OidIsValid(rel->rd_rel->reltoastrelid))
> > > {
> > > elog(ERROR, );
> > > }
> > > //size calculation here..
> > > }
> > > We want to error out if the toast table can't be found and the relation
> > > is expected to have one, which the existing code doesn't handle.
> > >
> > > > > 3.
> > > > >
> > > > > > -   if (rel->rd_rel->relkind == RELKIND_RELATION ||
> > > > > > -   rel->rd_rel->relkind == RELKIND_TOASTVALUE ||
> > > > > > -   rel->rd_rel->relkind == RELKIND_MATVIEW)
> > > > > > -   size = calculate_table_size(rel);
> > > > > > -   else
> > > > > > -   {
> > > > > > -   relation_close(rel, AccessShareLock);
> > > > > > -   PG_RETURN_NULL();
> > > > > > -   }
> > > > >
> > > > > This leads to behavioral changes:
> > > > > I am talking about the case where one calls pg_table_size() on an 
> > > > > index.
> > > > > W/o your patch, it returns the size of the index. W/ your patch it
> > > > > returns NULL. Judging by the documentation, this function should not
> > > > > ideally apply to indexes but it does! I have a sinking feeling that 
> > > > > lots
> > > > > of users use it for this purpose, as there is no function to calculate
> > > > > the size of a single specific index (except pg_relation_size()).

Re: PATCH: Attempt to make dbsize a bit more consistent

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Monday, February 1, 2021 1:18 PM, Masahiko Sawada  
wrote:

> On Thu, Nov 12, 2020 at 2:54 AM Soumyadeep Chakraborty
> soumyadeep2...@gmail.com wrote:
>
> > Hey Georgios,
> > On Tue, Nov 10, 2020 at 6:20 AM gkokola...@pm.me wrote:
> >
> > > ‐‐‐ Original Message ‐‐‐
> > > On Monday, November 9, 2020 7:50 PM, Soumyadeep Chakraborty 
> > > soumyadeep2...@gmail.com wrote:
> > >
> > > > Hey Georgios,
> > > > Thanks for looking for more avenues to invoke tableAM APIS! Please find
> > > > my review below:
> > >
> > > A great review Soumyadeep, it is much appreciated.
> > > Please remember to add yourself as a reviewer in the commitfest
> > > [https://commitfest.postgresql.org/30/2701/]
> >
> > Ah yes. Sorry, I forgot that!
> >
> > > > On Tue, Oct 13, 2020 at 6:28 AM gkokola...@pm.me wrote:
> > > >
> > > > 1.
> > > >
> > > > > /*
> > > > >
> > > > > -   -   heap size, including FSM and VM
> > > > > -   -   table size, including FSM and VM
> > > > > */
> > > > >
> > > >
> > > > We should not mention FSM and VM in dbsize.c at all as these are
> > > > heap AM specific. We can say:
> > > > table size, excluding TOAST relation
> > >
> > > Yeah, I was thinking that the notion that FSM and VM are still taken
> > > into account should be stated. We are iterating over ForkNumber
> > > after all.
> > > How about phrasing it as:
> > >
> > > -   table size, including all implemented forks from the AM (e.g. FSM, VM)
> > > -   excluding TOAST relations
> > >
> > > Thoughts?
> >
> > Yes, I was thinking along the same lines. The concept of a "fork" forced
> > should not be forced down into the tableAM. But that is a discussion for
> > another day. We can probably say:
> >
> > -   table size, including all implemented forks from the AM (e.g. FSM, VM
> > -   for the heap AM) excluding TOAST relations
> >
> > > > 2.
> > > >
> > > > > /*
> > > > >
> > > > > -   Size of toast relation
> > > > > */
> > > > > if (OidIsValid(rel->rd_rel->reltoastrelid))
> > > > >
> > > > > -   size += calculate_toast_table_size(rel->rd_rel->reltoastrelid);
> > > > >
> > > > > -   {
> > > > >
> > > > > -   Relation toastRel;
> > > > >
> > > > > -
> > > > > -   toastRel = relation_open(rel->rd_rel->reltoastrelid, 
> > > > > AccessShareLock);
> > > > >
> > > >
> > > > We can replace the OidIsValid check with relation_needs_toast_table()
> > > > and then have the OidIsValid() check in an Assert. Perhaps, that will
> > > > make things more readable.
> > >
> > > Please, allow me to kindly disagree.
> > > Relation is already open at this stage. Even create_toast_table(), the
> > > internal workhorse for creating toast relations, does check reltoastrelid
> > > to test if the relation is already toasted.
> > > Furthermore, we do call:
> > >
> > > -   toastRel = relation_open(rel->rd_rel->reltoastrelid, AccessShareLock);
> > >
> > > and in order to avoid elog() errors underneath us, we ought to have
> > > verified the validity of reltoastrelid.
> > > In short, I think that the code in the proposal is not too unreadable
> > > nor that it breaks the coding patterns throughout the codebase.
> > > Am I too wrong?
> >
> > No not at all. The code in the proposal is indeed adhering to the
> > codebase. What I was going for here was to increase the usage of
> > relation_needs_toast_table(). What I meant was:
> > if (table_relation_needs_toast_table(rel))
> > {
> > if (!OidIsValid(rel->rd_rel->reltoastrelid))
> > {
> > elog(ERROR, );
> > }
> > //size calculation here..
> > }
> > We want to error out if the toast table can't be found and the relation
> > is expected to have one, which the existing code doesn't handle.
> >
> > > > 3.
> > > >
> > > > > -   if (rel->rd_rel->relkind == RELKIND_RELATION ||
> > > > > -   rel->rd_rel->relkind == RELKIND_TOASTVALUE ||
> > > > > -   rel->rd_rel->relkind == RELKIND_MATVIEW)
> > > > > -   size = calculate_table_size(rel);
> > > > > -   else
> > > > > -   {
> > > > > -   relation_close(rel, AccessShareLock);
> > > > > -   PG_RETURN_NULL();
> > > > > -   }
> > > >
> > > > This leads to behavioral changes:
> > > > I am talking about the case where one calls pg_table_size() on an index.
> > > > W/o your patch, it returns the size of the index. W/ your patch it
> > > > returns NULL. Judging by the documentation, this function should not
> > > > ideally apply to indexes but it does! I have a sinking feeling that lots
> > > > of users use it for this purpose, as there is no function to calculate
> > > > the size of a single specific index (except pg_relation_size()).
> > > > The same argument I have made above applies to sequences. Users may have
> > > > trial-and-errored their way into finding that pg_table_size() can tell
> > > > them the size of a specific index/sequence! I don't know how widespread
> > > > the use is in the user community, so IMO maybe we should be conservative
> > > > and not introduce this change? Alternatively, we could call out that
> > > > 

Re: PATCH: Attempt to make dbsize a bit more consistent

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Monday, November 9, 2020 7:50 PM, Soumyadeep Chakraborty 
 wrote:

> Hey Georgios,
>
> Thanks for looking for more avenues to invoke tableAM APIS! Please find
> my review below:

A great review Soumyadeep, it is much appreciated.
Please remember to add yourself as a reviewer in the commitfest
[https://commitfest.postgresql.org/30/2701/]

>
> On Tue, Oct 13, 2020 at 6:28 AM gkokola...@pm.me wrote:
>
> 1.
>
> > /*
> >
> > -   -   heap size, including FSM and VM
> >
> > -   -   table size, including FSM and VM
> > */
> >
>
> We should not mention FSM and VM in dbsize.c at all as these are
> heap AM specific. We can say:
> table size, excluding TOAST relation

Yeah, I was thinking that the notion that FSM and VM are still taken
into account should be stated. We are iterating over ForkNumber
after all.

How about phrasing it as:

+ table size, including all implemented forks from the AM (e.g. FSM, VM)
+ excluding TOAST relations

Thoughts?

>
> 2.
>
> > /*
> >
> > -   Size of toast relation
> > */
> > if (OidIsValid(rel->rd_rel->reltoastrelid))
> >
> >
> > -   size += calculate_toast_table_size(rel->rd_rel->reltoastrelid);
> >
> > -   {
> > -   Relation toastRel;
> > -
> > -   toastRel = relation_open(rel->rd_rel->reltoastrelid, AccessShareLock);
>
> We can replace the OidIsValid check with relation_needs_toast_table()
> and then have the OidIsValid() check in an Assert. Perhaps, that will
> make things more readable.

Please, allow me to kindly disagree.

Relation is already open at this stage. Even create_toast_table(), the
internal workhorse for creating toast relations, does check reltoastrelid
to test if the relation is already toasted.

Furthermore, we do call:

+ toastRel = relation_open(rel->rd_rel->reltoastrelid, AccessShareLock);

and in order to avoid elog() errors underneath us, we ought to have
verified the validity of reltoastrelid.

In short, I think that the code in the proposal is not too unreadable
nor that it breaks the coding patterns throughout the codebase.

Am I too wrong?

>
> 3.
>
> > -   if (rel->rd_rel->relkind == RELKIND_RELATION ||
> > -   rel->rd_rel->relkind == RELKIND_TOASTVALUE ||
> > -   rel->rd_rel->relkind == RELKIND_MATVIEW)
> > -   size = calculate_table_size(rel);
> > -   else
> > -   {
> > -   relation_close(rel, AccessShareLock);
> > -   PG_RETURN_NULL();
> > -   }
>
> This leads to behavioral changes:
>
> I am talking about the case where one calls pg_table_size() on an index.
> W/o your patch, it returns the size of the index. W/ your patch it
> returns NULL. Judging by the documentation, this function should not
> ideally apply to indexes but it does! I have a sinking feeling that lots
> of users use it for this purpose, as there is no function to calculate
> the size of a single specific index (except pg_relation_size()).
> The same argument I have made above applies to sequences. Users may have
> trial-and-errored their way into finding that pg_table_size() can tell
> them the size of a specific index/sequence! I don't know how widespread
> the use is in the user community, so IMO maybe we should be conservative
> and not introduce this change? Alternatively, we could call out that
> pg_table_size() is only for tables by throwing an error if anything
> other than a table is passed in.
>
> If we decide to preserve the existing behavior of the pg_table_size():
> It seems that for things not backed by the tableAM (indexes and
> sequences), they should still go through calculate_relation_size().
> We can call table_relation_size() based on if relkind is
> RELKIND_RELATION, RELKIND_TOASTVALUE or RELKIND_MATVIEW. Perhaps it
> might be worthwhile making a new macro RELKIND_HAS_TABLE_STORAGE to
> capture these three cases (See RELKIND_HAS_STORAGE). This also ensures
> that we return 0 for things that don't qualify as RELKIND_HAS_STORAGE,
> such as a partitioned table (Currently w/ the patch applied, we return
> NULL for those cases, which is another behavior change)


Excellent point. This is the discussion I was longing to have.

I stand by the decision coded in the patch, that pg_table_size() should
return NULL for other kinds of relations, such as indexes, sequences
etc.

It is a conscious decision based on the following:

 * Supported by the documentation, pg_table_size() applies to tables only.
For other uses the higher-level functions pg_total_relation_size() or
pg_relation_size() should be used.
 * Commit fa352d662e taught pg_relation_size() and friends to return NULL if 
the object doesn't exist. This makes perfect sense for the
scenarios described in the commit:

   That avoids errors when the functions are used in queries like
  "SELECT pg_relation_size(oid) FROM pg_class",
   and a table is dropped concurrently.

IMHO: It is more consistent to return NULL when the relation does exist
OR it is not a table kind.
* Returning 0 for things that do not have storage, is nonsensical because
it implies that it 

Re: Commitfest manager 2020-11

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Sunday, October 25, 2020 8:01 PM, Anastasia Lubennikova 
 wrote:

> On 20.10.2020 10:30, gkokola...@pm.me wrote:
>
> > ‐‐‐ Original Message ‐‐‐
> >
> > > [snip]
> > > Wow, that was well in advance) I am willing to assist if you need any 
> > > help.
> >
> > Indeed it was a bit early. I left for vacation after that. For the record, 
> > I am newly active to the community. In our PUG, in Stockholm, we held a 
> > meetup during which a contributor presented ways to contribute to the 
> > community, one of which is becoming CFM. So, I thought of picking up the 
> > recommendation.
> > I have taken little part in CFs as reviewer/author and I have no idea how a 
> > CF is actually run. A contributor from Stockholm has been willing to mentor 
> > me to the part.
> > Since you have both the knowledge and specific ideas on improving the CF, 
> > how about me assisting you? I could shadow you and you can groom me to the 
> > part, so that I can take the lead to a future CF more effectively.
> > This is just a suggestion of course. I am happy with anything that can help 
> > the community as a whole.
>
> Even though, I've worked a lot with community, I have never been CFM
> before as well. So, I think we can just follow these articles:
>
> https://www.2ndquadrant.com/en/blog/managing-a-postgresql-commitfest/
> https://wiki.postgresql.org/wiki/CommitFest_Checklist
>
> Some parts are a bit outdated, but in general the checklist is clear.
> I've already requested CFM privileges in pgsql-www and I'm going to
> spend next week sending pings and updates to the patches at commitfest.

Awesome. I will start with requesting the privileges then.

>
> There are already 219 patches, so I will appreciate if you join me in
> this task.

Count me in.

>
> --
>
> Anastasia Lubennikova
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company

Re: Commitfest manager 2020-11

[gkokolatos]

‐‐‐ Original Message ‐‐‐
> [snip]
>
> Wow, that was well in advance) I am willing to assist if you need any help.
>

Indeed it was a bit early. I left for vacation after that. For the record, I am 
newly active to the community. In our PUG, in Stockholm, we held a meetup 
during which a contributor presented ways to contribute to the community, one 
of which is becoming CFM. So, I thought of picking up the recommendation.

I have taken little part in CFs as reviewer/author and I have no idea how a CF 
is actually run. A contributor from Stockholm has been willing to mentor me to 
the part.

Since you have both the knowledge and specific ideas on improving the CF, how 
about me assisting you? I could shadow you and you can groom me to the part, so 
that I can take the lead to a future CF more effectively.

This is just a suggestion of course. I am happy with anything that can help the 
community as a whole.

Re: PATCH: Attempt to make dbsize a bit more consistent

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Thursday, September 10, 2020 12:51 PM, Daniel Gustafsson  
wrote:

[snip]

> Since we have introduced the table AM api I support going throug it for all
> table accesses, so +1 on the overall idea.
>

Thank you for reviewing! Please find v2 of the patch attached.

In addition to addressing the comments, this patch contains a slightly 
opinionated approach during describe. In short, only relations that have 
storage, are returning non null size during when \d*+ commands are emitted. 
Such an example is a view which can be found in the psql tests. If a view was 
returning a size of 0 Bytes, it would indicate that it can potentially be non 
zero, which is of course wrong.


> Some comments on the patch:
>
> -   -   Note that this also behaves sanely if applied to an index or toast 
> table;
>
> -   -   Note that this also behaves sanely if applied to a toast table;
> -   those won't have attached toast tables, but they can have multiple 
> forks.
> This comment reads a bit odd now and should probably be reworded.
>

Agreed and amended.

>
> -   return size;
>
> -   Assert(size < PG_INT64_MAX);
>
> -
> -   return (int64)size;
> I assume that this change, and the other ones like that, aim to handle 
> int64
> overflow? Using the extra legroom of uint64 can still lead to an overflow,
> however theoretical it may be. Wouldn't it be better to check for overflow
> before adding to size rather than after the fact? Something along the 
> lines of
> checking for headroom left:
>
> rel_size = table_relation_size(..);
> if (rel_size > (PG_INT64_MAX - total_size))
> < error codepath >
>
>
> total_size += rel_size;

Actually not, the intention is not to handle overflow. The 
table_relation_size() returns a uint64 and the calling function returns int64.

The Assert() has been placed in order to denote that it is acknowledged that 
the two functions return different types. I was of the opinion that a run time 
check will not be needed as even the smaller type can cover more than 9200 
PetaByte tables.

If we were to change anything, then I would prefer to change the signature of 
the pg_*_size family of functions to return uint64 instead.


>
> -   if (rel->rd_rel->relkind != RELKIND_INDEX)
>
> -   {
>
> - relation_close(rel, AccessShareLock);
>
>
> - PG_RETURN_NULL();
>
>
> -   }
> pg_indexes_size is defined as returning the size of the indexes attached 
> to the
> specified relation, so this hunk is wrong as it instead requires rel to 
> be an
> index?

You are absolutely correct, amended.

>
> cheers ./daniel
>



v2-0001-Attempt-to-make-dbsize-a-bit-more-consistent.patch
Description: Binary data

Re: PATCH: Attempt to make dbsize a bit more consistent

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Tuesday, 8 September 2020 22:26, David Zhang  wrote:

>
>
> I found the function "table_relation_size" is only used by buffer
> manager for "RELKIND_RELATION", "RELKIND_TOASTVALUE" and
> "RELKIND_MATVIEW", i.e.
>
>         case RELKIND_RELATION:
>         case RELKIND_TOASTVALUE:
>         case RELKIND_MATVIEW:
>             {
>                 /*
>                  * Not every table AM uses BLCKSZ wide fixed size blocks.
>                  * Therefore tableam returns the size in bytes - but
> for the
>                  * purpose of this routine, we want the number of blocks.
>                  * Therefore divide, rounding up.
>                  */
>                 uint64        szbytes;
>
>                 szbytes = table_relation_size(relation, forkNum);
>
>                 return (szbytes + (BLCKSZ - 1)) / BLCKSZ;
>             }
>
> So using "calculate_relation_size" and "calculate_toast_table_size" in
> "calculate_table_size" is easy to understand and the original logic is
> simple.
>

You are correct. This is the logic that is attempted to be applied
in dbsize.c in this patch.

So what do you think of the patch?

Re: PATCH: Attempt to make dbsize a bit more consistent

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Tuesday, 8 September 2020 16:49, John Naylor  
wrote:

> On Thu, Aug 27, 2020 at 9:39 AM gkokola...@pm.me wrote:
>
> > Hi all,
> > this minor patch is attempting to force the use of the tableam api in 
> > dbsize where ever it is required.
> > Apparently something similar was introduced for toast relations only. 
> > Intuitively it seems that the distinction between a table and a toast table 
> > is not needed.
>
> I suspect the reason is found in the comment for table_block_relation_size():
>
> -   If a table AM uses the various relation forks as the sole place where data
> -   is stored, and if it uses them in the expected manner (e.g. the actual 
> data
> -   is in the main fork rather than some other), it can use this 
> implementation
> -   of the relation_size callback rather than implementing its own.


Thank you for your answer and interest at the patch.

I agree with the comment above. However I do not see why it is relevant here. 
When issuing:

SELECT pg_table_size('foo'::regclass);

I should not have to care about the on disk layout of the relation 'foo'.
Without this patch, one will get a correct result only when 'foo' is a heap 
table.
For custom layouts the result can potentially be wrong.



>
> --
> John Naylor https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: Strange behavior with polygon and NaN

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Thursday, 27 August 2020 14:24, Kyotaro Horiguchi  
wrote:

> At Wed, 26 Aug 2020 08:18:49 -0400, Tom Lane t...@sss.pgh.pa.us wrote in
>
> > Kyotaro Horiguchi horikyota@gmail.com writes:
> >
> > > At Tue, 25 Aug 2020 19:03:50 -0400, Bruce Momjian br...@momjian.us wrote 
> > > in
> > >
> > > > I can confirm that this two-month old email report still produces
> > > > different results with indexes on/off in git master, which I don't think
> > > > is ever correct behavior.
> >
> > > I agree to that the behavior is broken.
> >
> > Yeah, but ... what is "non broken" in this case? I'm not convinced
> > that having point_inside() return zero for any case involving NaN
> > is going to lead to noticeably saner behavior than today.
>
> Yes, just doing that leaves many unfixed behavior come from NaNs, but
> at least it seems to me one of sane definition candidates that a point
> cannot be inside a polygon when NaN is involved. It's similar to
> Fpxx() returns false if NaN is involved. As mentioned, I had't fully
> checked and haven't considered this seriously, but I changed my mind
> to check all the callers. I started checking all the callers of
> point_inside, then finally I had to check all functions in geo_ops.c:(
>

For what is worth, I agree with this definition.


> The attached is the result as of now.
>
> === Resulting behavior
>
> With the attached:
>
> -   All boolean functions return false if NaN is involved.
> -   All float8 functions return NaN if NaN is involved.
> -   All geometric arithmetics return NaN as output if NaN is involved.

Agreed! As in both this behaviour conforms to the definition above and the 
patch provides this behaviour with the exceptions below.

>
> With some exceptions:
>
> -   line_eq: needs to consider that NaNs are equal each other.
> -   point_eq/ne (point_eq_pint): ditto
> -   lseg_eq/ne: ditto
>
> The change makes some difference in the regression test.
> For example,
>
>  <->  changed from 0 to NaN. (distance)
>
>
>  <@  changed from true to false. (contained)
>  <->  changed from 0 to NaN. (distance)
>  ?#  changed from true to false (overlaps)
>
> === pg_hypot mistake?
>
> I noticed that pg_hypot returns inf for the parameters (NaN, Inf) but
> I think NaN is appropriate here since other operators behaves that
> way. This change causes a change of distance between point(1e+300,Inf)
> and line (1,-1,0) from infinity to NaN, which I think is correct
> because the arithmetic generates NaN as an intermediate value.
>
> === Infinity annoyances
>
> Infinity makes some not-great changes in regresssion results. For example:
>
> -   point '(1e+300,Infinity)' <-> path '((10,20))' returns
> NaN(previously Infinity), but point '(1e+300,Infinity)' <-> path
> '[(1,2),(3,4)]' returns Infinity. The difference of the two
> expressions is whether (0 * Inf = NaN) is performed or not. The
> former performs it but that was concealed by not propagating NaN to
> upper layer without the patch.

Although I understand the reasoning for this change. I am not certain I agree 
with the result. I feel that:
point '(1e+300,Infinity)' <-> path '((10,20))'
should return Infinity. Even if I am wrong to think that, the two results 
should be expected to behave the same. Am I wrong on that too?


>
> -   Without the patch, point '(1e+300,Infinity)' ## box '(2,2),(0,0)'
> generates '(0,2)', which is utterly wrong. It is because
> box_closept_point evaluates float8_lt(Inf, NaN) as true(!) and sets
> the wrong point for distance=NaN is set. With the patch, the NaN
> makes the result NULL.

Agreed.

>
> -   This is not a difference caused by this patch, but for both patched
> and unpatched, point '(1e+300,Inf)' <-> line '{3,0,0}' returns NaN,
> which should be 1e+300. However, the behavior comes from arithmetic
> reasons and wouldn't be a problem.
>
> create_index.out is changed since point(NaN,NaN) <@ polygon changed
> from true to false, which seems rather saner.
>
> I haven't checked unchanged results but at least changed results seems
> saner to me.

All in all a great patch!

It is clean, well reasoned and carefully crafted.

Do you think that the documentation needs to get updated to the 'new' behaviour?


//Georgios


>
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>

From 432a671517e78061edc87d18aec291f5629fcbe6 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 27 Aug 2020 14:49:21 +0900
Subject: [PATCH v1] Fix NaN handling of some geometric operators and functions

Some geometric operators shows somewhat odd behavior comes from NaN
handling and that leads to inconsistency between index scan and seq
scan on geometric conditions.

For example:
  point '(NaN,NaN)' <-> polygon '((0,0),(0,1),(1,1))' => 0, not NaN
  point '(0.3,0.5)' <-> polygon '((0,0),(0,NaN),(1,1))' => 1.14, not NaN
  point '(NaN,NaN)' <@ polygon 

Re: Reloptions for table access methods

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Tuesday, 1 September 2020 20:21, Jeff Davis  wrote:


>
> I'm fine removing the "validate" parameter completely for the sake of
> consistency.

FWIW, the more I think about this, I would agree with the removal.
However, isn't this mechanism used for other things too, e.g. 
attribute_reloptions,
tablespace_reloptions, that rely on the validation at that layer?. If my 
understanding
is correct, then simply removing the parameter would not cut it and a more 
extensive
refactoring would be needed.

>
> > [snip]
>
> The problem case would be a situation like the following:
>
> 1.  Someone loads an extension that creates a new reloption
> "custom_reloption" of kind RELOPT_KIND_HEAP for their table access
> method "my_tableam".
>
> 2.  They then create a table and forget to specify "USING my_tableam",
> but use the option "custom_reloption=123".
>
> Ideally, that would throw an error because "custom_reloption" is only
> valid for "my_tableam"; but with my patch, no error would be thrown
> because the extension has already added the reloption. It would just
> create a normal heap and "custom_reloption=123" would be ignored.

This is something that I struggle to understand as an "error". In the example,
the set RELOPT_KIND_HEAP was extended for everyone. Regardless of whether
the newly added member will be used or not.

I mean, if the intention was to add reloptions specific to the extension,
shouldn't a new RELOPT_KIND_XXX be introduced? You seem to be thinking
along the same lines. Please, correct me if I understand you wrong.

What I am trying to say, is that with the current patch, I feel the behaviour
is not strange nor unexpected.


> I went with the simple approach because fixing that problem seemed a
> bit over-engineered.

Fixing that problem seems worth it on the long run. I do see the benefit of
the simple approach on the meantime.

//Georgios

>
> Regards,
> Jeff Davis
>

Re: Reloptions for table access methods

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Tuesday, 1 September 2020 09:18, Jeff Davis  wrote:

> A custom table access method might want to add a new reloption to
> control something specific to that table access method. Unfortunately,
> if you add a new option of type RELOPT_KIND_HEAP, it will immediately
> fail because of the validation that happens in fillRelOptions().
>
> Right now, heap reloptions (e.g. FILLFACTOR) are validated in two
> places: parseRelOptions() and fillRelOptions().
>
> parseRelOptions() validates against boolRelOpts[], intRelOpts[], etc.
> This validation is extensible by add_bool_reloption(), etc.
>
> fillRelOptions() validates when filling in a struct to make sure there
> aren't "leftover" options. It does this using a hard-coded parsing
> table that is not extensible.
>
> Index access methods get total control over validation of reloptions,
> but that doesn't fit well with heaps, because all heaps need the
> vacuum-related options.
>
> I considered some other approaches, but they all seemed like over-
> engineering, so the attached patch just passes validate=false to
> fillRelOptions() for heaps. That allows custom table access methods to
> just define new options of kind RELOPT_KIND_HEAP.

I have yet to look at the diff. I simply wanted to give you my wholehearted +1 
to the idea. It is a necessary and an essential part of developing access 
methods.

I have worked extensively in the merge of PG12 into Greenplum, with a focus to 
the tableam api. Handling reloptions has been quite a pain to do cleanly. Given 
the nature of Greenplum's table access methods, we were forced to take it a 
couple of steps further. We did use an approach which I am certain that you 
have considered and discarded as over-engineering for postgres.

In short, I am really excited to see a patch for this topic!

>
> Regards,
> Jeff Davis

Re: New default role- 'pg_read_all_data'

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Monday, 31 August 2020 02:20, Stephen Frost  wrote:

> Greetings,
>
> -   Stephen Frost (sfr...@snowman.net) wrote:
>
> > -   Magnus Hagander (mag...@hagander.net) wrote:
> >
> > > On Fri, Aug 28, 2020 at 2:38 PM Stephen Frost sfr...@snowman.net wrote:
> > >
> > > > -   Magnus Hagander (mag...@hagander.net) wrote:
> > > >
> > > > > Without having actually looked at the code, definite +1 for this 
> > > > > feature.
> > > > > It's much requested...
> > > >
> > > > Thanks.
> > > >
> > > > > But, should we also have a pg_write_all_data to go along with it?
> > > >
> > > > Perhaps, but could certainly be a different patch, and it'd need to be
> > > > better defined, it seems to me... read_all is pretty straight-forward
> > > > (the general goal being "make pg_dumpall/pg_dump work"), what would
> > > > write mean? INSERT? DELETE? TRUNCATE? ALTER TABLE? System catalogs?
> > >
> > > Well, it's pg_write_all_data, so it certainly wouldn't be alter table or
> > > system catalogs.
> > > I'd say insert/update/delete yes.
> > > TRUNCATE is always an outlier.Given it's generally classified as DDL, I
> > > wouldn't include it.
> >
> > Alright, that seems like it'd be pretty easy. We already have a check
> > in pg_class_aclmask to disallow modification of system catalogs w/o
> > being a superuser, so we should be alright to add a similar check for
> > insert/update/delete just below where I added the SELECT check.
> >
> > > > Doesn't seem like you could just declare it to be 'allow pg_restore'
> > > > either, as that might include creating untrusted functions, et al.
> > >
> > > No definitely not. That wouldn't be the usecase at all :)
> >
> > Good. :)
> >
> > > (and fwiw to me the main use case for read_all_data also isn't pg_dump,
> > > because most people using pg_dump are already db owner or higher in my
> > > experience. But it is nice that it helps with that too)
> >
> > Glad to have confirmation that there's other use-cases this helps with.
> > I'll post an updated patch with that added in a day or two.
>
> Here's that updated patch, comments welcome.

Thank you for the updated patch!

Had a quick look on it and nothing stands out.

Also this sub-thread seems to have clearly responded on my early thoughts 
regarding invoking. Adding part of that subthread bellow:


>> My expectation was not met since in my manual test (unless I made a mistake 
>> which is entirely possible), the SELECT above did not fail. The insert did 
>> succeed though.

> That the INSERT worked seems pretty odd- could you post the exact
> changes you've made to the regression tests, or the exact script where
> you aren't seeing what you expect?  I've not been able to reproduce the
> GRANT allowing a user to INSERT into a table.

>>  The first question: Was my expectation wrong?

> If there aren't any other privileges involved, then REVOKE'ing the role
> from a user should prevent that user from being able to SELECT from the
> table.

>> The second question: Is there a privilege that can be granted to 
>> regress_priv_user6 that will not permit the select operation but will permit 
>> the insert operation? If no, should there be one?

As discussed above, while I was struggling to formulate the thought, Magnus had 
already proposed pg_write_all_data and the community had reached a consensus on 
what it actually means.

Please find attached a minimal test case and the output of it.

It is obvious that I was confused and added confusion to the thread. 
Permissions are additive and autonomous. Now it is rather clear to me what the 
expectations are and how the patch should behave.

To add to my embarrassment, the REVOKE operation emitted a warning which I had 
clearly missed.

Apologies.

//Georgios

>
> Thanks!
>
> Stephen



pg_rorole_v2_minimal_test.out
Description: Binary data


pg_rorole_v2_minimal_test.sql
Description: application/sql

Re: v13: show extended stats target in \d

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Monday, 31 August 2020 08:00, Justin Pryzby  wrote:

> The stats target can be set since commit d06215d03, but wasn't shown by psql.
> ALTER STATISISTICS .. SET STATISTICS n.
>
> Normal (1-D) stats targets are shown in \d+ table.
> Stats objects are shown in \d (no plus).
> Arguably, this should be shown only in "verbose" mode (\d+).

This seems rather useful. May I suggest you add it to the commitfest?

//Georgios

Re: New default role- 'pg_read_all_data'

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Friday, 28 August 2020 15:43, Stephen Frost  wrote:

> Greetings,
>
> -   Georgios Kokolatos (gkokola...@protonmail.com) wrote:
>
> > The patch seems to be implementing a useful and requested feature.
> > The patch applies cleanly and passes the basic regress tests. Also the 
> > commitfest bot is happy.
> > A first pass at the code, has not revealed any worthwhile comments.
> > Please allow me for a second and more thorough pass. The commitfest has 
> > hardly started after all.
>
> Great, thanks!
>
> > Also allow me a series of genuine questions:
> > What would the behaviour be with REVOKE?
> > In a sequence similar to:
> > GRANT ALL ON ...
>
> GRANT ALL would be independently GRANT'ing rights to some role and
> therefore unrelated.
>
> > REVOKE pg_read_all_data FROM ...
>
> This would simply REVOKE that role from the user. Privileges
> independently GRANT'd directly to the user wouldn't be affected. Nor
> would other role membership.
>
> > What privileges would the user be left with? Would it be possible to end up 
> > in the same privilege only with a GRANT command?
>
> I'm not sure what's being asked here.

You are correct. My phrasing is not clear. Please be patient and allow me to 
try again.

I was playing around with the code and I was trying a bit the opposite of what 
you have submitted in the test file.

You have, (snipped):

GRANT pg_read_all_data TO regress_priv_user6;

SET SESSION AUTHORIZATION regress_priv_user6;
SELECT * FROM atest1; -- ok
INSERT INTO atest2 VALUES ('foo', true); -- fail


I was expecting:
REVOKE pg_read_all_data FROM regress_priv_user6;

SET SESSION AUTHORIZATION regress_priv_user6;
SELECT * FROM atest1; -- fail
INSERT INTO atest2 VALUES ('foo', true); -- ok


My expectation was not met since in my manual test (unless I made a mistake 
which is entirely possible), the SELECT above did not fail. The insert did 
succeed though.

The first question: Was my expectation wrong?
The second question: Is there a privilege that can be granted to 
regress_priv_user6 that will not permit the select operation but will permit 
the insert operation? If no, should there be one?

I hope I am clearer now.

Thank you again for your patience.

>
> > Does the above scenario even make sense?
>
> I definitely believe it makes sense for a given role/user to be a member
> of pg_read_all_data and to be a member of other roles, or to have other
> privileges GRANT'd directly to them.
>
> Thanks,
>
> Stephen

Re: [PATCH] Explicit null dereferenced (src/backend/access/heap/heaptoast.c)

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Friday, 28 August 2020 03:22, Ranier Vilela  wrote:

> Hi,
>
> Per Coverity.
>
> When "Prepare for toasting", it is necessary to turn off the flag 
> TOAST_NEEDS_DELETE_OLD,
> if there is no need to delete external values from the old tuple, otherwise,
> there are dereference NULL at toast_helper.c (on toast_tuple_cleanup 
> function).
>

Excuse my ignorance, isn't this a false positive?

Regardless right after prepare for toasting, a call to toast_tuple_init is made 
which will explicitly and unconditionally set ttc_flags to zero so the flag bit 
set in the patch will be erased anyways. This patch may make coverity happy but 
does not really change anything in the behaviour of the code.

Furthermore, in the same function, toast_tuple_init, the flag is set to 
TOAST_NEEDS_DELETE_OLD after the old value is actually inspected and found to 
not be null, be stored on disk and to be different than the new value. To my 
understanding, this seems to be correct.

Can you please explain to me what I am missing?

//Georgios

> regards,
> Ranier Vilela