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 <dilipbal...@gmail.com> wrote: > On Wed, Jan 20, 2021 at 12:37 AM Justin Pryzby <pry...@telsasoft.com> > wrote: > > > > Thanks for updating the patch. > > Thanks for the review > > > On Mon, Jan 4, 2021 at 6:52 AM Justin Pryzby <pry...@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 <pry...@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 <dilipbal...@gmail.com> > wrote: > > > > On Mon, Jan 4, 2021 at 10:08 PM Justin Pryzby <pry...@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 <dilipbal...@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 <dilipbal...@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 <dilipbal...@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 <dilipbal...@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 > > >
