Re: [HACKERS] Custom compression methods

Hi,

I have been testing the patches for a while , below is the code coverage
observed on the v19 patches.

Sr No File name Code Coverage

Before After
Line % Function % Line % Function %
1 src/backend/access/brin/brin_tuple.c 96.7 100 96.7 100
2 src/backend/access/common/detoast.c 88 100 88.6 100
3 src/backend/access/common/indextuple.c 97.1 100 97.1 100
4 src/backend/access/common/toast_internals.c 88.8 88.9 88.6 88.9
5 src/backend/access/common/tupdesc.c 97.2 100 97.2 100
6 src/backend/access/compression/compress_lz4.c NA NA 93.5 100
7 src/backend/access/compression/compress_pglz.c NA NA 82.2 100
8 src/backend/access/compression/compressamapi.c NA NA 78.3 100
9 src/backend/access/index/amapi.c 73.5 100 74.5 100
10 src/backend/access/table/toast_helper.c 97.5 100 97.5 100
11 src/backend/access/common/reloptions.c 90.6 83.3 89.7 81.6
12 src/backend/bootstrap/bootparse.y 84.2 100 84.2 100
13 src/backend/bootstrap/bootstrap.c 66.4 100 66.4 100
14 src/backend/commands/cluster.c 90.4 100 90.4 100
15 src/backend/catalog/heap.c 97.3 100 97.3 100
16 src/backend/catalog/index.c 93.8 94.6 93.8 94.6
17 src/backend/catalog/toasting.c 96.7 100 96.8 100
18 src/backend/catalog/objectaddress.c 89.7 95.9 89.7 95.9
19 src/backend/catalog/pg_depend.c 98.6 100 98.6 100
20 src/backend/commands/foreigncmds.c 95.7 95.5 95.6 95.2
21 src/backend/commands/compressioncmds.c NA NA 97.2 100
22 src/backend/commands/amcmds.c 92.1 100 90.1 100
23 src/backend/commands/createas.c 96.8 90 96.8 90
24 src/backend/commands/matview.c 92.5 85.7 92.6 85.7
25 src/backend/commands/tablecmds.c 93.6 98.5 93.7 98.5
26 src/backend/executor/nodeModifyTable.c 93.8 92.9 93.7 92.9
27 src/backend/nodes/copyfuncs.c 79.1 78.7 79.2 78.8
28 src/backend/nodes/equalfuncs.c 28.8 23.9 28.7 23.8
29 src/backend/nodes/nodeFuncs.c 80.4 100 80.3 100
30 src/backend/nodes/outfuncs.c 38.2 38.1 38.1 38
31 src/backend/parser/gram.y 87.6 100 87.7 100
32 src/backend/parser/parse_utilcmd.c 91.6 100 91.6 100
33 src/backend/replication/logical/reorderbuffer.c 94.1 97 94.1 97
34 src/backend/utils/adt/pg_upgrade_support.c 56.2 83.3 58.4 84.6
35 src/backend/utils/adt/pseudotypes.c 18.5 11.3 18.3 10.9
36 src/backend/utils/adt/varlena.c 86.5 89 86.6 89.1
37 src/bin/pg_dump/pg_dump.c 89.4 97.4 89.5 97.4
38 src/bin/psql/tab-complete.c 50.8 57.7 50.8 57.7
39 src/bin/psql/describe.c 60.7 55.1 60.6 54.2
40 contrib/cmzlib/cmzlib.c NA NA 74.7 87.5

Thanks.
--
Regards,
Neha Sharma

On Wed, Jan 20, 2021 at 10:18 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

> On Wed, Jan 20, 2021 at 12:37 AM Justin Pryzby <pryzby(at)telsasoft(dot)com>
> wrote:
> >
> > Thanks for updating the patch.
>
> Thanks for the review
>
> > 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.
>
> 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(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 will do that.
>
> > 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".
>
> I will work on other comments and send the updated patch in a day or two.
>
> --
> Regards,
> Dilip Kumar
> EnterpriseDB: http://www.enterprisedb.com
>
>
>