Re: [HACKERS] Custom compression methods

On Wed, Jan 20, 2021 at 12:37 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> Thanks for updating the patch.

Thanks for the review

> On Mon, Jan 4, 2021 at 6:52 AM Justin Pryzby <pryzby@telsasoft.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@telsasoft.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.

Okay, let me think about how to deal with this.

> 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@gmail.com> wrote:
> > > On Mon, Jan 4, 2021 at 10:08 PM Justin Pryzby <pryzby@telsasoft.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 will do that.

> Some more language fixes:
>
> commit 3efafee52414503a87332fa6070541a3311a408c
> Author: dilipkumar <dilipbalaut@gmail.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@gmail.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@gmail.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@gmail.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".

I will work on other comments and send the updated patch in a day or two.

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