Small issues with CREATE TABLE COMPRESSION
Hi all,
I have been looking at and testing the patch set for CREATE TABLE
COMPRESSION, and spotted a couple of things in parallel of some work
done by Jacob (added in CC).
The behavior around CREATE TABLE AS and matviews is a bit confusing,
and not documented. First, at the grammar level, it is not possible
to specify which compression option is used per column when creating
the relation. So, all the relation columns would just set a column's
compression to be default_toast_compression for all the toastable
columns of the relation. That's not enforceable at column level when
the relation is created, except with a follow-up ALTER TABLE. That's
similar to STORAGE when it comes to matviews, but these are at least
documented.
And so, ALTER MATERIALIZED VIEW supports SET COMPRESSION but this is
not mentioned in its docs:
https://www.postgresql.org/docs/devel/sql-altermaterializedview.html
psql could have tab completion support for that.
There are no tests in pg_dump to make sure that some ALTER
MATERIALIZED VIEW or ALTER TABLE commands are generated when the
compression of a matview's or table's column is changed. This depends
on the value of default_toast_compression, but that would be nice to
have something, and get at least some coverage with
--no-toast-compression. You would need to make the tests conditional
here, for example with check_pg_config() (see for example what's done
with channel binding in ssl/t/002_scram.pl).
Another thing is the handling of the per-value compression that could
be confusing to the user. As no materialization of the data is done
for a CTAS or a matview, and the regression tests of compression.sql
track that AFAIK, there can be a mix of toast values compressed with
lz4 or pglz, with pg_attribute.attcompression being one or the other.
Now, we don't really document any of that, and the per-column
compression value would be set to default_toast_compression while the
stored values may use a mix of the compression methods, depending on
where the toasted values come from. If this behavior is intended, this
makes me wonder in what the possibility to set the compression for a
materialized view column is useful for except for a logical
dump/restore? As of HEAD we'd just insert the toasted value from the
origin as-is so the compression of the column has no effect at all.
Another thing here is the inconsistency that this brings with pg_dump.
For example, as the dumped values are decompressed, we could have
values compressed with pglz at the origin, with a column using lz4
within its definition that would make everything compressed with lz4
once the values are restored. This choice may be fine, but I think
that it would be good to document all that. That would be less
surprising to the user.
Similarly, we may want to document that COMPRESSION does not trigger a
table rewrite, but that it is effective only for the new toast values
inserted if a tuple is rebuilt and rewritten?
Would it be better to document that pg_column_compression() returns
NULL if the column is not a toastable type or if the column's value is
not compressed?
The flexibility with allow_system_table_mods allows one to change the
compression method of catalogs, for example switching rolpassword with
a SCRAM verifier large enough to be toasted would lock an access to
the cluster if restarting the server without lz4 support. I shouldn't
have done that but I did, and I like it :)
The design used by this feature is pretty cool, as long as you don't
read the compressed values, physical replication can work out of the
box even across nodes that are built with or without lz4.
Thanks,
--
Michael
On Tue, Apr 27, 2021 at 03:22:25PM +0900, Michael Paquier wrote:
Hi all,
I have been looking at and testing the patch set for CREATE TABLE
COMPRESSION, and spotted a couple of things in parallel of some work
done by Jacob (added in CC).The behavior around CREATE TABLE AS and matviews is a bit confusing,
and not documented. First, at the grammar level, it is not possible
to specify which compression option is used per column when creating
the relation. So, all the relation columns would just set a column's
compression to be default_toast_compression for all the toastable
columns of the relation. That's not enforceable at column level when
the relation is created, except with a follow-up ALTER TABLE. That's
similar to STORAGE when it comes to matviews, but these are at least
documented.And so, ALTER MATERIALIZED VIEW supports SET COMPRESSION but this is
not mentioned in its docs:
https://www.postgresql.org/docs/devel/sql-altermaterializedview.htmlpsql could have tab completion support for that.
Actually ALTER matview ALTER col has no tab completion at all, right ?
Now, we don't really document any of that, and the per-column
compression value would be set to default_toast_compression while the
stored values may use a mix of the compression methods, depending on
where the toasted values come from. If this behavior is intended, this
makes me wonder in what the possibility to set the compression for a
materialized view column is useful for except for a logical
dump/restore? As of HEAD we'd just insert the toasted value from the
origin as-is so the compression of the column has no effect at all.
That may be true if the mat view is trivial, but not true if it has
expressions. The mat view column may be built on multiple table columns, or be
of a different type than the columns it's built on top of, so the relationship
may not be so direct.
Another thing here is the inconsistency that this brings with pg_dump.
For example, as the dumped values are decompressed, we could have
values compressed with pglz at the origin, with a column using lz4
within its definition that would make everything compressed with lz4
once the values are restored. This choice may be fine, but I think
that it would be good to document all that. That would be less
surprising to the user.
Can you suggest what or where we'd say it? I think this is just the behavior
that pg_dump shouldn't lose the user's compression setting.
The setting itself is for "future" data, and the only way to guarantee what
compression types are in use are by vacuum full/cluster or pg_dump restore.
Similarly, we may want to document that COMPRESSION does not trigger a
table rewrite, but that it is effective only for the new toast values
inserted if a tuple is rebuilt and rewritten?
Good point. I started with this.
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 39927be41e..8cceea41d0 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -391,7 +391,21 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
</term>
<listitem>
<para>
- This sets the compression method for a column. The supported compression
+ This sets the compression method to be used for data inserted into a column.
+
+ This does not cause the table to be rewritten, so existing data may still
+ be compressed with other compression methods. If the table is rewritten with
+ <command>VACUUM FULL</command> or <command>CLUSTER</command>, or restored
+ with <application>pg_restore</application>, then all tuples are rewritten
+ with the configured compression methods.
+
+ Also, note that when data is inserted from another relation (for example,
+ by <command>INSERT ... SELECT</command>), tuples from the source data are
+ not necessarily detoasted, and any previously compressed data is retained
+ with its existing compression method, rather than recompressing with the
+ compression methods of the target columns.
+
+ The supported compression
methods are <literal>pglz</literal> and <literal>lz4</literal>.
<literal>lz4</literal> is available only if <literal>--with-lz4</literal>
was used when building <productname>PostgreSQL</productname>.
Hi,
My analysis of this open item is there are no code-level issues here,
but there is one line of documentation that clearly got forgotten,
some other documentation changes that might be nice, and maybe someone
wants to work more on testing and/or tab completion at some point.
On Tue, Apr 27, 2021 at 2:22 AM Michael Paquier <michael@paquier.xyz> wrote:
The behavior around CREATE TABLE AS and matviews is a bit confusing,
and not documented.
It's no different from a bunch of other column properties that you
also can't set when creating a materialized view. I don't think this
patch created this problem, or that it is responsible for solving it.
And so, ALTER MATERIALIZED VIEW supports SET COMPRESSION but this is
not mentioned in its docs:
I agree that's an oversight and should be fixed.
https://www.postgresql.org/docs/devel/sql-altermaterializedview.html
psql could have tab completion support for that.
I don't believe it's our policy that incomplete tab completion support
rises to the level of an open item, especially given that, as Justin
points out, it's not supported for ALTER MATERIALIZED VIEW name ALTER
COLUMN name <tab> doesn't complete anything *at all*.
There are no tests in pg_dump to make sure that some ALTER
MATERIALIZED VIEW or ALTER TABLE commands are generated when the
compression of a matview's or table's column is changed.
True, but it does seem to work. I am happy if you or anyone want to
write some tests.
Another thing is the handling of the per-value compression that could
be confusing to the user. As no materialization of the data is done
for a CTAS or a matview, and the regression tests of compression.sql
track that AFAIK, there can be a mix of toast values compressed with
lz4 or pglz, with pg_attribute.attcompression being one or the other.
Yes. This is mentioned in the commit message, and was discussed
extensively on the original thread. We probably should have included
it in the documentation, as well. Justin's text seems fairly
reasonable to me.
Similarly, we may want to document that COMPRESSION does not trigger a
table rewrite, but that it is effective only for the new toast values
inserted if a tuple is rebuilt and rewritten?
Sure. I think Justin's text covers this, too.
Would it be better to document that pg_column_compression() returns
NULL if the column is not a toastable type or if the column's value is
not compressed?
We can.
Here's a proposed patch for the documentation issues not covered by
Justin's proposal.
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
compression-doc-fixes-v1.patchapplication/octet-stream; name=compression-doc-fixes-v1.patchDownload
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7a8aefc513b..35bd43c704d 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -26185,7 +26185,8 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
</para>
<para>
Shows the compression algorithm that was used to compress a
- an individual variable-length value.
+ an individual variable-length value. Returns <literal>NULL</literal>
+ if the value is not compressed.
</para></entry>
</row>
diff --git a/doc/src/sgml/ref/alter_materialized_view.sgml b/doc/src/sgml/ref/alter_materialized_view.sgml
index 27f60f6df59..7011a0e7da0 100644
--- a/doc/src/sgml/ref/alter_materialized_view.sgml
+++ b/doc/src/sgml/ref/alter_materialized_view.sgml
@@ -40,6 +40,7 @@ ALTER MATERIALIZED VIEW ALL IN TABLESPACE <replaceable class="parameter">name</r
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET ( <replaceable class="parameter">attribute_option</replaceable> = <replaceable class="parameter">value</replaceable> [, ... ] )
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> RESET ( <replaceable class="parameter">attribute_option</replaceable> [, ... ] )
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
+ ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET COMPRESSION <replaceable class="parameter">compression_method</replaceable>
CLUSTER ON <replaceable class="parameter">index_name</replaceable>
SET WITHOUT CLUSTER
SET ( <replaceable class="parameter">storage_parameter</replaceable> [= <replaceable class="parameter">value</replaceable>] [, ... ] )
On Thu, Apr 29, 2021 at 9:31 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
On Tue, Apr 27, 2021 at 03:22:25PM +0900, Michael Paquier wrote:
Hi all,
And so, ALTER MATERIALIZED VIEW supports SET COMPRESSION but this is
not mentioned in its docs:
https://www.postgresql.org/docs/devel/sql-altermaterializedview.htmlpsql could have tab completion support for that.
Actually ALTER matview ALTER col has no tab completion at all, right ?
Right.
Good point. I started with this.
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 39927be41e..8cceea41d0 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -391,7 +391,21 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM </term> <listitem> <para> - This sets the compression method for a column. The supported compression + This sets the compression method to be used for data inserted into a column. + + This does not cause the table to be rewritten, so existing data may still + be compressed with other compression methods. If the table is rewritten with + <command>VACUUM FULL</command> or <command>CLUSTER</command>, or restored + with <application>pg_restore</application>, then all tuples are rewritten + with the configured compression methods. + + Also, note that when data is inserted from another relation (for example, + by <command>INSERT ... SELECT</command>), tuples from the source data are + not necessarily detoasted, and any previously compressed data is retained + with its existing compression method, rather than recompressing with the + compression methods of the target columns. + + The supported compression methods are <literal>pglz</literal> and <literal>lz4</literal>. <literal>lz4</literal> is available only if <literal>--with-lz4</literal> was used when building <productname>PostgreSQL</productname>.
Your documentation looks fine to me.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Wed, May 5, 2021 at 12:06 AM Robert Haas <robertmhaas@gmail.com> wrote:
There are no tests in pg_dump to make sure that some ALTER
MATERIALIZED VIEW or ALTER TABLE commands are generated when the
compression of a matview's or table's column is changed.True, but it does seem to work. I am happy if you or anyone want to
write some tests.
I think it will be really hard to generate such a test in pg_dump,
because default we are compiling --without-lz4, which means we have
only one compression option available, and if there is only one option
available then the column compression method and the default
compression method will be same so the dump will not generate an extra
command of type ALTER TABLE... SET COMPRESSION.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Wed, May 5, 2021 at 11:02 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Wed, May 5, 2021 at 12:06 AM Robert Haas <robertmhaas@gmail.com> wrote:
There are no tests in pg_dump to make sure that some ALTER
MATERIALIZED VIEW or ALTER TABLE commands are generated when the
compression of a matview's or table's column is changed.True, but it does seem to work. I am happy if you or anyone want to
write some tests.I think it will be really hard to generate such a test in pg_dump,
because default we are compiling --without-lz4, which means we have
only one compression option available, and if there is only one option
available then the column compression method and the default
compression method will be same so the dump will not generate an extra
command of type ALTER TABLE... SET COMPRESSION.
I think we already have such test cases at least through pg_upgrade.
Basically, if you see in compression.sql we are not dropping the table
so that pg_upgrade and dump them and test. So if test run --with-lz4
then in pg_upgrade dump we can see ALTER TABLE... SET COMPRESSION type
of commands.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Wed, May 05, 2021 at 01:41:03PM +0530, Dilip Kumar wrote:
I think we already have such test cases at least through pg_upgrade.
Basically, if you see in compression.sql we are not dropping the table
so that pg_upgrade and dump them and test. So if test run --with-lz4
then in pg_upgrade dump we can see ALTER TABLE... SET COMPRESSION type
of commands.
The TAP tests of pg_dump are much more picky than what pg_upgrade is
able to do. With the existing set of tests in place, what you are
able to detect is that pg_upgrade does not *break* if there are tables
with attributes using various compression types, but that would not be
enough to make sure that the correct compression method is *applied*
depending on the context expected (default_toast_compression + the
attribute compression + pg_dump options), which is what the TAP tests
of pg_dump are able to correctly detect if extended in an appropriate
way.
With what's on HEAD, we would easily miss any bugs introduced in
pg_dump that change the set of commands generated depending on the
options given by a user, but still allow pg_upgrade to work correctly.
For example, there could be issues where we finish by setting up the
incorrect compression option, with pg_upgrade happily finishing.
There is a gap in the test coverage here.
--
Michael
On Wed, May 5, 2021 at 4:00 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, May 05, 2021 at 01:41:03PM +0530, Dilip Kumar wrote:
I think we already have such test cases at least through pg_upgrade.
Basically, if you see in compression.sql we are not dropping the table
so that pg_upgrade and dump them and test. So if test run --with-lz4
then in pg_upgrade dump we can see ALTER TABLE... SET COMPRESSION type
of commands.The TAP tests of pg_dump are much more picky than what pg_upgrade is
able to do. With the existing set of tests in place, what you are
able to detect is that pg_upgrade does not *break* if there are tables
with attributes using various compression types, but that would not be
enough to make sure that the correct compression method is *applied*
depending on the context expected (default_toast_compression + the
attribute compression + pg_dump options), which is what the TAP tests
of pg_dump are able to correctly detect if extended in an appropriate
way.
Okay, got your point.
With what's on HEAD, we would easily miss any bugs introduced in
pg_dump that change the set of commands generated depending on the
options given by a user, but still allow pg_upgrade to work correctly.
For example, there could be issues where we finish by setting up the
incorrect compression option, with pg_upgrade happily finishing.
There is a gap in the test coverage here.
Basically, the problem is default compilation is --without-lz4 so by
default there is only one compression method and with only one
compression method we can not generate the test case you asked for,
because that will be the default compression method and we don't dump
the default compression method.
So basically, if we have to write this test case in pg_dump then we
will have to use lz4 which means it will generate different output
--with-lz4 vs --without-lz4. With a simple regress test it easy to
deal with such cases by keeping multiple .out files but I am not sure
can we do this easily with pg_dump test without adding much
complexity?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Wed, May 5, 2021 at 7:09 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
So basically, if we have to write this test case in pg_dump then we
will have to use lz4 which means it will generate different output
--with-lz4 vs --without-lz4. With a simple regress test it easy to
deal with such cases by keeping multiple .out files but I am not sure
can we do this easily with pg_dump test without adding much
complexity?
TAP tests have a facility for conditionally skipping tests; see
perldoc Test::More. That's actually superior to what you can do with
pg_regress. We'd need to come up with some logic to determine when to
skip or not, though. Perhaps the easiest method would be to have the
relevant Perl script try to create a table with an lz4 column. If that
works, then perform the LZ4-based tests. If it fails, check the error
message. If it says anything that LZ4 is not supported by this build,
skip those tests. If it says anything else, die.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Wed, May 05, 2021 at 09:59:41AM -0400, Robert Haas wrote:
TAP tests have a facility for conditionally skipping tests; see
perldoc Test::More. That's actually superior to what you can do with
pg_regress. We'd need to come up with some logic to determine when to
skip or not, though. Perhaps the easiest method would be to have the
relevant Perl script try to create a table with an lz4 column. If that
works, then perform the LZ4-based tests. If it fails, check the error
message. If it says anything that LZ4 is not supported by this build,
skip those tests. If it says anything else, die.
There is a simpler and cheaper method to make the execution of TAP
test conditional. As in src/test/ssl/t/002_scram.pl for channel
binding, I think that you could use something like
check_pg_config("#define HAVE_LIBLZ4 1") and use its result to decide
which tests to skip or not.
--
Michael
On Thu, May 6, 2021 at 5:35 AM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, May 05, 2021 at 09:59:41AM -0400, Robert Haas wrote:
TAP tests have a facility for conditionally skipping tests; see
perldoc Test::More. That's actually superior to what you can do with
pg_regress. We'd need to come up with some logic to determine when to
skip or not, though. Perhaps the easiest method would be to have the
relevant Perl script try to create a table with an lz4 column. If that
works, then perform the LZ4-based tests. If it fails, check the error
message. If it says anything that LZ4 is not supported by this build,
skip those tests. If it says anything else, die.There is a simpler and cheaper method to make the execution of TAP
test conditional. As in src/test/ssl/t/002_scram.pl for channel
binding, I think that you could use something like
check_pg_config("#define HAVE_LIBLZ4 1") and use its result to decide
which tests to skip or not.
Thanks, Robert and Michael for your input. I will try to understand
how it is done in the example shared by you and come up with the test
once I get time. I assume this is not something urgent.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Thu, May 6, 2021 at 10:45 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
I noticed that the error code for invalid compression method is not
perfect, basically when we pass the invalid compression method during
CREATE/ALTER table that time we give
ERRCODE_FEATURE_NOT_SUPPORTED. I think the correct error code is
ERRCODE_INVALID_PARAMETER_VALUE. I have attached a patch to fix this.
I thought of starting a new thread first but then I thought the
subject of this thread is quite generic and this is a fairly small fix
so we can use the same thread.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Attachments:
0001-Fix-thinko-in-invalid-compression-error-code.patchtext/x-patch; charset=US-ASCII; name=0001-Fix-thinko-in-invalid-compression-error-code.patchDownload
From f09e9940772c6d493624268b776931633c882c7d Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilipkumar@localhost.localdomain>
Date: Thu, 6 May 2021 15:13:06 +0530
Subject: [PATCH] Fix thinko in invalid compression error code
---
src/backend/commands/tablecmds.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d9ba87a..7351b78 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -18491,7 +18491,7 @@ GetAttributeCompression(Form_pg_attribute att, char *compression)
cmethod = CompressionNameToMethod(compression);
if (!CompressionMethodIsValid(cmethod))
ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid compression method \"%s\"", compression)));
return cmethod;
--
1.8.3.1
On Thu, May 06, 2021 at 05:01:23PM +0530, Dilip Kumar wrote:
I noticed that the error code for invalid compression method is not
perfect, basically when we pass the invalid compression method during
CREATE/ALTER table that time we give
ERRCODE_FEATURE_NOT_SUPPORTED. I think the correct error code is
ERRCODE_INVALID_PARAMETER_VALUE. I have attached a patch to fix this.
Yeah, I agree that this is an improvement, so let's fix this.
--
Michael
On Thu, May 06, 2021 at 10:45:53AM +0530, Dilip Kumar wrote:
Thanks, Robert and Michael for your input. I will try to understand
how it is done in the example shared by you and come up with the test
once I get time. I assume this is not something urgent.
Thanks. FWIW, I'd rather see this gap closed asap, as features should
come with proper tests IMO.
While on it, I can see that there is no support for --with-lz4 in the
MSVC scripts. I think that this is something where we had better
close the gap, and upstream provides binaries on Windows on their
release page:
https://github.com/lz4/lz4/releases
And I am familiar with both areas, so I have no helping out and
getting that in shape correctly before beta1.
--
Michael
On Thu, May 6, 2021 at 5:42 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, May 06, 2021 at 10:45:53AM +0530, Dilip Kumar wrote:
Thanks, Robert and Michael for your input. I will try to understand
how it is done in the example shared by you and come up with the test
once I get time. I assume this is not something urgent.Thanks. FWIW, I'd rather see this gap closed asap, as features should
come with proper tests IMO.
I have done this please find the attached patch.
While on it, I can see that there is no support for --with-lz4 in the
MSVC scripts. I think that this is something where we had better
close the gap, and upstream provides binaries on Windows on their
release page:
https://github.com/lz4/lz4/releasesAnd I am familiar with both areas, so I have no helping out and
getting that in shape correctly before beta1.
I don't have much idea about the MSVC script, but I will try to see
some other parameters and fix this.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v1-0001-Add-pg_dump-test-case-for-TOAST-compression.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Add-pg_dump-test-case-for-TOAST-compression.patchDownload
From 104fc883f5e4ca4e268c29350de0cf173065cb68 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilipkumar@localhost.localdomain>
Date: Thu, 6 May 2021 21:32:12 +0530
Subject: [PATCH v1] Add pg_dump test case for TOAST compression
---
src/bin/pg_dump/t/002_pg_dump.pl | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 86113df..197d677 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -2334,6 +2334,26 @@ my %tests = (
unlike => { exclude_dump_test_schema => 1, },
},
+ 'CREATE TABLE test_compression' => {
+ create_order => 3,
+ create_sql => 'CREATE TABLE dump_test.test_compression (
+ col1 int,
+ col2 text COMPRESSION lz4
+ );',
+ regexp => qr/^
+ \QCREATE TABLE dump_test.test_compression (\E\n
+ \s+\Qcol1 integer,\E\n
+ \s+\Qcol2 text\E\n
+ \);\n
+ .*
+ \QALTER TABLE ONLY dump_test.test_compression ALTER COLUMN col2 SET COMPRESSION lz4;\E\n
+ /xms,
+ lz4 => 1,
+ like =>
+ { %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
+ unlike => { exclude_dump_test_schema => 1, },
+ },
+
'CREATE TABLE measurement PARTITIONED BY' => {
create_order => 90,
create_sql => 'CREATE TABLE dump_test.measurement (
@@ -3377,6 +3397,7 @@ my $port = $node->port;
# If it doesn't then we will skip all the COLLATION-related tests.
my $collation_support = 0;
my $collation_check_stderr;
+my $supports_lz4 = check_pg_config("#define HAVE_LIBLZ4 1");
$node->psql(
'postgres',
"CREATE COLLATION testing FROM \"C\"; DROP COLLATION testing;",
@@ -3445,6 +3466,12 @@ foreach my $run (sort keys %pgdump_runs)
next;
}
+ # Skip any lz4 compression related test
+ if (!$supports_lz4 && defined($tests{$test}->{lz4}))
+ {
+ next;
+ }
+
# If there is a like entry, but no unlike entry, then we will test the like case
if ($tests{$test}->{like}->{$test_key}
&& !defined($tests{$test}->{unlike}->{$test_key}))
@@ -3502,6 +3529,12 @@ foreach my $test (
next;
}
+ # Skip any lz4 compression related test
+ if (!$supports_lz4 && defined($tests{$test}->{lz4}))
+ {
+ next;
+ }
+
# Add terminating semicolon
$create_sql{$test_db} .= $tests{$test}->{create_sql} . ";";
}
@@ -3600,6 +3633,12 @@ foreach my $run (sort keys %pgdump_runs)
next;
}
+ # Skip any lz4 compression related test
+ if (!$supports_lz4 && defined($tests{$test}->{lz4}))
+ {
+ next;
+ }
+
if ($run_db ne $test_db)
{
next;
--
1.8.3.1
On Thu, May 06, 2021 at 09:04:57PM +0900, Michael Paquier wrote:
Yeah, I agree that this is an improvement, so let's fix this.
Just noticed that this was not applied yet, so done while I was
looking at this thread again.
--
Michael
On Sat, May 8, 2021 at 7:04 AM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, May 06, 2021 at 09:04:57PM +0900, Michael Paquier wrote:
Yeah, I agree that this is an improvement, so let's fix this.
Just noticed that this was not applied yet, so done while I was
looking at this thread again.
Thanks!
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Thu, May 06, 2021 at 09:33:53PM +0530, Dilip Kumar wrote:
On Thu, May 6, 2021 at 5:42 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, May 06, 2021 at 10:45:53AM +0530, Dilip Kumar wrote:
Thanks, Robert and Michael for your input. I will try to understand
how it is done in the example shared by you and come up with the test
once I get time. I assume this is not something urgent.Thanks. FWIW, I'd rather see this gap closed asap, as features should
come with proper tests IMO.I have done this please find the attached patch.
No objections to take the approach to mark the lz4-related test with a
special flag and skip them. I have three comments:
- It would be good to document this new flag. See the comment block
on top of %dump_test_schema_runs.
- There should be a test for --no-toast-compression. You can add a
new command in %pgdump_runs, then unmatch the expected output with the
option.
- I would add one test case with COMPRESSION pglz somewhere to check
after the case of ALTER TABLE COMPRESSION commands not generated as
this depends on default_toast_compression. A second test I'd add is a
materialized view with a column switched to use lz4 as compression
method with an extra ALTER command in create_sql.
I don't have much idea about the MSVC script, but I will try to see
some other parameters and fix this.
Thanks! I can dive into that if that's an issue. Let's make things
compatible with what upstream provides, meaning that we should have
some documentation pointing to the location of their deliverables,
equally to what we do for the Perl and OpenSSL dependencies for
example.
--
Michael
On Sat, May 08, 2021 at 05:37:58PM +0900, Michael Paquier wrote:
Thanks! I can dive into that if that's an issue. Let's make things
compatible with what upstream provides, meaning that we should have
some documentation pointing to the location of their deliverables,
equally to what we do for the Perl and OpenSSL dependencies for
example.
Dilip has sent me a patch set without adding pgsql-hackers in CC (I
guess these will be available soon). Anyway, this patch included a
change to fix a hole in the installation docs, where --with-lz4 is not
listed yet. I have reviewed that stuff and found more
inconsistencies in the docs, leading me to the attached:
- The upstream project name is "LZ4", so we had better use the correct
name when not referring to the option value used in CREATE/ALTER
TABLE.
- doc/src/sgml/installation.sgml misses a description for --with-lz4.
Without the Windows changes, I am finishing with the attached to close
the loop with the docs.
Thanks,
--
Michael
Attachments:
fix-lz4-docs.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 29ee9605b6..6d06ad22b9 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1363,7 +1363,7 @@
The current compression method of the column. If it is an invalid
compression method (<literal>'\0'</literal>) then column data will not
be compressed. Otherwise, <literal>'p'</literal> = pglz compression or
- <literal>'l'</literal> = lz4 compression.
+ <literal>'l'</literal> = <productname>LZ4</productname> compression.
</para></entry>
</row>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 5efbfe97b5..5c18deddb6 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8321,9 +8321,9 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
<command>CREATE TABLE</command> statement can override this default
by specifying the <literal>COMPRESSION</literal> column option.
- The supported compression methods are <literal>pglz</literal> and
- (if configured at the time <productname>PostgreSQL</productname> was
- built) <literal>lz4</literal>.
+ The supported compression methods are <literal>pglz</literal> and,
+ if <productname>PostgreSQL</productname> was compiled with
+ <literal>--with-lz4</literal>, <literal>lz4</literal>.
The default is <literal>pglz</literal>.
</para>
</listitem>
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index 50d9fa2021..5af48275e4 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -266,6 +266,14 @@ su - postgres
</para>
</listitem>
+ <listitem>
+ <para>
+ You need <productname>LZ4</productname>, if you want to support
+ the compression of data with this method; see
+ <xref linkend="sql-createtable"/>.
+ </para>
+ </listitem>
+
<listitem>
<para>
To build the <productname>PostgreSQL</productname> documentation,
@@ -966,6 +974,17 @@ build-postgresql:
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--with-lz4</option></term>
+ <listitem>
+ <para>
+ Build with <productname>LZ4</productname> compression support.
+ This allows the use of <productname>LZ4</productname> for the
+ compression of table data.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>--with-ssl=<replaceable>LIBRARY</replaceable></option>
<indexterm>
On Sat, May 08, 2021 at 10:13:09PM +0900, Michael Paquier wrote:
+ You need <productname>LZ4</productname>, if you want to support + the compression of data with this method; see + <xref linkend="sql-createtable"/>.
I suggest to change "the compression" to "compression".
I would write the whole thing like:
| The LZ4 library is needed to support compression of data using that method...
+ Build with <productname>LZ4</productname> compression support. + This allows the use of <productname>LZ4</productname> for the + compression of table data.
remove "the"
--
Justin
| You need LZ4, if you want to support the compression of data with this method; see CREATE TABLE.
I suggest that should reference guc-default-toast-compression instead of CREATE
TABLE, since CREATE TABLE is large and very non-specific.
Also, in at least 3 places there's extraneous trailing whitespace.
Two of these should (I think) be a blank line.
+ <xref linkend="sql-createtable"/>.$
+ </para>$
+ </listitem>$
+ $
<listitem>$+ compression of table data. $
+ </para>$
+ </listitem>$
+ </varlistentry>$
+ $On Sat, May 8, 2021 at 6:43 PM Michael Paquier <michael@paquier.xyz> wrote:
On Sat, May 08, 2021 at 05:37:58PM +0900, Michael Paquier wrote:
Thanks! I can dive into that if that's an issue. Let's make things
compatible with what upstream provides, meaning that we should have
some documentation pointing to the location of their deliverables,
equally to what we do for the Perl and OpenSSL dependencies for
example.Dilip has sent me a patch set without adding pgsql-hackers in CC (I
guess these will be available soon).
My bad.
Anyway, this patch included a
change to fix a hole in the installation docs, where --with-lz4 is not
listed yet. I have reviewed that stuff and found more
inconsistencies in the docs, leading me to the attached:
- The upstream project name is "LZ4", so we had better use the correct
name when not referring to the option value used in CREATE/ALTER
TABLE.
- doc/src/sgml/installation.sgml misses a description for --with-lz4.Without the Windows changes, I am finishing with the attached to close
the loop with the docs.
Thanks for the changes. I will send the other patches soon, after
removing the doc part which you have already included here.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Sat, May 8, 2021 at 2:08 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, May 06, 2021 at 09:33:53PM +0530, Dilip Kumar wrote:
On Thu, May 6, 2021 at 5:42 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, May 06, 2021 at 10:45:53AM +0530, Dilip Kumar wrote:
Thanks, Robert and Michael for your input. I will try to understand
how it is done in the example shared by you and come up with the test
once I get time. I assume this is not something urgent.Thanks. FWIW, I'd rather see this gap closed asap, as features should
come with proper tests IMO.I have done this please find the attached patch.
No objections to take the approach to mark the lz4-related test with a
special flag and skip them. I have three comments:
- It would be good to document this new flag. See the comment block
on top of %dump_test_schema_runs.
- There should be a test for --no-toast-compression. You can add a
new command in %pgdump_runs, then unmatch the expected output with the
option.
- I would add one test case with COMPRESSION pglz somewhere to check
after the case of ALTER TABLE COMPRESSION commands not generated as
this depends on default_toast_compression. A second test I'd add is a
materialized view with a column switched to use lz4 as compression
method with an extra ALTER command in create_sql.
I have fixed some of them, I could not write the test cases where we
have to ensure that 'ALTER TABLE COMPRESSION' is not appearing in the
dump. Basically, I don't have knowledge of the perl language so even
after trying for some time I could not write those 2 test cases. I
have fixed the remaining comments.
I don't have much idea about the MSVC script, but I will try to see
some other parameters and fix this.Thanks! I can dive into that if that's an issue. Let's make things
compatible with what upstream provides, meaning that we should have
some documentation pointing to the location of their deliverables,
equally to what we do for the Perl and OpenSSL dependencies for
example.
I have changed the documentation and also updated the Solution.pm. I
could not verify the windows build yet as I am not having windows
environment.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v2-0001-Add-pg_dump-test-case-for-TOAST-compression.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Add-pg_dump-test-case-for-TOAST-compression.patchDownload
From c69ab6dd0fde8e39b7d5aeeb4d74119621c31f58 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilipkumar@localhost.localdomain>
Date: Thu, 6 May 2021 21:32:12 +0530
Subject: [PATCH v2 1/2] Add pg_dump test case for TOAST compression
---
src/bin/pg_dump/t/002_pg_dump.pl | 64 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 86113df..3ea6f10 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -374,6 +374,10 @@ my %pgdump_runs = (
# of the pg_dump runs happening. This is what "seeds" the
# system with objects to be dumped out.
#
+# There is also a flag 'lz4' which can be set if the test case
+# depends upon lz4 library. And if this flag is set then the
+# test will not be executed if HAVE_LIBLZ4 is not set.
+#
# Building of this hash takes a bit of time as all of the regexps
# included in it are compiled. This greatly improves performance
# as the regexps are used for each run the test applies to.
@@ -2068,6 +2072,27 @@ my %tests = (
unlike => { exclude_dump_test_schema => 1, },
},
+ 'CREATE MATERIALIZED VIEW matview_compression' => {
+ create_order => 20,
+ create_sql => 'CREATE MATERIALIZED VIEW
+ dump_test.matview_compression (col2) AS
+ SELECT col2 FROM dump_test.test_table;
+ ALTER MATERIALIZED VIEW dump_test.matview_compression
+ ALTER COLUMN col2 SET COMPRESSION lz4',
+ regexp => qr/^
+ \QCREATE MATERIALIZED VIEW dump_test.matview_compression AS\E
+ \n\s+\QSELECT test_table.col2\E
+ \n\s+\QFROM dump_test.test_table\E
+ \n\s+\QWITH NO DATA;\E
+ .*
+ \QALTER TABLE ONLY dump_test.matview_compression ALTER COLUMN col2 SET COMPRESSION lz4;\E\n
+ /xms,
+ lz4 => 1,
+ like =>
+ { %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
+ unlike => { exclude_dump_test_schema => 1, },
+ },
+
'CREATE POLICY p1 ON test_table' => {
create_order => 22,
create_sql => 'CREATE POLICY p1 ON dump_test.test_table
@@ -2334,6 +2359,26 @@ my %tests = (
unlike => { exclude_dump_test_schema => 1, },
},
+ 'CREATE TABLE test_compression' => {
+ create_order => 3,
+ create_sql => 'CREATE TABLE dump_test.test_compression (
+ col1 int,
+ col2 text COMPRESSION lz4
+ );',
+ regexp => qr/^
+ \QCREATE TABLE dump_test.test_compression (\E\n
+ \s+\Qcol1 integer,\E\n
+ \s+\Qcol2 text\E\n
+ \);\n
+ .*
+ \QALTER TABLE ONLY dump_test.test_compression ALTER COLUMN col2 SET COMPRESSION lz4;\E\n
+ /xms,
+ lz4 => 1,
+ like =>
+ { %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
+ unlike => { exclude_dump_test_schema => 1, },
+ },
+
'CREATE TABLE measurement PARTITIONED BY' => {
create_order => 90,
create_sql => 'CREATE TABLE dump_test.measurement (
@@ -3377,6 +3422,7 @@ my $port = $node->port;
# If it doesn't then we will skip all the COLLATION-related tests.
my $collation_support = 0;
my $collation_check_stderr;
+my $supports_lz4 = check_pg_config("#define HAVE_LIBLZ4 1");
$node->psql(
'postgres',
"CREATE COLLATION testing FROM \"C\"; DROP COLLATION testing;",
@@ -3445,6 +3491,12 @@ foreach my $run (sort keys %pgdump_runs)
next;
}
+ # Skip any lz4 compression related test
+ if (!$supports_lz4 && defined($tests{$test}->{lz4}))
+ {
+ next;
+ }
+
# If there is a like entry, but no unlike entry, then we will test the like case
if ($tests{$test}->{like}->{$test_key}
&& !defined($tests{$test}->{unlike}->{$test_key}))
@@ -3502,6 +3554,12 @@ foreach my $test (
next;
}
+ # Skip any lz4 compression related test
+ if (!$supports_lz4 && defined($tests{$test}->{lz4}))
+ {
+ next;
+ }
+
# Add terminating semicolon
$create_sql{$test_db} .= $tests{$test}->{create_sql} . ";";
}
@@ -3600,6 +3658,12 @@ foreach my $run (sort keys %pgdump_runs)
next;
}
+ # Skip any lz4 compression related test
+ if (!$supports_lz4 && defined($tests{$test}->{lz4}))
+ {
+ next;
+ }
+
if ($run_db ne $test_db)
{
next;
--
1.8.3.1
v2-0002-LZ4-option-for-windows-build-and-documentation-up.patchtext/x-patch; charset=US-ASCII; name=v2-0002-LZ4-option-for-windows-build-and-documentation-up.patchDownload
From 7c71f46cfe09bcf0e1c6c4e6f093b0178d60f5cd Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilipkumar@localhost.localdomain>
Date: Sat, 8 May 2021 20:12:56 +0530
Subject: [PATCH v2 2/2] LZ4 option for windows build and documentation update
---
doc/src/sgml/install-windows.sgml | 9 +++++++++
src/tools/msvc/Solution.pm | 10 ++++++++++
2 files changed, 19 insertions(+)
diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index 64687b1..bad8431 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -339,6 +339,15 @@ $ENV{MSBFLAGS}="/m";
</para></listitem>
</varlistentry>
+ <varlistentry>
+ <term><productname>lz4</productname></term>
+ <listitem><para>
+ Required for supporting lz4 compression method for compressing the
+ table data. Binaries can be downloaded from
+ <ulink url="https://github.com/lz4/lz4/releases"></ulink>.
+ </para></listitem>
+ </varlistentry>
+
</variablelist>
</para>
</sect2>
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index d2bc7ab..4658637 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -532,6 +532,10 @@ sub GenerateFiles
$define{HAVE_LIBXSLT} = 1;
$define{USE_LIBXSLT} = 1;
}
+ if ($self->{options}->{lz4})
+ {
+ $define{HAVE_LIBLZ4} = 1;
+ }
if ($self->{options}->{openssl})
{
$define{USE_OPENSSL} = 1;
@@ -1051,6 +1055,11 @@ sub AddProject
$proj->AddIncludeDir($self->{options}->{xslt} . '\include');
$proj->AddLibrary($self->{options}->{xslt} . '\lib\libxslt.lib');
}
+ if ($self->{options}->{lz4})
+ {
+ $proj->AddIncludeDir($self->{options}->{lz4} . '\include');
+ $proj->AddLibrary($self->{options}->{lz4} . '\lib\liblz4_static.lib');
+ }
if ($self->{options}->{uuid})
{
$proj->AddIncludeDir($self->{options}->{uuid} . '\include');
@@ -1162,6 +1171,7 @@ sub GetFakeConfigure
$cfg .= ' --with-uuid' if ($self->{options}->{uuid});
$cfg .= ' --with-libxml' if ($self->{options}->{xml});
$cfg .= ' --with-libxslt' if ($self->{options}->{xslt});
+ $cfg .= ' --with-lz4' if ($self->{options}->{lz4});
$cfg .= ' --with-gssapi' if ($self->{options}->{gss});
$cfg .= ' --with-icu' if ($self->{options}->{icu});
$cfg .= ' --with-tcl' if ($self->{options}->{tcl});
--
1.8.3.1
On Sat, May 08, 2021 at 09:06:18AM -0500, Justin Pryzby wrote:
I suggest that should reference guc-default-toast-compression instead of CREATE
TABLE, since CREATE TABLE is large and very non-specific.
Yes, that's a better idea.
Also, in at least 3 places there's extraneous trailing whitespace.
Two of these should (I think) be a blank line.
Fixed these, and applied this doc patch as a first piece. Thanks for
the review.
--
Michael
On Sat, May 08, 2021 at 08:19:03PM +0530, Dilip Kumar wrote:
I have fixed some of them, I could not write the test cases where we
have to ensure that 'ALTER TABLE COMPRESSION' is not appearing in the
dump. Basically, I don't have knowledge of the perl language so even
after trying for some time I could not write those 2 test cases. I
have fixed the remaining comments.
Thanks. I have spent some time on that, and after adding some tests
with --no-toast-compression, I have applied this part.
Now comes the last part of the thread: support for the build with
MSVC. I have looked in details at the binaries provided by upstream
on its release page, but these are for msys and mingw, so MSVC won't
work with that.
Saying that, the upstream code can be compiled across various MSVC
versions, with 2010 being the oldest version supported, even if there
is no compiled libraries provided on the release pages. The way of
doing things here is to compile the code by yourself after downloading
the source tarball, with liblz4.lib and liblz4.dll being the generated
bits interesting for Postgres, so using
https://github.com/lz4/lz4/releases as reference for the download
looks enough, still that requires some efforts from the users to be
able to do that. Another trick is to use vcpkg, but the deliverables
generated are named lz4.{dll,lib} which is inconsistent with the
upstream naming liblz4.{dll,lib} (see Makefile.inc for the details).
My image of the whole thing is that this finishes by being a pain,
still that's possible, but that's similar with my experience with any
other dependencies.
I have been spending some time playing with the builds and that was
working nicely. Please note that you have missed an update in
config_default.pl and not all the CFLAGS entries were present in
GenerateFiles().
It may be nice to see if this stuff requires any adjustments for msys
and mingw, but I don't have such environments at hand.
All that leads me to the updated version attached.
Thoughts?
--
Michael
Attachments:
v3-0001-LZ4-option-for-windows-build-and-documentation-up.patchtext/x-diff; charset=us-asciiDownload
From 9825cfd54d62004b334af647817aba32e538bbf8 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Mon, 10 May 2021 14:55:39 +0900
Subject: [PATCH v3] LZ4 option for windows build and documentation update
---
doc/src/sgml/install-windows.sgml | 10 ++++++++++
src/tools/msvc/Solution.pm | 12 ++++++++++++
src/tools/msvc/config_default.pl | 1 +
3 files changed, 23 insertions(+)
diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index 64687b12e6..92087dba68 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -304,6 +304,16 @@ $ENV{MSBFLAGS}="/m";
</para></listitem>
</varlistentry>
+ <varlistentry>
+ <term><productname>LZ4</productname></term>
+ <listitem><para>
+ Required for supporting <productname>LZ4</productname> compression
+ method for compressing the table data. Binaries and source can be
+ downloaded from
+ <ulink url="https://github.com/lz4/lz4/releases"></ulink>.
+ </para></listitem>
+ </varlistentry>
+
<varlistentry>
<term><productname>OpenSSL</productname></term>
<listitem><para>
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 459579d312..60b8d6f650 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -535,6 +535,12 @@ sub GenerateFiles
$define{HAVE_LIBXSLT} = 1;
$define{USE_LIBXSLT} = 1;
}
+ if ($self->{options}->{lz4})
+ {
+ $define{HAVE_LIBLZ4} = 1;
+ $define{HAVE_LZ4_H} = 1;
+ $define{USE_LZ4} = 1;
+ }
if ($self->{options}->{openssl})
{
$define{USE_OPENSSL} = 1;
@@ -1054,6 +1060,11 @@ sub AddProject
$proj->AddIncludeDir($self->{options}->{xslt} . '\include');
$proj->AddLibrary($self->{options}->{xslt} . '\lib\libxslt.lib');
}
+ if ($self->{options}->{lz4})
+ {
+ $proj->AddIncludeDir($self->{options}->{lz4} . '\include');
+ $proj->AddLibrary($self->{options}->{lz4} . '\lib\liblz4.lib');
+ }
if ($self->{options}->{uuid})
{
$proj->AddIncludeDir($self->{options}->{uuid} . '\include');
@@ -1165,6 +1176,7 @@ sub GetFakeConfigure
$cfg .= ' --with-uuid' if ($self->{options}->{uuid});
$cfg .= ' --with-libxml' if ($self->{options}->{xml});
$cfg .= ' --with-libxslt' if ($self->{options}->{xslt});
+ $cfg .= ' --with-lz4' if ($self->{options}->{lz4});
$cfg .= ' --with-gssapi' if ($self->{options}->{gss});
$cfg .= ' --with-icu' if ($self->{options}->{icu});
$cfg .= ' --with-tcl' if ($self->{options}->{tcl});
diff --git a/src/tools/msvc/config_default.pl b/src/tools/msvc/config_default.pl
index 256878f582..460c0375d4 100644
--- a/src/tools/msvc/config_default.pl
+++ b/src/tools/msvc/config_default.pl
@@ -14,6 +14,7 @@ our $config = {
extraver => undef, # --with-extra-version=<string>
gss => undef, # --with-gssapi=<path>
icu => undef, # --with-icu=<path>
+ lz4 => undef, # --with-lz4=<path>
nls => undef, # --enable-nls=<path>
tap_tests => undef, # --enable-tap-tests
tcl => undef, # --with-tcl=<path>
--
2.31.1
On Mon, May 10, 2021 at 11:27 AM Michael Paquier <michael@paquier.xyz> wrote:
On Sat, May 08, 2021 at 08:19:03PM +0530, Dilip Kumar wrote:
I have fixed some of them, I could not write the test cases where we
have to ensure that 'ALTER TABLE COMPRESSION' is not appearing in the
dump. Basically, I don't have knowledge of the perl language so even
after trying for some time I could not write those 2 test cases. I
have fixed the remaining comments.Thanks. I have spent some time on that, and after adding some tests
with --no-toast-compression, I have applied this part.
Thanks!
Now comes the last part of the thread: support for the build with
MSVC. I have looked in details at the binaries provided by upstream
on its release page, but these are for msys and mingw, so MSVC won't
work with that.Saying that, the upstream code can be compiled across various MSVC
versions, with 2010 being the oldest version supported, even if there
is no compiled libraries provided on the release pages. The way of
doing things here is to compile the code by yourself after downloading
the source tarball, with liblz4.lib and liblz4.dll being the generated
bits interesting for Postgres, so using
https://github.com/lz4/lz4/releases as reference for the download
looks enough, still that requires some efforts from the users to be
able to do that. Another trick is to use vcpkg, but the deliverables
generated are named lz4.{dll,lib} which is inconsistent with the
upstream naming liblz4.{dll,lib} (see Makefile.inc for the details).
My image of the whole thing is that this finishes by being a pain,
still that's possible, but that's similar with my experience with any
other dependencies.
Even I was confused about that's the reason I used liblz4_static.lib,
but I see you have changed to liblz4.lib to make it consistent I
guess?
I have been spending some time playing with the builds and that was
working nicely. Please note that you have missed an update in
config_default.pl and not all the CFLAGS entries were present in
GenerateFiles().
Yeah, I have noticed, and thanks for changing that.
It may be nice to see if this stuff requires any adjustments for msys
and mingw, but I don't have such environments at hand.All that leads me to the updated version attached.
Thoughts?
Patch looks good to me, I can not verify though because I don't have
such an environment. Thanks for improving the patch.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Mon, May 10, 2021 at 12:17:19PM +0530, Dilip Kumar wrote:
Even I was confused about that's the reason I used liblz4_static.lib,
but I see you have changed to liblz4.lib to make it consistent I
guess?
That's the name the upstream code is using, yes.
Patch looks good to me, I can not verify though because I don't have
such an environment. Thanks for improving the patch.
Thanks, I got that applied to finish the work of this thread for
beta1. At least this will give people an option to test LZ4 on
Windows. Perhaps this will require some adjustments, but let's see if
that's necessary when that comes up.
--
Michael
On Tue, May 11, 2021 at 7:19 AM Michael Paquier <michael@paquier.xyz> wrote:
Patch looks good to me, I can not verify though because I don't have
such an environment. Thanks for improving the patch.Thanks, I got that applied to finish the work of this thread for
beta1. At least this will give people an option to test LZ4 on
Windows. Perhaps this will require some adjustments, but let's see if
that's necessary when that comes up.
Thanks, make sense.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com