Re: [HACKERS] Custom compression methods

Thanks for updating the patch.

On Mon, Jan 4, 2021 at 6:52 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> The most recent patch doesn't compile --without-lz4:
On Tue, Jan 05, 2021 at 11:19:33AM +0530, Dilip Kumar wrote:
> On Mon, Jan 4, 2021 at 10:08 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> > I think I first saw it on cfbot and I reproduced it locally, too.
> > http://cfbot.cputube.org/dilip-kumar.html
> >
> > I think you'll have to make --without-lz4 the default until the build
> > environments include it, otherwise the patch checker will show red :(
>
> Oh ok, but if we make by default --without-lz4 then the test cases
> will start failing which is using lz4 compression. Am I missing
> something?

The CIs are failing like this:

http://cfbot.cputube.org/dilip-kumar.html
|checking for LZ4_compress in -llz4... no
|configure: error: lz4 library not found
|If you have lz4 already installed, see config.log for details on the
|failure. It is possible the compiler isn't looking in the proper directory.
|Use --without-lz4 to disable lz4 support.

I thought that used to work (except for windows). I don't see that anything
changed in the configure tests... Is it because the CI moved off travis 2
weeks ago ? I don't' know whether the travis environment had liblz4, and I
don't remember if the build was passing or if it was failing for some other
reason. I'm guessing historic logs from travis are not available, if they ever
were.

I'm not sure how to deal with that, but maybe you'd need:
1) A separate 0001 patch *allowing* LZ4 to be enabled/disabled;
2) Current patchset needs to compile with/without LZ4, and pass tests in both
cases - maybe you can use "alternate test" output [0] to handle the "without"
case.
3) Eventually, the CI and build environments may have LZ4 installed, and then
we can have a separate debate about whether to enable it by default.

[0] cp -iv src/test/regress/results/compression.out src/test/regress/expected/compression_1.out

On Tue, Jan 05, 2021 at 02:20:26PM +0530, Dilip Kumar wrote:
> On Tue, Jan 5, 2021 at 11:19 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > On Mon, Jan 4, 2021 at 10:08 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> > > I see the windows build is failing:
> > > https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.123730
> > > |undefined symbol: HAVE_LIBLZ4 at src/include/pg_config.h line 350 at src/tools/msvc/Mkvcbuild.pm line 852.
> > > This needs to be patched: src/tools/msvc/Solution.pm
> > > You can see my zstd/pg_dump patch for an example, if needed (actually I'm not
> > > 100% sure it's working yet, since the windows build failed for another reason).
> >
> > Okay, I will check that.

This still needs help.
perl ./src/tools/msvc/mkvcbuild.pl
...
undefined symbol: HAVE_LIBLZ4 at src/include/pg_config.h line 350 at /home/pryzbyj/src/postgres/src/tools/msvc/Mkvcbuild.pm line 852.

Fix like:

+ HAVE_LIBLZ4 => $self->{options}->{zlib} ? 1 : undef,

Some more language fixes:

commit 3efafee52414503a87332fa6070541a3311a408c
Author: dilipkumar <dilipbalaut(at)gmail(dot)com>
Date: Tue Sep 8 15:24:33 2020 +0530

Built-in compression method

+ If the compression method is not specified for the compressible type then
+ it will have the default compression method. The default compression

I think this should say:
If no compression method is specified, then compressible types will have the
default compression method (pglz).

+ *
+ * Since version 11 TOAST_COMPRESS_SET_RAWSIZE also marks compressed

Should say v14 ??

diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 059dec3647..e4df6bc5c1 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -156,6 +156,14 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
/* attribute's collation */
Oid attcollation;

+ /*
+ * Oid of the compression method that will be used for compressing the value
+ * for this attribute. For the compressible atttypid this must always be a

say "For compressible types, ..."

+ * valid Oid irrespective of what is the current value of the attstorage.
+ * And for the incompressible atttypid this must always be an invalid Oid.

say "must be InvalidOid"

@@ -685,6 +686,7 @@ typedef enum TableLikeOption
CREATE_TABLE_LIKE_INDEXES = 1 << 5,
CREATE_TABLE_LIKE_STATISTICS = 1 << 6,
CREATE_TABLE_LIKE_STORAGE = 1 << 7,
+ CREATE_TABLE_LIKE_COMPRESSION = 1 << 8,

This is interesting...
I have a patch to implement LIKE .. (INCLUDING ACCESS METHOD).
I guess I should change it to say LIKE .. (TABLE ACCESS METHOD), right ?
https://commitfest.postgresql.org/31/2865/

Your first patch is large due to updating a large number of test cases to
include the "compression" column in \d+ output. Maybe that column should be
hidden when HIDE_TABLEAM is set by pg_regress ? I think that would allow
testing with alternate, default compression.

commit ddcae4095e36e94e3e7080e2ab5a8d42cc2ca843
Author: dilipkumar <dilipbalaut(at)gmail(dot)com>
Date: Tue Jan 19 15:10:14 2021 +0530

Support compression methods options

+ * we don't need do it again in cminitstate function.

need *to* do it again

+ * Fetch atttributes compression options

attribute's :)

commit b7946eda581230424f73f23d90843f4c2db946c2
Author: dilipkumar <dilipbalaut(at)gmail(dot)com>
Date: Wed Jan 13 12:14:40 2021 +0530

Create custom compression methods

+ * compression header otherwise, directly translate the buil-in compression

built-in

commit 0746a4d7a14209ebf62fe0dc1d12999ded879cfd
Author: dilipkumar <dilipbalaut(at)gmail(dot)com>
Date: Mon Jan 4 15:15:20 2021 +0530

Add support for PRESERVE

--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -15,6 +15,7 @@

#include "postgres.h"

+#include "access/compressamapi.h"

Unnecessary change to this file ?

+ * ... Collect the list of access method
+ * oids on which this attribute has a dependency upon.

"upon" is is redundant. Say "on which this attribute has a dependency".

+ * Check whether the given compression method oid is supported by
+ * the target attribue.

attribute

+ * In binary upgrade mode just create the dependency for all preserve
+ * list compression method as a dependecy.

dependency
I think you could say: "In binary upgrade mode, just create a dependency on all
preserved methods".