Re: [HACKERS] Custom compression methods

On Wed, Jan 20, 2021 at 12:37 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> 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

I have done that so now default will be --without-lz4

> 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,

I added HAVE_LIBLZ4 undef, but I haven't yet tested on windows as I
don't have a windows system. Later I will check this and fix if it
doesn't work.

> 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.

I am not sure whether we should hide the compression method when
HIDE_TABLEAM is set. I agree that it is actually an access method but
it is not the same right? Because we are using it for compression not
for storing data.

> 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

Fixed

> + * Fetch atttributes compression options
>
> attribute's :)

Fixed

> 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

Fixed

> 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 ?

Fixed

>
> + * ... 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".

Changed

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

Fixed

>
> + * 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".

Fixed

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com