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

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

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

On Fri, Jan 29, 2021 at 9:47 AM Neha Sharma
<neha(dot)sharma(at)enterprisedb(dot)com> wrote:
>
> Hi,
>
> I have been testing the patches for a while , below is the code coverage observed on the v19 patches.
>
> Sr NoFile nameCode Coverage
> BeforeAfter
> Line %Function %Line %Function %
> 1src/backend/access/brin/brin_tuple.c96.710096.7100
> 2src/backend/access/common/detoast.c8810088.6100
> 3src/backend/access/common/indextuple.c97.110097.1100
> 4src/backend/access/common/toast_internals.c88.888.988.688.9
> 5src/backend/access/common/tupdesc.c97.210097.2100
> 6src/backend/access/compression/compress_lz4.cNANA93.5100
> 7src/backend/access/compression/compress_pglz.cNANA82.2100
> 8src/backend/access/compression/compressamapi.cNANA78.3100
> 9src/backend/access/index/amapi.c73.510074.5100
> 10src/backend/access/table/toast_helper.c97.510097.5100
> 11src/backend/access/common/reloptions.c90.683.389.781.6
> 12src/backend/bootstrap/bootparse.y84.210084.2100
> 13src/backend/bootstrap/bootstrap.c66.410066.4100
> 14src/backend/commands/cluster.c90.410090.4100
> 15src/backend/catalog/heap.c97.310097.3100
> 16src/backend/catalog/index.c93.894.693.894.6
> 17src/backend/catalog/toasting.c96.710096.8100
> 18src/backend/catalog/objectaddress.c89.795.989.795.9
> 19src/backend/catalog/pg_depend.c98.610098.6100
> 20src/backend/commands/foreigncmds.c95.795.595.695.2
> 21src/backend/commands/compressioncmds.cNANA97.2100
> 22src/backend/commands/amcmds.c92.110090.1100
> 23src/backend/commands/createas.c96.89096.890
> 24src/backend/commands/matview.c92.585.792.685.7
> 25src/backend/commands/tablecmds.c93.698.593.798.5
> 26src/backend/executor/nodeModifyTable.c93.892.993.792.9
> 27src/backend/nodes/copyfuncs.c79.178.779.278.8
> 28src/backend/nodes/equalfuncs.c28.823.928.723.8
> 29src/backend/nodes/nodeFuncs.c80.410080.3100
> 30src/backend/nodes/outfuncs.c38.238.138.138
> 31src/backend/parser/gram.y87.610087.7100
> 32src/backend/parser/parse_utilcmd.c91.610091.6100
> 33src/backend/replication/logical/reorderbuffer.c94.19794.197
> 34src/backend/utils/adt/pg_upgrade_support.c56.283.358.484.6
> 35src/backend/utils/adt/pseudotypes.c18.511.318.310.9
> 36src/backend/utils/adt/varlena.c86.58986.689.1
> 37src/bin/pg_dump/pg_dump.c89.497.489.597.4
> 38src/bin/psql/tab-complete.c50.857.750.857.7
> 39src/bin/psql/describe.c60.755.160.654.2
> 40contrib/cmzlib/cmzlib.cNANA74.787.5
>

Thanks, Neha for testing this, overall coverage looks good to me
except compress_pglz.c, compressamapi.c and cmzlib.c. I will analyze
this and see if we can improve coverage for these files or not.

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

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

> + * Helper function for ATExecSetStorage and ATExecSetCompression
> + *
> + * Set the attcompression or attstorage for the respective index attribute. If
> + * attcompression is a valid oid then it will update the attcompression for the
> + * index attribute otherwise it will update the attstorage.
> + */
> +static void
> +ApplyChangesToIndexes(Relation rel, Relation attrelation, AttrNumber attnum,
> + Oid newcompression, char newstorage, LOCKMODE lockmode)

The comment should refer to "newcompression" not attcompression.
Maybe you'd want to assert that exactly one of these is valid.
Or maybe you should allow setting *both* if they're both valid.

> case AMTYPE_TABLE:
> return "TABLE";
>+ case AMTYPE_COMPRESSION:
>+ return "TABLE";

Looks like this part got confused, maybe during rebase.

> + if (strcmp(def->defname, "acceleration") == 0)
> + {
> + int32 acceleration =
> + pg_atoi(defGetString(def), sizeof(acceleration), 0);
> +
> + if (acceleration < INT32_MIN || acceleration > INT32_MAX)

Is this even reachable given that it's int32 type ?

What do you think about this idea:

On Sun, Jan 03, 2021 at 07:22:20PM -0600, Justin Pryzby wrote:
> I think there should also be an option for pg_restore, like --no-tablespaces.
> And I think there should be a GUC for default_compression, like
> default_table_access_method, so one can restore into an alternate compression
> by setting PGOPTIONS=-cdefault_compression=lz4.
>
> I'd like to be able to make all compressible columns of a table use a
> non-default compression (except those which cannot), without having to use
> \gexec... We have tables with up to 1600 columns. So a GUC would allow that.
>
> Previously (on separate threads) I wondered whether pg_dump
> --no-table-access-method was needed - maybe that be sufficient for this case,
> too, but I think it should be possible to separately avoid restoring
> compression AM and AM "proper". So maybe it'd be like --no-tableam=compress
> --no-tableam=storage or --no-tableam-all.

I implemented the GUC. I'm sending a patchset rebased with this as 0002, and
an un-rebased version in case you prefer that.

I think the pg_dump half (--no-acccess-method=compress) is independently useful
for tableams.

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

You're right that TABLEAM should only be for table AM's. So I suggest to do
something similar for HIDE_COMPRESSAM. That allows testing with an alternate,
default compression (which is possible with the added GUC).

Language/style fixen to follow:

> + /* acoid should be found in some cases */
amoid

> +-- test alter compression method for the partioned table
partitioned

> + /*
> + * Loop for all the attributes in the tuple and check if any of the
> + * attribute is compressed in the source tuple and its compression method
> + * is not same as the target compression method then we need to decompress
> + * it.

Loop *over*
any attribute is compressed (or any of the attributes *are* compressed)

> + * If we have decompressed any of the field in the source tuple then free
> + * the existing tuple.

any field (or any of the fields)

> + /* Likewise, copy compression if requested */
> + if (table_like_clause->options & CREATE_TABLE_LIKE_COMPRESSION
> + && OidIsValid(attribute->attcompression))

Should say (.. & ..) != 0 && OidIsValid()

> + else if (strcmp(def->compression, compression))

I think postgres likes you to explicitly write != 0

> + n->def = (Node *) makeString($5);;
The 0002 patch has this double semicolon, which is removed by 0003.

> + By default, each column in a partition inherits the compression method
> + from its parent table, however a different compression method can be set
> + for each partition.

I guess it should say that it inherits from the parent table's *column*.

> This clause specifies the type of access method to define.
> - Only <literal>TABLE</literal> and <literal>INDEX</literal>
> + <literal>TABLE</literal>, <literal>INDEX</literal> and <literal>COMPRESSION</literal>
> are supported at present.

I think "ONLY" got lost (?)
Maybe it should say: "Currently, TABLE, INDEX, and COMPRESSION access methods
are supported.

--
Justin

Some more review comments:

'git am' barfs on v0001 because it's got a whitespace error.

VARFLAGS_4B_C() doesn't seem to be used in any of the patches. I'm OK
with keeping it even if it's not used just because maybe someone will
need it later but, uh, don't we need to use it someplace?

To avoid moving the goalposts for a basic install, I suggest that
--with-lz4 should default to disabled. Maybe we'll want to rethink
that at some point, but since we're just getting started with this
whole thing, I don't think now is the time.

The change to ddl.sgml doesn't seem to make sense to me. There might
be someplace where we want to explain how properties are inherited in
partitioning hierarchies, but I don't think this is the right place,
and I don't think this explanation is particularly clear.

+ This clause adds the compression method to a column. The Compression
+ method can be set from available compression methods. The built-in
+ methods are <literal>pglz</literal> and <literal>lz4</literal>.
+ If no compression method is specified, then compressible types will have
+ the default compression method <literal>pglz</literal>.

Suggest: This sets the compression method for a column. The supported
compression methods are <literal>pglz</literal> and
<literal>lz4</literal>. <literal>lz4</literal> is available only if
<literal>--with-lz4</literal> was used when building
<productname>PostgreSQL</productname>. The default is
<literal>pglz</literal>.

We should make sure, if you haven't already, that trying to create a
column with LZ4 compression fails at table creation time if the build
does not support LZ4. But, someone could also create a table using a
build that has LZ4 support and then switch to a different set of
binaries that do not have it, so we need the runtime checks also.
However, those runtime checks shouldn't fail simplify from trying to
access a table that is set to use LZ4 compression; they should only
fail if we actually need to decompress an LZ4'd value.

Since indexes don't have TOAST tables, it surprises me that
brin_form_tuple() thinks it can TOAST anything. But I guess that's not
this patch's problem, if it's a problem at all.

I like the fact that you changed the message "compressed data is
corrupt" to indicate the compression method, but I think the resulting
message doesn't follow style guidelines because I don't believe we
normally put something with a colon prefix at the beginning of a
primary error message. So instead of saying "pglz: compressed data is
corrupt" I think you should say something like "compressed pglz data
is corrupt". Also, I suggest that we take this opportunity to switch
to ereport() rather than elog() and set
errcode(ERRCODE_DATA_CORRUPTED).

What testing have you done for performance impacts? Does the patch
slow things down noticeably with pglz? (Hopefully not.) Can you
measure a performance improvement with pglz? (Hopefully so.) Is it
likely to hurt performance that there's no minimum size for lz4
compression as we have for pglz? Seems like that could result in a lot
of wasted cycles trying to compress short strings.

pglz_cmcompress() cancels compression if the resulting value would be
larger than the original one, but it looks like lz4_cmcompress() will
just store the enlarged value. That seems bad.

pglz_cmcompress() doesn't need to pfree(tmp) before elog(ERROR).

CompressionOidToId(), CompressionIdToOid() and maybe other places need
to remember the message style guidelines. Primary error messages are
not capitalized.

Why should we now have to include toast_internals.h in
reorderbuffer.c, which has no other changes? That definitely shouldn't
be necessary. If something in another header file now requires
something from toast_internals.h, then that header file would be
obliged to include toast_internals.h itself. But actually that
shouldn't happen, because the whole point of toast_internals.h is that
it should not be included in very many places at all. If we're adding
stuff there that is going to be broadly needed, we're adding it in the
wrong place.

varlena.c shouldn't need toast_internals.h either, and if it did, it
should be in alphabetical order.

--
Robert Haas
EDB: http://www.enterprisedb.com

Even more review comments, still looking mostly at 0001:

If there's a reason why parallel_schedule is arranging to run the
compression test in parallel with nothing else, the comment in that
file should explain the reason. If there isn't, it should be added to
a parallel group that doesn't have the maximum number of tests yet,
probably the last such group in the file.

serial_schedule should add the test in a position that roughly
corresponds to where it appears in parallel_schedule.

I believe it's relatively standard practice to put variable
declarations at the top of the file. compress_lz4.c and
compress_pglz.c instead put those declarations nearer to the point of
use.

compressamapi.c has an awful lot of #include directives for the code
it actually contains. I believe that we should cut that down to what
is required by 0001, and other patches can add more later as required.
In fact, it's tempting to just get rid of this .c file altogether and
make the two functions it contains static inline functions in the
header, but I'm not 100% sure that's a good idea.

The copyright dates in a number of the file headers are out of date.

binary_upgrade_next_pg_am_oid and the related changes to
CreateAccessMethod don't belong in 0001, because it doesn't support
non-built-in compression methods. These changes and the related
pg_dump change should be moved to the patch that adds support for
that.

The comments added to dumpTableSchema() say that "compression is
assigned by ALTER" but don't give a reason. I think they should. I
don't know how much they need to explain about what the code does, but
they definitely need to explain why it does it. Also, isn't this bad?
If we create the column with the wrong compression setting initially
and then ALTER it, we have to rewrite the table. If it's empty, that's
cheap, but it'd still be better not to do it at all.

I'm not sure it's a good idea for dumpTableSchema() to leave out
specifying the compression method if it happens to be pglz. I think we
definitely shouldn't do it in binary-upgrade mode. What if we changed
the default in a future release? For that matter, even 0002 could make
the current approach unsafe.... I think, anyway.

The changes to pg_dump.h look like they haven't had a visit from
pgindent. You should probably try to do that for the whole patch,
though it's a bit annoying since you'll have to manually remove
unrelated changes to the same files that are being modified by the
patch. Also, why the extra blank line here?

GetAttributeCompression() is hard to understand. I suggest changing
the comment to "resolve column compression specification to an OID"
and somehow rejigger the code so that you aren't using one not-NULL
test and one NULL test on the same variable. Like maybe change the
first part to if (!IsStorageCompressible(typstorage)) { if
(compression == NULL) return InvalidOid; ereport(ERROR, ...); }

It puzzles me that CompareCompressionMethodAndDecompress() calls
slot_getallattrs() just before clearing the slot. It seems like this
ought to happen before we loop over the attributes, so that we don't
need to call slot_getattr() every time. See the comment for that
function. But even if we didn't do that for some reason, why would we
do it here? If it's already been done, it shouldn't do anything, and
if it hasn't been done, it might overwrite some of the values we just
poked into tts_values. It also seems suspicious that we can get away
with clearing the slot and then again marking it valid. I'm not sure
it really works like that. Like, can't clearing the slot invalidate
pointers stored in tts_values[]? For instance, if they are pointers
into an in-memory heap tuple, tts_heap_clear() is going to free the
tuple; if they are pointers into a buffer, tts_buffer_heap_clear() is
going to unpin it. I think the supported procedure for this sort of
thing is to have a second slot, set tts_values, tts_isnull etc. and
then materialize the slot. After materializing the new slot, it's
independent of the old slot, which can then be cleared. See for
example tts_virtual_materialize(). The whole approach you've taken
here might need to be rethought a bit. I think you are right to want
to avoid copying everything over into a new slot if nothing needs to
be done, and I think we should definitely keep that optimization, but
I think if you need to copy stuff, you have to do the above procedure
and then continue using the other slot instead of the original one.
Some places I think we have functions that return either the original
slot or a different one depending on how it goes; that might be a
useful idea here. But, you also can't just spam-create slots; it's
important that whatever ones we end up with get reused for every
tuple.

Doesn't the change to describeOneTableDetails() require declaring
changing the declaration of char *headers[11] to char *headers[12]?
How does this not fail Assert(cols <= lengthof(headers))?

Why does describeOneTableDetais() arrange to truncate the printed
value? We don't seem to do that for the other column properties, and
it's not like this one is particularly long.

Perhaps the changes to pg_am.dat shouldn't remove the blank line?

I think the comment to pg_attribute.h could be rephrased to stay
something like: "OID of compression AM. Must be InvalidOid if and only
if typstorage is 'a' or 'b'," replacing 'a' and 'b' with whatever the
right letters are. This would be shorter and I think also clearer than
what you have

The first comment change in postgres.h is wrong. You changed
va_extsize to "size in va_extinfo" but the associated structure
definition is unchanged, so the comment shouldn't be changed either.

In toast_internals.h, you end using 30 as a constant several times but
have no #define for it. You do have a #define for RAWSIZEMASK, but
that's really a derived value from 30. Also, it's not a great name
because it's kind of generic. So how about something like:

#define TOAST_RAWSIZE_BITS 30
#define TOAST_RAWSIZE_MASK ((1 << (TOAST_RAW_SIZE_BITS + 1)) - 1)

But then again on second thought, this 30 seems to be the same 30 that
shows up in the changes to postgres.h, and there again 0x3FFFFFFF
shows up too. So maybe we should actually be defining these constants
there, using names like VARLENA_RAWSIZE_BITS and VARLENA_RAWSIZE_MASK
and then having toast_internals.h use those constants as well.

Taken with the email I sent yesterday, I think this is a more or less
complete review of 0001. Although there are a bunch of things to fix
here still, I don't think this is that far from being committable. I
don't at this point see too much in terms of big design problems.
Probably the CompareCompressionMethodAndDecompress() is the closest to
a design-level problem, and certainly something needs to be done about
it, but even that is a fairly localized problem in the context of the
entire patch.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Feb 3, 2021 at 2:07 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> Even more review comments, still looking mostly at 0001:
>
> If there's a reason why parallel_schedule is arranging to run the
> compression test in parallel with nothing else, the comment in that
> file should explain the reason. If there isn't, it should be added to
> a parallel group that doesn't have the maximum number of tests yet,
> probably the last such group in the file.
>
> serial_schedule should add the test in a position that roughly
> corresponds to where it appears in parallel_schedule.
>
> I believe it's relatively standard practice to put variable
> declarations at the top of the file. compress_lz4.c and
> compress_pglz.c instead put those declarations nearer to the point of
> use.
>
> compressamapi.c has an awful lot of #include directives for the code
> it actually contains. I believe that we should cut that down to what
> is required by 0001, and other patches can add more later as required.
> In fact, it's tempting to just get rid of this .c file altogether and
> make the two functions it contains static inline functions in the
> header, but I'm not 100% sure that's a good idea.
>
> The copyright dates in a number of the file headers are out of date.
>
> binary_upgrade_next_pg_am_oid and the related changes to
> CreateAccessMethod don't belong in 0001, because it doesn't support
> non-built-in compression methods. These changes and the related
> pg_dump change should be moved to the patch that adds support for
> that.
>
> The comments added to dumpTableSchema() say that "compression is
> assigned by ALTER" but don't give a reason. I think they should. I
> don't know how much they need to explain about what the code does, but
> they definitely need to explain why it does it. Also, isn't this bad?
> If we create the column with the wrong compression setting initially
> and then ALTER it, we have to rewrite the table. If it's empty, that's
> cheap, but it'd still be better not to do it at all.
>
> I'm not sure it's a good idea for dumpTableSchema() to leave out
> specifying the compression method if it happens to be pglz. I think we
> definitely shouldn't do it in binary-upgrade mode. What if we changed
> the default in a future release? For that matter, even 0002 could make
> the current approach unsafe.... I think, anyway.
>
> The changes to pg_dump.h look like they haven't had a visit from
> pgindent. You should probably try to do that for the whole patch,
> though it's a bit annoying since you'll have to manually remove
> unrelated changes to the same files that are being modified by the
> patch. Also, why the extra blank line here?
>
> GetAttributeCompression() is hard to understand. I suggest changing
> the comment to "resolve column compression specification to an OID"
> and somehow rejigger the code so that you aren't using one not-NULL
> test and one NULL test on the same variable. Like maybe change the
> first part to if (!IsStorageCompressible(typstorage)) { if
> (compression == NULL) return InvalidOid; ereport(ERROR, ...); }
>
> It puzzles me that CompareCompressionMethodAndDecompress() calls
> slot_getallattrs() just before clearing the slot. It seems like this
> ought to happen before we loop over the attributes, so that we don't
> need to call slot_getattr() every time. See the comment for that
> function. But even if we didn't do that for some reason, why would we
> do it here? If it's already been done, it shouldn't do anything, and
> if it hasn't been done, it might overwrite some of the values we just
> poked into tts_values. It also seems suspicious that we can get away
> with clearing the slot and then again marking it valid. I'm not sure
> it really works like that. Like, can't clearing the slot invalidate
> pointers stored in tts_values[]? For instance, if they are pointers
> into an in-memory heap tuple, tts_heap_clear() is going to free the
> tuple; if they are pointers into a buffer, tts_buffer_heap_clear() is
> going to unpin it. I think the supported procedure for this sort of
> thing is to have a second slot, set tts_values, tts_isnull etc. and
> then materialize the slot. After materializing the new slot, it's
> independent of the old slot, which can then be cleared. See for
> example tts_virtual_materialize(). The whole approach you've taken
> here might need to be rethought a bit. I think you are right to want
> to avoid copying everything over into a new slot if nothing needs to
> be done, and I think we should definitely keep that optimization, but
> I think if you need to copy stuff, you have to do the above procedure
> and then continue using the other slot instead of the original one.
> Some places I think we have functions that return either the original
> slot or a different one depending on how it goes; that might be a
> useful idea here. But, you also can't just spam-create slots; it's
> important that whatever ones we end up with get reused for every
> tuple.
>
> Doesn't the change to describeOneTableDetails() require declaring
> changing the declaration of char *headers[11] to char *headers[12]?
> How does this not fail Assert(cols <= lengthof(headers))?
>
> Why does describeOneTableDetais() arrange to truncate the printed
> value? We don't seem to do that for the other column properties, and
> it's not like this one is particularly long.
>
> Perhaps the changes to pg_am.dat shouldn't remove the blank line?
>
> I think the comment to pg_attribute.h could be rephrased to stay
> something like: "OID of compression AM. Must be InvalidOid if and only
> if typstorage is 'a' or 'b'," replacing 'a' and 'b' with whatever the
> right letters are. This would be shorter and I think also clearer than
> what you have
>
> The first comment change in postgres.h is wrong. You changed
> va_extsize to "size in va_extinfo" but the associated structure
> definition is unchanged, so the comment shouldn't be changed either.
>
> In toast_internals.h, you end using 30 as a constant several times but
> have no #define for it. You do have a #define for RAWSIZEMASK, but
> that's really a derived value from 30. Also, it's not a great name
> because it's kind of generic. So how about something like:
>
> #define TOAST_RAWSIZE_BITS 30
> #define TOAST_RAWSIZE_MASK ((1 << (TOAST_RAW_SIZE_BITS + 1)) - 1)
>
> But then again on second thought, this 30 seems to be the same 30 that
> shows up in the changes to postgres.h, and there again 0x3FFFFFFF
> shows up too. So maybe we should actually be defining these constants
> there, using names like VARLENA_RAWSIZE_BITS and VARLENA_RAWSIZE_MASK
> and then having toast_internals.h use those constants as well.
>
> Taken with the email I sent yesterday, I think this is a more or less
> complete review of 0001. Although there are a bunch of things to fix
> here still, I don't think this is that far from being committable. I
> don't at this point see too much in terms of big design problems.
> Probably the CompareCompressionMethodAndDecompress() is the closest to
> a design-level problem, and certainly something needs to be done about
> it, but even that is a fairly localized problem in the context of the
> entire patch.

Thanks, Robert for the detailed review. I will work on these comments
and post the updated patch.

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

On Wed, Feb 3, 2021 at 2:07 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

While going through your comments, I need some suggestions about the
one except this all other comments looks fine to me.
>
> It puzzles me that CompareCompressionMethodAndDecompress() calls
> slot_getallattrs() just before clearing the slot. It seems like this
> ought to happen before we loop over the attributes, so that we don't
> need to call slot_getattr() every time.

Yeah, actually, I thought I would avoid calling slot_getallattrs if
none of the attributes got decompress. I agree if we call this before
we can avoid calling slot_getattr but slot_getattr
is only called for the attribute which has attlen -1. I agree that if
we call slot_getattr for attnum n then it will deform all the
attributes before that. But then slot_getallattrs only need to deform
the remaining attributes not all. But maybe we can call the
slot_getallattrs as soon as we see the first attribute with attlen -1
and then avoid calling subsequent slot_getattr, maybe that is better
than compared to what I have because we will avoid calling
slot_getattr for many attributes, especially when there are many
verlena.

See the comment for that
> function. But even if we didn't do that for some reason, why would we
> do it here? If it's already been done, it shouldn't do anything, and
> if it hasn't been done, it might overwrite some of the values we just
> poked into tts_values.

It will not overwrite those values because slot_getallattrs will only
fetch the values for "attnum > slot->tts_nvalid" so whatever we
already fetched will not be overwritten. Just did that at the end to
optimize the normal cases where we are not doing "insert into select *
from" so that those can get away without calling slot_getallattrs at
all. However, maybe calling slot_getattr for each varlena might cost
us extra so I am okay to call slot_getallattrs this early.

It also seems suspicious that we can get away
> with clearing the slot and then again marking it valid. I'm not sure
> it really works like that. Like, can't clearing the slot invalidate
> pointers stored in tts_values[]? For instance, if they are pointers
> into an in-memory heap tuple, tts_heap_clear() is going to free the
> tuple; if they are pointers into a buffer, tts_buffer_heap_clear() is
> going to unpin it.

Yeah, that's completely wrong. I think I missed that part. One
solution can be that we can just detach the tuple from the slot and
then materialize so it will form the tuple with new values and then we
can clear the old tuple. But that seems a bit hacky.

I think the supported procedure for this sort of
> thing is to have a second slot, set tts_values, tts_isnull etc. and
> then materialize the slot. After materializing the new slot, it's
> independent of the old slot, which can then be cleared. See for
> example tts_virtual_materialize().

Okay, so if we take a new slot then we need to set this slot reference
in the ScanState also otherwise that might point to the old slot. I
haven't yet analyzed where all we might be keeping the reference to
that old slot. Or I am missing something.

Anyway, I will get a better idea once I try to implement this.

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

On Thu, Feb 4, 2021 at 11:39 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> Yeah, actually, I thought I would avoid calling slot_getallattrs if
> none of the attributes got decompress. I agree if we call this before
> we can avoid calling slot_getattr but slot_getattr
> is only called for the attribute which has attlen -1. I agree that if
> we call slot_getattr for attnum n then it will deform all the
> attributes before that. But then slot_getallattrs only need to deform
> the remaining attributes not all. But maybe we can call the
> slot_getallattrs as soon as we see the first attribute with attlen -1
> and then avoid calling subsequent slot_getattr, maybe that is better
> than compared to what I have because we will avoid calling
> slot_getattr for many attributes, especially when there are many
> verlena.

I think that if we need to deform at all, we need to deform all
attributes, right? So there's no point in considering e.g.
slot_getsomeattrs(). But just slot_getallattrs() as soon as we know we
need to do it might be worthwhile. Could even have two loops: one that
just figures out whether we need to deform; if not, return. Then
slot_getallattrs(). Then another loop to do the work.

> I think the supported procedure for this sort of
> > thing is to have a second slot, set tts_values, tts_isnull etc. and
> > then materialize the slot. After materializing the new slot, it's
> > independent of the old slot, which can then be cleared. See for
> > example tts_virtual_materialize().
>
> Okay, so if we take a new slot then we need to set this slot reference
> in the ScanState also otherwise that might point to the old slot. I
> haven't yet analyzed where all we might be keeping the reference to
> that old slot. Or I am missing something.

My guess is you want to leave the ScanState alone so that we keep
fetching into the same slot as before and have an extra slot on the
side someplace.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, Feb 5, 2021 at 3:51 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Thu, Feb 4, 2021 at 11:39 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > Yeah, actually, I thought I would avoid calling slot_getallattrs if
> > none of the attributes got decompress. I agree if we call this before
> > we can avoid calling slot_getattr but slot_getattr
> > is only called for the attribute which has attlen -1. I agree that if
> > we call slot_getattr for attnum n then it will deform all the
> > attributes before that. But then slot_getallattrs only need to deform
> > the remaining attributes not all. But maybe we can call the
> > slot_getallattrs as soon as we see the first attribute with attlen -1
> > and then avoid calling subsequent slot_getattr, maybe that is better
> > than compared to what I have because we will avoid calling
> > slot_getattr for many attributes, especially when there are many
> > verlena.
>
> I think that if we need to deform at all, we need to deform all
> attributes, right?

IMHO that is not true, because we might need to deform the attribute
just to check its stored compression. So for example the first
attribute is varchar and the remaining 100 attributes are interger.
So we just need to deform the first attribute and if the compression
method of that is the same as the target attribute then we are done
and not need to deform the remaining and we can just continue with the
original slot and tuple.

I am not saying this is a very practical example and we have to do it
like this, but I am just making a point that it is not true that if we
deform at all then we have to deform all. However, if we decompress
any then we have to deform all because we need to materialize the
tuple again.

So there's no point in considering e.g.
> slot_getsomeattrs(). But just slot_getallattrs() as soon as we know we
> need to do it might be worthwhile. Could even have two loops: one that
> just figures out whether we need to deform; if not, return. Then
> slot_getallattrs(). Then another loop to do the work.
>
> > I think the supported procedure for this sort of
> > > thing is to have a second slot, set tts_values, tts_isnull etc. and
> > > then materialize the slot. After materializing the new slot, it's
> > > independent of the old slot, which can then be cleared. See for
> > > example tts_virtual_materialize().
> >
> > Okay, so if we take a new slot then we need to set this slot reference
> > in the ScanState also otherwise that might point to the old slot. I
> > haven't yet analyzed where all we might be keeping the reference to
> > that old slot. Or I am missing something.
>
> My guess is you want to leave the ScanState alone so that we keep
> fetching into the same slot as before and have an extra slot on the
> side someplace.

Okay, got your point. Thanks.

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

On Wed, Feb 3, 2021 at 2:07 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> Even more review comments, still looking mostly at 0001:
>
> If there's a reason why parallel_schedule is arranging to run the
> compression test in parallel with nothing else, the comment in that
> file should explain the reason. If there isn't, it should be added to
> a parallel group that doesn't have the maximum number of tests yet,
> probably the last such group in the file.
>
> serial_schedule should add the test in a position that roughly
> corresponds to where it appears in parallel_schedule.

Done

> I believe it's relatively standard practice to put variable
> declarations at the top of the file. compress_lz4.c and
> compress_pglz.c instead put those declarations nearer to the point of
> use.

Do you mean pglz_compress_methods and lz4_compress_methods ? I
followed that style from
heapam_handler.c. If you think that doesn't look good then I can move it up.

> compressamapi.c has an awful lot of #include directives for the code
> it actually contains. I believe that we should cut that down to what
> is required by 0001, and other patches can add more later as required.
> In fact, it's tempting to just get rid of this .c file altogether and
> make the two functions it contains static inline functions in the
> header, but I'm not 100% sure that's a good idea.

I think it looks better to move them to compressamapi.h so done that.

> The copyright dates in a number of the file headers are out of date.

Fixed

> binary_upgrade_next_pg_am_oid and the related changes to
> CreateAccessMethod don't belong in 0001, because it doesn't support
> non-built-in compression methods. These changes and the related
> pg_dump change should be moved to the patch that adds support for
> that.

Fixed

> The comments added to dumpTableSchema() say that "compression is
> assigned by ALTER" but don't give a reason. I think they should. I
> don't know how much they need to explain about what the code does, but
> they definitely need to explain why it does it. Also, isn't this bad?
> If we create the column with the wrong compression setting initially
> and then ALTER it, we have to rewrite the table. If it's empty, that's
> cheap, but it'd still be better not to do it at all.

Yeah, actually that part should go in 0003 patch where we implement
the custom compression method.
in that patch we need to alter and set because we want to keep the
preserved method as well
So I will add it there

> I'm not sure it's a good idea for dumpTableSchema() to leave out
> specifying the compression method if it happens to be pglz. I think we
> definitely shouldn't do it in binary-upgrade mode. What if we changed
> the default in a future release? For that matter, even 0002 could make
> the current approach unsafe.... I think, anyway.

Fixed

> The changes to pg_dump.h look like they haven't had a visit from
> pgindent. You should probably try to do that for the whole patch,
> though it's a bit annoying since you'll have to manually remove
> unrelated changes to the same files that are being modified by the
> patch. Also, why the extra blank line here?

Fixed, ran pgindent for other files as well.

> GetAttributeCompression() is hard to understand. I suggest changing
> the comment to "resolve column compression specification to an OID"
> and somehow rejigger the code so that you aren't using one not-NULL
> test and one NULL test on the same variable. Like maybe change the
> first part to if (!IsStorageCompressible(typstorage)) { if
> (compression == NULL) return InvalidOid; ereport(ERROR, ...); }

Done

> It puzzles me that CompareCompressionMethodAndDecompress() calls
> slot_getallattrs() just before clearing the slot. It seems like this
> ought to happen before we loop over the attributes, so that we don't
> need to call slot_getattr() every time. See the comment for that
> function. But even if we didn't do that for some reason, why would we
> do it here? If it's already been done, it shouldn't do anything, and
> if it hasn't been done, it might overwrite some of the values we just
> poked into tts_values. It also seems suspicious that we can get away
> with clearing the slot and then again marking it valid. I'm not sure
> it really works like that. Like, can't clearing the slot invalidate
> pointers stored in tts_values[]? For instance, if they are pointers
> into an in-memory heap tuple, tts_heap_clear() is going to free the
> tuple; if they are pointers into a buffer, tts_buffer_heap_clear() is
> going to unpin it. I think the supported procedure for this sort of
> thing is to have a second slot, set tts_values, tts_isnull etc. and
> then materialize the slot. After materializing the new slot, it's
> independent of the old slot, which can then be cleared. See for
> example tts_virtual_materialize(). The whole approach you've taken
> here might need to be rethought a bit. I think you are right to want
> to avoid copying everything over into a new slot if nothing needs to
> be done, and I think we should definitely keep that optimization, but
> I think if you need to copy stuff, you have to do the above procedure
> and then continue using the other slot instead of the original one.
> Some places I think we have functions that return either the original
> slot or a different one depending on how it goes; that might be a
> useful idea here. But, you also can't just spam-create slots; it's
> important that whatever ones we end up with get reused for every
> tuple.

I have changed this algorithm, so now if we have to decompress
anything we will use the new slot and we will stick that new slot to
the ModifyTableState, DR_transientrel for matviews and DR_intorel for
CTAS. Does this looks okay or we need to do something else? If this
logic looks fine then maybe we can think of some more optimization and
cleanup in this function.

> Doesn't the change to describeOneTableDetails() require declaring
> changing the declaration of char *headers[11] to char *headers[12]?
> How does this not fail Assert(cols <= lengthof(headers))?

Fixed

> Why does describeOneTableDetais() arrange to truncate the printed
> value? We don't seem to do that for the other column properties, and
> it's not like this one is particularly long.

Not required, fixed.

> Perhaps the changes to pg_am.dat shouldn't remove the blank line?

Fixed

> I think the comment to pg_attribute.h could be rephrased to stay
> something like: "OID of compression AM. Must be InvalidOid if and only
> if typstorage is 'a' or 'b'," replacing 'a' and 'b' with whatever the
> right letters are. This would be shorter and I think also clearer than
> what you have

Fixed

> The first comment change in postgres.h is wrong. You changed
> va_extsize to "size in va_extinfo" but the associated structure
> definition is unchanged, so the comment shouldn't be changed either.

Yup, not required.

> In toast_internals.h, you end using 30 as a constant several times but
> have no #define for it. You do have a #define for RAWSIZEMASK, but
> that's really a derived value from 30. Also, it's not a great name
> because it's kind of generic. So how about something like:
>
> #define TOAST_RAWSIZE_BITS 30
> #define TOAST_RAWSIZE_MASK ((1 << (TOAST_RAW_SIZE_BITS + 1)) - 1)
>
> But then again on second thought, this 30 seems to be the same 30 that
> shows up in the changes to postgres.h, and there again 0x3FFFFFFF
> shows up too. So maybe we should actually be defining these constants
> there, using names like VARLENA_RAWSIZE_BITS and VARLENA_RAWSIZE_MASK
> and then having toast_internals.h use those constants as well.

Done, IMHO it should be
#define VARLENA_RAWSIZE_BITS 30
#define VARLENA_RAWSIZE_MASK ((1 << VARLENA_RAWSIZE_BITS) -1 )

> Taken with the email I sent yesterday, I think this is a more or less
> complete review of 0001. Although there are a bunch of things to fix
> here still, I don't think this is that far from being committable. I
> don't at this point see too much in terms of big design problems.
> Probably the CompareCompressionMethodAndDecompress() is the closest to
> a design-level problem, and certainly something needs to be done about
> it, but even that is a fairly localized problem in the context of the
> entire patch.

0001 is attached, now pending parts are

- Confirm the new design of CompareCompressionMethodAndDecompress
- Performance test, especially lz4 with small varlena
- Rebase other patches atop this patch
- comment in ddl.sgml

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

On Tue, Feb 2, 2021 at 2:45 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> Some more review comments:
>
> 'git am' barfs on v0001 because it's got a whitespace error.

Fixed

> VARFLAGS_4B_C() doesn't seem to be used in any of the patches. I'm OK
> with keeping it even if it's not used just because maybe someone will
> need it later but, uh, don't we need to use it someplace?

Actually I was using TOAST_COMPRESS_METHOD and that required inclusion
of toast_internal.h so now
I have used VARFLAGS_4B_C and with that we are able to remove the
inclusion of toast_internal.h
in unwanted places.

> To avoid moving the goalposts for a basic install, I suggest that
> --with-lz4 should default to disabled. Maybe we'll want to rethink
> that at some point, but since we're just getting started with this
> whole thing, I don't think now is the time.

Done

> The change to ddl.sgml doesn't seem to make sense to me. There might
> be someplace where we want to explain how properties are inherited in
> partitioning hierarchies, but I don't think this is the right place,
> and I don't think this explanation is particularly clear.

Not yet done, I thought at the same place we are describing the
storage relationship with the partition so that is the place for the
compression also. Maybe I will have to read ddl.sgml file and find
out the most suitable place. So I kept is as pending.

> + This clause adds the compression method to a column. The Compression
> + method can be set from available compression methods. The built-in
> + methods are <literal>pglz</literal> and <literal>lz4</literal>.
> + If no compression method is specified, then compressible types will have
> + the default compression method <literal>pglz</literal>.
>
> Suggest: This sets the compression method for a column. The supported
> compression methods are <literal>pglz</literal> and
> <literal>lz4</literal>. <literal>lz4</literal> is available only if
> <literal>--with-lz4</literal> was used when building
> <productname>PostgreSQL</productname>. The default is
> <literal>pglz</literal>.

Done

> We should make sure, if you haven't already, that trying to create a
> column with LZ4 compression fails at table creation time if the build
> does not support LZ4. But, someone could also create a table using a
> build that has LZ4 support and then switch to a different set of
> binaries that do not have it, so we need the runtime checks also.
> However, those runtime checks shouldn't fail simplify from trying to
> access a table that is set to use LZ4 compression; they should only
> fail if we actually need to decompress an LZ4'd value.

Done, I have cheched the compression method Oid if it LZ4 and if the
lz4 library is not install
then error out. We can also use the handler sepcific check function
but I am not sure does that make
sense to add extra routine for that. In later patch 0006 we have an
check function to verify the
option so during that we can error out and no need to check this outside.

> Since indexes don't have TOAST tables, it surprises me that
> brin_form_tuple() thinks it can TOAST anything. But I guess that's not
> this patch's problem, if it's a problem at all.

it is just trying to compress it not externalize.

> I like the fact that you changed the message "compressed data is
> corrupt" to indicate the compression method, but I think the resulting
> message doesn't follow style guidelines because I don't believe we
> normally put something with a colon prefix at the beginning of a
> primary error message. So instead of saying "pglz: compressed data is
> corrupt" I think you should say something like "compressed pglz data
> is corrupt". Also, I suggest that we take this opportunity to switch
> to ereport() rather than elog() and set
> errcode(ERRCODE_DATA_CORRUPTED).

Done

>
> What testing have you done for performance impacts? Does the patch
> slow things down noticeably with pglz? (Hopefully not.) Can you
> measure a performance improvement with pglz? (Hopefully so.) Is it
> likely to hurt performance that there's no minimum size for lz4
> compression as we have for pglz? Seems like that could result in a lot
> of wasted cycles trying to compress short strings.

Not sure what to do about this, I will check the performance with
small varlenas and see.

> pglz_cmcompress() cancels compression if the resulting value would be
> larger than the original one, but it looks like lz4_cmcompress() will
> just store the enlarged value. That seems bad.

you mean lz4_cmcompress, Done

> pglz_cmcompress() doesn't need to pfree(tmp) before elog(ERROR).

Done

> CompressionOidToId(), CompressionIdToOid() and maybe other places need
> to remember the message style guidelines. Primary error messages are
> not capitalized.

Fixed

> Why should we now have to include toast_internals.h in
> reorderbuffer.c, which has no other changes? That definitely shouldn't
> be necessary. If something in another header file now requires
> something from toast_internals.h, then that header file would be
> obliged to include toast_internals.h itself. But actually that
> shouldn't happen, because the whole point of toast_internals.h is that
> it should not be included in very many places at all. If we're adding
> stuff there that is going to be broadly needed, we're adding it in the
> wrong place.

Done

> varlena.c shouldn't need toast_internals.h either, and if it did, it
> should be in alphabetical order.
>

It was the wrong usage, fixed now.

Please refer to the latest patch at

https://www.postgresql.org/message-id/CAFiTN-v9Cs1MORnp-3bGZ5QBwr5v3VarSvfaDizHi1acXES5xQ%40mail.gmail.com

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

+ * If the called has passed an invalid slot then create a new slot.
*caller

+ * Slot for storing the modified tuple, incase the target attribute's
*in case

Could you comment on the patch I sent on Jan 30 ? I think it would be squished
into 0001.
> Subject: [PATCH v21 2/7] Add default_toast_compression GUC

Also, what about the idea to add HIDE_COMPRESSAM ? Right now, your patch
changes a great many regression tests, and I doubt many people are going to try
to look closely to verify the differences, now, or when setting a non-default
compression method.

Also, I think we may want to make enable-lz4 the default *for testing
purposes*, now that the linux and BSD environments include that.

--
Justin

On Fri, Feb 5, 2021 at 10:56 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> Could you comment on the patch I sent on Jan 30 ? I think it would be squished
> into 0001.

I don't see why we have to do that. Seems fine to have it as a separate patch.

> Also, what about the idea to add HIDE_COMPRESSAM ? Right now, your patch
> changes a great many regression tests, and I doubt many people are going to try
> to look closely to verify the differences, now, or when setting a non-default
> compression method.

Personally, my preference is to just update the test outputs. It's not
important whether many people look closely to verify the differences;
we just need to look them over on a one-time basis to see if they seem
OK. After that it's 0 effort, vs. having to maintain HIDE_COMPRESSAM
forever.

> Also, I think we may want to make enable-lz4 the default *for testing
> purposes*, now that the linux and BSD environments include that.

My guess was that would annoy some hackers whose build environments
got broken. If everyone thinks otherwise I'm willing to be persuaded,
but it's going to take more than 1 vote...

--
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, Feb 5, 2021 at 11:07 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Personally, my preference is to just update the test outputs. It's not
> important whether many people look closely to verify the differences;
> we just need to look them over on a one-time basis to see if they seem
> OK. After that it's 0 effort, vs. having to maintain HIDE_COMPRESSAM
> forever.

Oh, I guess you're thinking about the case where someone wants to run
the tests with a different default. That might be a good reason to
have this. But then those changes should go in 0002.

Regarding 0002, I'm not feeling very excited about having every call
to TupleDescInitEntry() do an extra syscache lookup. It's going to be
the same lookup every time forever to get the same value every time
forever. Now maybe that function can never get hot enough for it to
matter, but can't we find a way to be smarter about this? Like,
suppose we cache the OID in a global variable the first time we look
it up, and then use CacheRegisterSyscacheCallback() to have it zeroed
out if pg_am is updated?

Taking that idea a bit further, suppose you get rid of all the places
where you do get_compression_am_oid(default_toast_compression, false)
and change them to get_default_compression_am_oid(), which is defined
thus:

static Oid
get_default_compression_am_oid(void)
{
if (unlikely(!OidIsValid(cached_default_compression_oid))
// figure it out;
return cached_default_compression_oid;
}

Also, how about removing the debugging leftovers from syscache.c?

--
Robert Haas
EDB: http://www.enterprisedb.com

This fails make-check world for me, and CFBOT will say the same.
Be sure to compile with --enable-tap-tests

cd . && TESTDIR='/home/pryzbyj/src/postgres/src/bin/pg_dump' PATH="/home/pryzbyj/src/postgres/tmp_install/usr/local/pgsql/bin:$PATH" LD_LIBRARY_PATH="/home/pryzbyj/src/postgres/tmp_install/usr/local/pgsql/lib" PGPORT='65432' PG_REGRESS='/home/pryzbyj/src/postgres/src/bin/pg_dump/../../../src/test/regress/pg_regress' REGRESS_SHLIB='/home/pryzbyj/src/postgres/src/test/regress/regress.so' /usr/bin/prove -I ../../../src/test/perl/ -I . t/*.pl
t/001_basic.pl ................ ok
t/002_pg_dump.pl .............. 13/6408
# Failed test 'binary_upgrade: should dump CREATE TABLE test_fifth_table'
# at t/002_pg_dump.pl line 3601.
# Review binary_upgrade results in /home/pryzbyj/src/postgres/src/bin/pg_dump/tmp_check/tmp_test_gw4p

# Failed test 'binary_upgrade: should dump CREATE TABLE test_second_table'
# at t/002_pg_dump.pl line 3601.
# Review binary_upgrade results in /home/pryzbyj/src/postgres/src/bin/pg_dump/tmp_check/tmp_test_gw4p

# Failed test 'binary_upgrade: should dump CREATE TABLE test_table'
# at t/002_pg_dump.pl line 3601.
# Review binary_upgrade results in /home/pryzbyj/src/postgres/src/bin/pg_dump/tmp_check/tmp_test_gw4p

# Failed test 'binary_upgrade: should dump CREATE TABLE test_table_identity'
# at t/002_pg_dump.pl line 3601.
# Review binary_upgrade results in /home/pryzbyj/src/postgres/src/bin/pg_dump/tmp_check/tmp_test_gw4p
t/002_pg_dump.pl .............. 6203/6408 # Looks like you failed 4 tests of 6408.
t/002_pg_dump.pl .............. Dubious, test returned 4 (wstat 1024, 0x400)
Failed 4/6408 subtests

On Fri, Feb 5, 2021 at 8:11 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Wed, Feb 3, 2021 at 2:07 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >
> > Even more review comments, still looking mostly at 0001:
> >
> > If there's a reason why parallel_schedule is arranging to run the
> > compression test in parallel with nothing else, the comment in that
> > file should explain the reason. If there isn't, it should be added to
> > a parallel group that doesn't have the maximum number of tests yet,
> > probably the last such group in the file.
> >
> > serial_schedule should add the test in a position that roughly
> > corresponds to where it appears in parallel_schedule.
>
> Done
>
> > I believe it's relatively standard practice to put variable
> > declarations at the top of the file. compress_lz4.c and
> > compress_pglz.c instead put those declarations nearer to the point of
> > use.
>
> Do you mean pglz_compress_methods and lz4_compress_methods ? I
> followed that style from
> heapam_handler.c. If you think that doesn't look good then I can move it up.
>
> > compressamapi.c has an awful lot of #include directives for the code
> > it actually contains. I believe that we should cut that down to what
> > is required by 0001, and other patches can add more later as required.
> > In fact, it's tempting to just get rid of this .c file altogether and
> > make the two functions it contains static inline functions in the
> > header, but I'm not 100% sure that's a good idea.
>
> I think it looks better to move them to compressamapi.h so done that.
>
> > The copyright dates in a number of the file headers are out of date.
>
> Fixed
>
> > binary_upgrade_next_pg_am_oid and the related changes to
> > CreateAccessMethod don't belong in 0001, because it doesn't support
> > non-built-in compression methods. These changes and the related
> > pg_dump change should be moved to the patch that adds support for
> > that.
>
> Fixed
>
> > The comments added to dumpTableSchema() say that "compression is
> > assigned by ALTER" but don't give a reason. I think they should. I
> > don't know how much they need to explain about what the code does, but
> > they definitely need to explain why it does it. Also, isn't this bad?
> > If we create the column with the wrong compression setting initially
> > and then ALTER it, we have to rewrite the table. If it's empty, that's
> > cheap, but it'd still be better not to do it at all.
>
> Yeah, actually that part should go in 0003 patch where we implement
> the custom compression method.
> in that patch we need to alter and set because we want to keep the
> preserved method as well
> So I will add it there
>
> > I'm not sure it's a good idea for dumpTableSchema() to leave out
> > specifying the compression method if it happens to be pglz. I think we
> > definitely shouldn't do it in binary-upgrade mode. What if we changed
> > the default in a future release? For that matter, even 0002 could make
> > the current approach unsafe.... I think, anyway.
>
> Fixed
>
>
> > The changes to pg_dump.h look like they haven't had a visit from
> > pgindent. You should probably try to do that for the whole patch,
> > though it's a bit annoying since you'll have to manually remove
> > unrelated changes to the same files that are being modified by the
> > patch. Also, why the extra blank line here?
>
> Fixed, ran pgindent for other files as well.
>
> > GetAttributeCompression() is hard to understand. I suggest changing
> > the comment to "resolve column compression specification to an OID"
> > and somehow rejigger the code so that you aren't using one not-NULL
> > test and one NULL test on the same variable. Like maybe change the
> > first part to if (!IsStorageCompressible(typstorage)) { if
> > (compression == NULL) return InvalidOid; ereport(ERROR, ...); }
>
> Done
>
> > It puzzles me that CompareCompressionMethodAndDecompress() calls
> > slot_getallattrs() just before clearing the slot. It seems like this
> > ought to happen before we loop over the attributes, so that we don't
> > need to call slot_getattr() every time. See the comment for that
> > function. But even if we didn't do that for some reason, why would we
> > do it here? If it's already been done, it shouldn't do anything, and
> > if it hasn't been done, it might overwrite some of the values we just
> > poked into tts_values. It also seems suspicious that we can get away
> > with clearing the slot and then again marking it valid. I'm not sure
> > it really works like that. Like, can't clearing the slot invalidate
> > pointers stored in tts_values[]? For instance, if they are pointers
> > into an in-memory heap tuple, tts_heap_clear() is going to free the
> > tuple; if they are pointers into a buffer, tts_buffer_heap_clear() is
> > going to unpin it. I think the supported procedure for this sort of
> > thing is to have a second slot, set tts_values, tts_isnull etc. and
> > then materialize the slot. After materializing the new slot, it's
> > independent of the old slot, which can then be cleared. See for
> > example tts_virtual_materialize(). The whole approach you've taken
> > here might need to be rethought a bit. I think you are right to want
> > to avoid copying everything over into a new slot if nothing needs to
> > be done, and I think we should definitely keep that optimization, but
> > I think if you need to copy stuff, you have to do the above procedure
> > and then continue using the other slot instead of the original one.
> > Some places I think we have functions that return either the original
> > slot or a different one depending on how it goes; that might be a
> > useful idea here. But, you also can't just spam-create slots; it's
> > important that whatever ones we end up with get reused for every
> > tuple.
>
> I have changed this algorithm, so now if we have to decompress
> anything we will use the new slot and we will stick that new slot to
> the ModifyTableState, DR_transientrel for matviews and DR_intorel for
> CTAS. Does this looks okay or we need to do something else? If this
> logic looks fine then maybe we can think of some more optimization and
> cleanup in this function.
>
>
> > Doesn't the change to describeOneTableDetails() require declaring
> > changing the declaration of char *headers[11] to char *headers[12]?
> > How does this not fail Assert(cols <= lengthof(headers))?
>
> Fixed
>
> > Why does describeOneTableDetais() arrange to truncate the printed
> > value? We don't seem to do that for the other column properties, and
> > it's not like this one is particularly long.
>
> Not required, fixed.
>
> > Perhaps the changes to pg_am.dat shouldn't remove the blank line?
>
> Fixed
>
> > I think the comment to pg_attribute.h could be rephrased to stay
> > something like: "OID of compression AM. Must be InvalidOid if and only
> > if typstorage is 'a' or 'b'," replacing 'a' and 'b' with whatever the
> > right letters are. This would be shorter and I think also clearer than
> > what you have
>
> Fixed
>
> > The first comment change in postgres.h is wrong. You changed
> > va_extsize to "size in va_extinfo" but the associated structure
> > definition is unchanged, so the comment shouldn't be changed either.
>
> Yup, not required.
>
> > In toast_internals.h, you end using 30 as a constant several times but
> > have no #define for it. You do have a #define for RAWSIZEMASK, but
> > that's really a derived value from 30. Also, it's not a great name
> > because it's kind of generic. So how about something like:
> >
> > #define TOAST_RAWSIZE_BITS 30
> > #define TOAST_RAWSIZE_MASK ((1 << (TOAST_RAW_SIZE_BITS + 1)) - 1)
> >
> > But then again on second thought, this 30 seems to be the same 30 that
> > shows up in the changes to postgres.h, and there again 0x3FFFFFFF
> > shows up too. So maybe we should actually be defining these constants
> > there, using names like VARLENA_RAWSIZE_BITS and VARLENA_RAWSIZE_MASK
> > and then having toast_internals.h use those constants as well.
>
> Done, IMHO it should be
> #define VARLENA_RAWSIZE_BITS 30
> #define VARLENA_RAWSIZE_MASK ((1 << VARLENA_RAWSIZE_BITS) -1 )
>
>
> > Taken with the email I sent yesterday, I think this is a more or less
> > complete review of 0001. Although there are a bunch of things to fix
> > here still, I don't think this is that far from being committable. I
> > don't at this point see too much in terms of big design problems.
> > Probably the CompareCompressionMethodAndDecompress() is the closest to
> > a design-level problem, and certainly something needs to be done about
> > it, but even that is a fairly localized problem in the context of the
> > entire patch.
>
> 0001 is attached, now pending parts are
>
> - Confirm the new design of CompareCompressionMethodAndDecompress
> - Performance test, especially lz4 with small varlena

I have tested the performance, pglz vs lz4

Test1: With a small simple string, pglz doesn't select compression but
lz4 select as no min limit
Table: 100 varchar column
Test: Insert 1000 tuple, each column of 25 bytes string (32 is min
limit for pglz)
Result:
pglz: 1030 ms (doesn't attempt compression so externalize),
lz4: 212 ms

Test2: With small incompressible string, pglz don't select compression
lz4 select but can not compress
Table: 100 varchar column
Test: Insert 1000 tuple, each column of 25 bytes string (32 is min
limit for pglz)
Result:
pglz: 1030 ms (doesn't attempt compression so externalize),
lz4: 1090 ms (attempt to compress but externalize):

Test3: Test a few columns with large random data
Table: 3 varchar column
Test: Insert 1000 tuple 3 columns size(3500 byes, 4200 bytes, 4900bytes)
pglz: 150 ms (compression ratio: 3.02%),
lz4: 30 ms (compression ratio : 2.3%)

Test4: Test3 with different large random slighly compressible, need to
compress + externalize:
Table: 3 varchar column
Insert: Insert 1000 tuple 3 columns size(8192, 8192, 8192)
CREATE OR REPLACE FUNCTION large_val() RETURNS TEXT LANGUAGE SQL AS
'select array_agg(md5(g::text))::text from generate_series(1, 256) g';
Test: insert into t1 select large_val(), large_val(), large_val() from
generate_series(1,1000);
pglz: 2000 ms
lz4: 1500 ms

Conclusion:
1. In most cases lz4 is faster and doing better compression as well.
2. In Test2 when small data is incompressible then lz4 tries to
compress whereas pglz doesn't try so there is some performance loss.
But if we want we can fix
it by setting some minimum limit of size for lz4 as well, maybe the
same size as pglz?

> - Rebase other patches atop this patch
> - comment in ddl.sgml

Other changes in patch:
- Now we are dumping the default compression method in the
binary-upgrade mode so the pg_dump test needed some change, fixed
that.
- in compress_pglz.c and compress_lz4.c, we were using
toast_internal.h macros so I removed and used varlena macros instead.

While testing, I noticed that if the compressed data are externalized
then pg_column_compression(), doesn't fetch the compression method
from the toast chunk, I think we should do that. I will analyze this
and fix it in the next version.

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

On Sun, Feb 7, 2021 at 5:15 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Fri, Feb 5, 2021 at 8:11 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Wed, Feb 3, 2021 at 2:07 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > >
> > > Even more review comments, still looking mostly at 0001:
> > >
> > > If there's a reason why parallel_schedule is arranging to run the
> > > compression test in parallel with nothing else, the comment in that
> > > file should explain the reason. If there isn't, it should be added to
> > > a parallel group that doesn't have the maximum number of tests yet,
> > > probably the last such group in the file.
> > >
> > > serial_schedule should add the test in a position that roughly
> > > corresponds to where it appears in parallel_schedule.
> >
> > Done
> >
> > > I believe it's relatively standard practice to put variable
> > > declarations at the top of the file. compress_lz4.c and
> > > compress_pglz.c instead put those declarations nearer to the point of
> > > use.
> >
> > Do you mean pglz_compress_methods and lz4_compress_methods ? I
> > followed that style from
> > heapam_handler.c. If you think that doesn't look good then I can move it up.
> >
> > > compressamapi.c has an awful lot of #include directives for the code
> > > it actually contains. I believe that we should cut that down to what
> > > is required by 0001, and other patches can add more later as required.
> > > In fact, it's tempting to just get rid of this .c file altogether and
> > > make the two functions it contains static inline functions in the
> > > header, but I'm not 100% sure that's a good idea.
> >
> > I think it looks better to move them to compressamapi.h so done that.
> >
> > > The copyright dates in a number of the file headers are out of date.
> >
> > Fixed
> >
> > > binary_upgrade_next_pg_am_oid and the related changes to
> > > CreateAccessMethod don't belong in 0001, because it doesn't support
> > > non-built-in compression methods. These changes and the related
> > > pg_dump change should be moved to the patch that adds support for
> > > that.
> >
> > Fixed
> >
> > > The comments added to dumpTableSchema() say that "compression is
> > > assigned by ALTER" but don't give a reason. I think they should. I
> > > don't know how much they need to explain about what the code does, but
> > > they definitely need to explain why it does it. Also, isn't this bad?
> > > If we create the column with the wrong compression setting initially
> > > and then ALTER it, we have to rewrite the table. If it's empty, that's
> > > cheap, but it'd still be better not to do it at all.
> >
> > Yeah, actually that part should go in 0003 patch where we implement
> > the custom compression method.
> > in that patch we need to alter and set because we want to keep the
> > preserved method as well
> > So I will add it there
> >
> > > I'm not sure it's a good idea for dumpTableSchema() to leave out
> > > specifying the compression method if it happens to be pglz. I think we
> > > definitely shouldn't do it in binary-upgrade mode. What if we changed
> > > the default in a future release? For that matter, even 0002 could make
> > > the current approach unsafe.... I think, anyway.
> >
> > Fixed
> >
> >
> > > The changes to pg_dump.h look like they haven't had a visit from
> > > pgindent. You should probably try to do that for the whole patch,
> > > though it's a bit annoying since you'll have to manually remove
> > > unrelated changes to the same files that are being modified by the
> > > patch. Also, why the extra blank line here?
> >
> > Fixed, ran pgindent for other files as well.
> >
> > > GetAttributeCompression() is hard to understand. I suggest changing
> > > the comment to "resolve column compression specification to an OID"
> > > and somehow rejigger the code so that you aren't using one not-NULL
> > > test and one NULL test on the same variable. Like maybe change the
> > > first part to if (!IsStorageCompressible(typstorage)) { if
> > > (compression == NULL) return InvalidOid; ereport(ERROR, ...); }
> >
> > Done
> >
> > > It puzzles me that CompareCompressionMethodAndDecompress() calls
> > > slot_getallattrs() just before clearing the slot. It seems like this
> > > ought to happen before we loop over the attributes, so that we don't
> > > need to call slot_getattr() every time. See the comment for that
> > > function. But even if we didn't do that for some reason, why would we
> > > do it here? If it's already been done, it shouldn't do anything, and
> > > if it hasn't been done, it might overwrite some of the values we just
> > > poked into tts_values. It also seems suspicious that we can get away
> > > with clearing the slot and then again marking it valid. I'm not sure
> > > it really works like that. Like, can't clearing the slot invalidate
> > > pointers stored in tts_values[]? For instance, if they are pointers
> > > into an in-memory heap tuple, tts_heap_clear() is going to free the
> > > tuple; if they are pointers into a buffer, tts_buffer_heap_clear() is
> > > going to unpin it. I think the supported procedure for this sort of
> > > thing is to have a second slot, set tts_values, tts_isnull etc. and
> > > then materialize the slot. After materializing the new slot, it's
> > > independent of the old slot, which can then be cleared. See for
> > > example tts_virtual_materialize(). The whole approach you've taken
> > > here might need to be rethought a bit. I think you are right to want
> > > to avoid copying everything over into a new slot if nothing needs to
> > > be done, and I think we should definitely keep that optimization, but
> > > I think if you need to copy stuff, you have to do the above procedure
> > > and then continue using the other slot instead of the original one.
> > > Some places I think we have functions that return either the original
> > > slot or a different one depending on how it goes; that might be a
> > > useful idea here. But, you also can't just spam-create slots; it's
> > > important that whatever ones we end up with get reused for every
> > > tuple.
> >
> > I have changed this algorithm, so now if we have to decompress
> > anything we will use the new slot and we will stick that new slot to
> > the ModifyTableState, DR_transientrel for matviews and DR_intorel for
> > CTAS. Does this looks okay or we need to do something else? If this
> > logic looks fine then maybe we can think of some more optimization and
> > cleanup in this function.
> >
> >
> > > Doesn't the change to describeOneTableDetails() require declaring
> > > changing the declaration of char *headers[11] to char *headers[12]?
> > > How does this not fail Assert(cols <= lengthof(headers))?
> >
> > Fixed
> >
> > > Why does describeOneTableDetais() arrange to truncate the printed
> > > value? We don't seem to do that for the other column properties, and
> > > it's not like this one is particularly long.
> >
> > Not required, fixed.
> >
> > > Perhaps the changes to pg_am.dat shouldn't remove the blank line?
> >
> > Fixed
> >
> > > I think the comment to pg_attribute.h could be rephrased to stay
> > > something like: "OID of compression AM. Must be InvalidOid if and only
> > > if typstorage is 'a' or 'b'," replacing 'a' and 'b' with whatever the
> > > right letters are. This would be shorter and I think also clearer than
> > > what you have
> >
> > Fixed
> >
> > > The first comment change in postgres.h is wrong. You changed
> > > va_extsize to "size in va_extinfo" but the associated structure
> > > definition is unchanged, so the comment shouldn't be changed either.
> >
> > Yup, not required.
> >
> > > In toast_internals.h, you end using 30 as a constant several times but
> > > have no #define for it. You do have a #define for RAWSIZEMASK, but
> > > that's really a derived value from 30. Also, it's not a great name
> > > because it's kind of generic. So how about something like:
> > >
> > > #define TOAST_RAWSIZE_BITS 30
> > > #define TOAST_RAWSIZE_MASK ((1 << (TOAST_RAW_SIZE_BITS + 1)) - 1)
> > >
> > > But then again on second thought, this 30 seems to be the same 30 that
> > > shows up in the changes to postgres.h, and there again 0x3FFFFFFF
> > > shows up too. So maybe we should actually be defining these constants
> > > there, using names like VARLENA_RAWSIZE_BITS and VARLENA_RAWSIZE_MASK
> > > and then having toast_internals.h use those constants as well.
> >
> > Done, IMHO it should be
> > #define VARLENA_RAWSIZE_BITS 30
> > #define VARLENA_RAWSIZE_MASK ((1 << VARLENA_RAWSIZE_BITS) -1 )
> >
> >
> > > Taken with the email I sent yesterday, I think this is a more or less
> > > complete review of 0001. Although there are a bunch of things to fix
> > > here still, I don't think this is that far from being committable. I
> > > don't at this point see too much in terms of big design problems.
> > > Probably the CompareCompressionMethodAndDecompress() is the closest to
> > > a design-level problem, and certainly something needs to be done about
> > > it, but even that is a fairly localized problem in the context of the
> > > entire patch.
> >
> > 0001 is attached, now pending parts are
> >
> > - Confirm the new design of CompareCompressionMethodAndDecompress
> > - Performance test, especially lz4 with small varlena
>
> I have tested the performance, pglz vs lz4
>
> Test1: With a small simple string, pglz doesn't select compression but
> lz4 select as no min limit
> Table: 100 varchar column
> Test: Insert 1000 tuple, each column of 25 bytes string (32 is min
> limit for pglz)
> Result:
> pglz: 1030 ms (doesn't attempt compression so externalize),
> lz4: 212 ms
>
> Test2: With small incompressible string, pglz don't select compression
> lz4 select but can not compress
> Table: 100 varchar column
> Test: Insert 1000 tuple, each column of 25 bytes string (32 is min
> limit for pglz)
> Result:
> pglz: 1030 ms (doesn't attempt compression so externalize),
> lz4: 1090 ms (attempt to compress but externalize):
>
> Test3: Test a few columns with large random data
> Table: 3 varchar column
> Test: Insert 1000 tuple 3 columns size(3500 byes, 4200 bytes, 4900bytes)
> pglz: 150 ms (compression ratio: 3.02%),
> lz4: 30 ms (compression ratio : 2.3%)
>
> Test4: Test3 with different large random slighly compressible, need to
> compress + externalize:
> Table: 3 varchar column
> Insert: Insert 1000 tuple 3 columns size(8192, 8192, 8192)
> CREATE OR REPLACE FUNCTION large_val() RETURNS TEXT LANGUAGE SQL AS
> 'select array_agg(md5(g::text))::text from generate_series(1, 256) g';
> Test: insert into t1 select large_val(), large_val(), large_val() from
> generate_series(1,1000);
> pglz: 2000 ms
> lz4: 1500 ms
>
> Conclusion:
> 1. In most cases lz4 is faster and doing better compression as well.
> 2. In Test2 when small data is incompressible then lz4 tries to
> compress whereas pglz doesn't try so there is some performance loss.
> But if we want we can fix
> it by setting some minimum limit of size for lz4 as well, maybe the
> same size as pglz?
>
> > - Rebase other patches atop this patch
> > - comment in ddl.sgml
>
> Other changes in patch:
> - Now we are dumping the default compression method in the
> binary-upgrade mode so the pg_dump test needed some change, fixed
> that.
> - in compress_pglz.c and compress_lz4.c, we were using
> toast_internal.h macros so I removed and used varlena macros instead.
>
> While testing, I noticed that if the compressed data are externalized
> then pg_column_compression(), doesn't fetch the compression method
> from the toast chunk, I think we should do that. I will analyze this
> and fix it in the next version.

While trying to fix this, I have realized this problem exists in
CompareCompressionMethodAndDecompress
see below code.
---
+ new_value = (struct varlena *)
+ DatumGetPointer(slot->tts_values[attnum - 1]);
+
+ /* nothing to be done, if it is not compressed */
+ if (!VARATT_IS_COMPRESSED(new_value))
+ continue;
---

Basically, we are just checking whether the stored value is compressed
or not, but we are clearly ignoring the fact that it might be
compressed and stored externally on disk. So basically if the value
is stored externally we can not know whether the external data were
compressed or not without fetching the values from the toast table, I
think instead of fetching the complete data from toast we can just
fetch the header using 'toast_fetch_datum_slice'.

Any other thoughts on this?

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

On Tue, Feb 9, 2021 at 2:08 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Sun, Feb 7, 2021 at 5:15 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Fri, Feb 5, 2021 at 8:11 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Feb 3, 2021 at 2:07 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > > >
> > > > Even more review comments, still looking mostly at 0001:
> > > >
> > > > If there's a reason why parallel_schedule is arranging to run the
> > > > compression test in parallel with nothing else, the comment in that
> > > > file should explain the reason. If there isn't, it should be added to
> > > > a parallel group that doesn't have the maximum number of tests yet,
> > > > probably the last such group in the file.
> > > >
> > > > serial_schedule should add the test in a position that roughly
> > > > corresponds to where it appears in parallel_schedule.
> > >
> > > Done
> > >
> > > > I believe it's relatively standard practice to put variable
> > > > declarations at the top of the file. compress_lz4.c and
> > > > compress_pglz.c instead put those declarations nearer to the point of
> > > > use.
> > >
> > > Do you mean pglz_compress_methods and lz4_compress_methods ? I
> > > followed that style from
> > > heapam_handler.c. If you think that doesn't look good then I can move it up.
> > >
> > > > compressamapi.c has an awful lot of #include directives for the code
> > > > it actually contains. I believe that we should cut that down to what
> > > > is required by 0001, and other patches can add more later as required.
> > > > In fact, it's tempting to just get rid of this .c file altogether and
> > > > make the two functions it contains static inline functions in the
> > > > header, but I'm not 100% sure that's a good idea.
> > >
> > > I think it looks better to move them to compressamapi.h so done that.
> > >
> > > > The copyright dates in a number of the file headers are out of date.
> > >
> > > Fixed
> > >
> > > > binary_upgrade_next_pg_am_oid and the related changes to
> > > > CreateAccessMethod don't belong in 0001, because it doesn't support
> > > > non-built-in compression methods. These changes and the related
> > > > pg_dump change should be moved to the patch that adds support for
> > > > that.
> > >
> > > Fixed
> > >
> > > > The comments added to dumpTableSchema() say that "compression is
> > > > assigned by ALTER" but don't give a reason. I think they should. I
> > > > don't know how much they need to explain about what the code does, but
> > > > they definitely need to explain why it does it. Also, isn't this bad?
> > > > If we create the column with the wrong compression setting initially
> > > > and then ALTER it, we have to rewrite the table. If it's empty, that's
> > > > cheap, but it'd still be better not to do it at all.
> > >
> > > Yeah, actually that part should go in 0003 patch where we implement
> > > the custom compression method.
> > > in that patch we need to alter and set because we want to keep the
> > > preserved method as well
> > > So I will add it there
> > >
> > > > I'm not sure it's a good idea for dumpTableSchema() to leave out
> > > > specifying the compression method if it happens to be pglz. I think we
> > > > definitely shouldn't do it in binary-upgrade mode. What if we changed
> > > > the default in a future release? For that matter, even 0002 could make
> > > > the current approach unsafe.... I think, anyway.
> > >
> > > Fixed
> > >
> > >
> > > > The changes to pg_dump.h look like they haven't had a visit from
> > > > pgindent. You should probably try to do that for the whole patch,
> > > > though it's a bit annoying since you'll have to manually remove
> > > > unrelated changes to the same files that are being modified by the
> > > > patch. Also, why the extra blank line here?
> > >
> > > Fixed, ran pgindent for other files as well.
> > >
> > > > GetAttributeCompression() is hard to understand. I suggest changing
> > > > the comment to "resolve column compression specification to an OID"
> > > > and somehow rejigger the code so that you aren't using one not-NULL
> > > > test and one NULL test on the same variable. Like maybe change the
> > > > first part to if (!IsStorageCompressible(typstorage)) { if
> > > > (compression == NULL) return InvalidOid; ereport(ERROR, ...); }
> > >
> > > Done
> > >
> > > > It puzzles me that CompareCompressionMethodAndDecompress() calls
> > > > slot_getallattrs() just before clearing the slot. It seems like this
> > > > ought to happen before we loop over the attributes, so that we don't
> > > > need to call slot_getattr() every time. See the comment for that
> > > > function. But even if we didn't do that for some reason, why would we
> > > > do it here? If it's already been done, it shouldn't do anything, and
> > > > if it hasn't been done, it might overwrite some of the values we just
> > > > poked into tts_values. It also seems suspicious that we can get away
> > > > with clearing the slot and then again marking it valid. I'm not sure
> > > > it really works like that. Like, can't clearing the slot invalidate
> > > > pointers stored in tts_values[]? For instance, if they are pointers
> > > > into an in-memory heap tuple, tts_heap_clear() is going to free the
> > > > tuple; if they are pointers into a buffer, tts_buffer_heap_clear() is
> > > > going to unpin it. I think the supported procedure for this sort of
> > > > thing is to have a second slot, set tts_values, tts_isnull etc. and
> > > > then materialize the slot. After materializing the new slot, it's
> > > > independent of the old slot, which can then be cleared. See for
> > > > example tts_virtual_materialize(). The whole approach you've taken
> > > > here might need to be rethought a bit. I think you are right to want
> > > > to avoid copying everything over into a new slot if nothing needs to
> > > > be done, and I think we should definitely keep that optimization, but
> > > > I think if you need to copy stuff, you have to do the above procedure
> > > > and then continue using the other slot instead of the original one.
> > > > Some places I think we have functions that return either the original
> > > > slot or a different one depending on how it goes; that might be a
> > > > useful idea here. But, you also can't just spam-create slots; it's
> > > > important that whatever ones we end up with get reused for every
> > > > tuple.
> > >
> > > I have changed this algorithm, so now if we have to decompress
> > > anything we will use the new slot and we will stick that new slot to
> > > the ModifyTableState, DR_transientrel for matviews and DR_intorel for
> > > CTAS. Does this looks okay or we need to do something else? If this
> > > logic looks fine then maybe we can think of some more optimization and
> > > cleanup in this function.
> > >
> > >
> > > > Doesn't the change to describeOneTableDetails() require declaring
> > > > changing the declaration of char *headers[11] to char *headers[12]?
> > > > How does this not fail Assert(cols <= lengthof(headers))?
> > >
> > > Fixed
> > >
> > > > Why does describeOneTableDetais() arrange to truncate the printed
> > > > value? We don't seem to do that for the other column properties, and
> > > > it's not like this one is particularly long.
> > >
> > > Not required, fixed.
> > >
> > > > Perhaps the changes to pg_am.dat shouldn't remove the blank line?
> > >
> > > Fixed
> > >
> > > > I think the comment to pg_attribute.h could be rephrased to stay
> > > > something like: "OID of compression AM. Must be InvalidOid if and only
> > > > if typstorage is 'a' or 'b'," replacing 'a' and 'b' with whatever the
> > > > right letters are. This would be shorter and I think also clearer than
> > > > what you have
> > >
> > > Fixed
> > >
> > > > The first comment change in postgres.h is wrong. You changed
> > > > va_extsize to "size in va_extinfo" but the associated structure
> > > > definition is unchanged, so the comment shouldn't be changed either.
> > >
> > > Yup, not required.
> > >
> > > > In toast_internals.h, you end using 30 as a constant several times but
> > > > have no #define for it. You do have a #define for RAWSIZEMASK, but
> > > > that's really a derived value from 30. Also, it's not a great name
> > > > because it's kind of generic. So how about something like:
> > > >
> > > > #define TOAST_RAWSIZE_BITS 30
> > > > #define TOAST_RAWSIZE_MASK ((1 << (TOAST_RAW_SIZE_BITS + 1)) - 1)
> > > >
> > > > But then again on second thought, this 30 seems to be the same 30 that
> > > > shows up in the changes to postgres.h, and there again 0x3FFFFFFF
> > > > shows up too. So maybe we should actually be defining these constants
> > > > there, using names like VARLENA_RAWSIZE_BITS and VARLENA_RAWSIZE_MASK
> > > > and then having toast_internals.h use those constants as well.
> > >
> > > Done, IMHO it should be
> > > #define VARLENA_RAWSIZE_BITS 30
> > > #define VARLENA_RAWSIZE_MASK ((1 << VARLENA_RAWSIZE_BITS) -1 )
> > >
> > >
> > > > Taken with the email I sent yesterday, I think this is a more or less
> > > > complete review of 0001. Although there are a bunch of things to fix
> > > > here still, I don't think this is that far from being committable. I
> > > > don't at this point see too much in terms of big design problems.
> > > > Probably the CompareCompressionMethodAndDecompress() is the closest to
> > > > a design-level problem, and certainly something needs to be done about
> > > > it, but even that is a fairly localized problem in the context of the
> > > > entire patch.
> > >
> > > 0001 is attached, now pending parts are
> > >
> > > - Confirm the new design of CompareCompressionMethodAndDecompress
> > > - Performance test, especially lz4 with small varlena
> >
> > I have tested the performance, pglz vs lz4
> >
> > Test1: With a small simple string, pglz doesn't select compression but
> > lz4 select as no min limit
> > Table: 100 varchar column
> > Test: Insert 1000 tuple, each column of 25 bytes string (32 is min
> > limit for pglz)
> > Result:
> > pglz: 1030 ms (doesn't attempt compression so externalize),
> > lz4: 212 ms
> >
> > Test2: With small incompressible string, pglz don't select compression
> > lz4 select but can not compress
> > Table: 100 varchar column
> > Test: Insert 1000 tuple, each column of 25 bytes string (32 is min
> > limit for pglz)
> > Result:
> > pglz: 1030 ms (doesn't attempt compression so externalize),
> > lz4: 1090 ms (attempt to compress but externalize):
> >
> > Test3: Test a few columns with large random data
> > Table: 3 varchar column
> > Test: Insert 1000 tuple 3 columns size(3500 byes, 4200 bytes, 4900bytes)
> > pglz: 150 ms (compression ratio: 3.02%),
> > lz4: 30 ms (compression ratio : 2.3%)
> >
> > Test4: Test3 with different large random slighly compressible, need to
> > compress + externalize:
> > Table: 3 varchar column
> > Insert: Insert 1000 tuple 3 columns size(8192, 8192, 8192)
> > CREATE OR REPLACE FUNCTION large_val() RETURNS TEXT LANGUAGE SQL AS
> > 'select array_agg(md5(g::text))::text from generate_series(1, 256) g';
> > Test: insert into t1 select large_val(), large_val(), large_val() from
> > generate_series(1,1000);
> > pglz: 2000 ms
> > lz4: 1500 ms
> >
> > Conclusion:
> > 1. In most cases lz4 is faster and doing better compression as well.
> > 2. In Test2 when small data is incompressible then lz4 tries to
> > compress whereas pglz doesn't try so there is some performance loss.
> > But if we want we can fix
> > it by setting some minimum limit of size for lz4 as well, maybe the
> > same size as pglz?
> >
> > > - Rebase other patches atop this patch
> > > - comment in ddl.sgml
> >
> > Other changes in patch:
> > - Now we are dumping the default compression method in the
> > binary-upgrade mode so the pg_dump test needed some change, fixed
> > that.
> > - in compress_pglz.c and compress_lz4.c, we were using
> > toast_internal.h macros so I removed and used varlena macros instead.
> >
> > While testing, I noticed that if the compressed data are externalized
> > then pg_column_compression(), doesn't fetch the compression method
> > from the toast chunk, I think we should do that. I will analyze this
> > and fix it in the next version.
>
> While trying to fix this, I have realized this problem exists in
> CompareCompressionMethodAndDecompress
> see below code.
> ---
> + new_value = (struct varlena *)
> + DatumGetPointer(slot->tts_values[attnum - 1]);
> +
> + /* nothing to be done, if it is not compressed */
> + if (!VARATT_IS_COMPRESSED(new_value))
> + continue;
> ---
>
> Basically, we are just checking whether the stored value is compressed
> or not, but we are clearly ignoring the fact that it might be
> compressed and stored externally on disk. So basically if the value
> is stored externally we can not know whether the external data were
> compressed or not without fetching the values from the toast table, I
> think instead of fetching the complete data from toast we can just
> fetch the header using 'toast_fetch_datum_slice'.
>
> Any other thoughts on this?

I think I was partially wrong here. Basically, there is a way to know
whether the external data are compressed or not using
VARATT_EXTERNAL_IS_COMPRESSED macro. However, if it is compressed
then we will have to fetch the toast slice of size
toast_compress_header, to know the compression method.

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

Please remember to trim unnecessary quoted material.

On Sun, Feb 7, 2021 at 6:45 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> [ a whole lot of quoted stuff ]
>
> I have tested the performance, pglz vs lz4
>
> Test1: With a small simple string, pglz doesn't select compression but
> lz4 select as no min limit
> Table: 100 varchar column
> Test: Insert 1000 tuple, each column of 25 bytes string (32 is min
> limit for pglz)
> Result:
> pglz: 1030 ms (doesn't attempt compression so externalize),
> lz4: 212 ms
>
> Test2: With small incompressible string, pglz don't select compression
> lz4 select but can not compress
> Table: 100 varchar column
> Test: Insert 1000 tuple, each column of 25 bytes string (32 is min
> limit for pglz)
> Result:
> pglz: 1030 ms (doesn't attempt compression so externalize),
> lz4: 1090 ms (attempt to compress but externalize):
>
> Test3: Test a few columns with large random data
> Table: 3 varchar column
> Test: Insert 1000 tuple 3 columns size(3500 byes, 4200 bytes, 4900bytes)
> pglz: 150 ms (compression ratio: 3.02%),
> lz4: 30 ms (compression ratio : 2.3%)
>
> Test4: Test3 with different large random slighly compressible, need to
> compress + externalize:
> Table: 3 varchar column
> Insert: Insert 1000 tuple 3 columns size(8192, 8192, 8192)
> CREATE OR REPLACE FUNCTION large_val() RETURNS TEXT LANGUAGE SQL AS
> 'select array_agg(md5(g::text))::text from generate_series(1, 256) g';
> Test: insert into t1 select large_val(), large_val(), large_val() from
> generate_series(1,1000);
> pglz: 2000 ms
> lz4: 1500 ms
>
> Conclusion:
> 1. In most cases lz4 is faster and doing better compression as well.
> 2. In Test2 when small data is incompressible then lz4 tries to
> compress whereas pglz doesn't try so there is some performance loss.
> But if we want we can fix
> it by setting some minimum limit of size for lz4 as well, maybe the
> same size as pglz?

So my conclusion here is that perhaps there's no real problem. It
looks like externalizing is so expensive compared to compression that
it's worth trying to compress even though it may not always pay off.
If, by trying to compress, we avoid externalizing, it's a huge win
(~5x). If we try to compress and don't manage to avoid externalizing,
it's a small loss (~6%). It's probably reasonable to expect that
compressible data is more common than incompressible data, so not only
is the win a lot bigger than the loss, but we should be able to expect
it to happen a lot more often. It's not impossible that somebody could
get bitten, but it doesn't feel like a huge risk to me.

One thing that does occur to me is that it might be a good idea to
skip compression if it doesn't change the number of chunks that will
be stored into the TOAST table. If we compress the value but still
need to externalize it, and the compression didn't save enough to
reduce the number of chunks, I suppose we ideally would externalize
the uncompressed version. That would save decompression time later,
without really costing anything. However, I suppose that would be a
separate improvement from this patch. Maybe the possibility of
compressing smaller values makes it slightly more important, but I'm
not sure that it's worth getting excited about.

If anyone feels otherwise on either point, it'd be good to hear about it.

--
Robert Haas
EDB: http://www.enterprisedb.com

I see the thread got broken somehow (or cfbot thought it did), so I added the
new thread, and this is now passing all tests. (I think using the v22
patches). http://cfbot.cputube.org/dilip-kumar.html

On Fri, Feb 05, 2021 at 11:07:53AM -0500, Robert Haas wrote:
> > Also, I think we may want to make enable-lz4 the default *for testing
> > purposes*, now that the linux and BSD environments include that.
>
> My guess was that would annoy some hackers whose build environments
> got broken. If everyone thinks otherwise I'm willing to be persuaded,
> but it's going to take more than 1 vote...

I think you misunderstood: I mean that the WIP patch should default to
--enable-lz4, to exercise on a few CI. It's hardly useful to run CI with the
feature disabled. I assume that the patch would be committed with default
--disable-lz4.

On Fri, Feb 05, 2021 at 11:23:46AM -0500, Robert Haas wrote:
> On Fri, Feb 5, 2021 at 11:07 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > Personally, my preference is to just update the test outputs. It's not
> > important whether many people look closely to verify the differences;
> > we just need to look them over on a one-time basis to see if they seem
> > OK. After that it's 0 effort, vs. having to maintain HIDE_COMPRESSAM
> > forever.
>
> Oh, I guess you're thinking about the case where someone wants to run
> the tests with a different default. That might be a good reason to
> have this. But then those changes should go in 0002.

Right, it's not one-time, it's also whenever setting a non-default compression
method. I say it should go into 0001 to avoid a whole bunch of churn in
src/test/regress, and then more churn (and rebase conflicts in other patches)
while adding HIDE_COMPRESSAM in 0002.

--
Justin

On Tue, Feb 9, 2021 at 3:37 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> I think you misunderstood: I mean that the WIP patch should default to
> --enable-lz4, to exercise on a few CI. It's hardly useful to run CI with the
> feature disabled. I assume that the patch would be committed with default
> --disable-lz4.

Oh, I see. I guess we could do that.

> Right, it's not one-time, it's also whenever setting a non-default compression
> method. I say it should go into 0001 to avoid a whole bunch of churn in
> src/test/regress, and then more churn (and rebase conflicts in other patches)
> while adding HIDE_COMPRESSAM in 0002.

Hmm, I guess that makes some sense, too.

I'm not sure either one is completely critical, but it does make sense
to me now.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Feb 10, 2021 at 1:42 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> Please remember to trim unnecessary quoted material.

Okay, I will.

> On Sun, Feb 7, 2021 at 6:45 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > [ a whole lot of quoted stuff ]
> >
> > Conclusion:
> > 1. In most cases lz4 is faster and doing better compression as well.
> > 2. In Test2 when small data is incompressible then lz4 tries to
> > compress whereas pglz doesn't try so there is some performance loss.
> > But if we want we can fix
> > it by setting some minimum limit of size for lz4 as well, maybe the
> > same size as pglz?
>
> So my conclusion here is that perhaps there's no real problem. It
> looks like externalizing is so expensive compared to compression that
> it's worth trying to compress even though it may not always pay off.
> If, by trying to compress, we avoid externalizing, it's a huge win
> (~5x). If we try to compress and don't manage to avoid externalizing,
> it's a small loss (~6%). It's probably reasonable to expect that
> compressible data is more common than incompressible data, so not only
> is the win a lot bigger than the loss, but we should be able to expect
> it to happen a lot more often. It's not impossible that somebody could
> get bitten, but it doesn't feel like a huge risk to me.

I agree with this. That said maybe we could test the performance of
pglz also by lowering/removing the min compression limit but maybe
that should be an independent change.

> One thing that does occur to me is that it might be a good idea to
> skip compression if it doesn't change the number of chunks that will
> be stored into the TOAST table. If we compress the value but still
> need to externalize it, and the compression didn't save enough to
> reduce the number of chunks, I suppose we ideally would externalize
> the uncompressed version. That would save decompression time later,
> without really costing anything. However, I suppose that would be a
> separate improvement from this patch.

Yeah, this seems like a good idea and we can work on that in a different thread.

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

On Tue, Feb 9, 2021 at 6:14 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> > Basically, we are just checking whether the stored value is compressed
> > or not, but we are clearly ignoring the fact that it might be
> > compressed and stored externally on disk. So basically if the value
> > is stored externally we can not know whether the external data were
> > compressed or not without fetching the values from the toast table, I
> > think instead of fetching the complete data from toast we can just
> > fetch the header using 'toast_fetch_datum_slice'.
> >
> > Any other thoughts on this?
>
> I think I was partially wrong here. Basically, there is a way to know
> whether the external data are compressed or not using
> VARATT_EXTERNAL_IS_COMPRESSED macro. However, if it is compressed
> then we will have to fetch the toast slice of size
> toast_compress_header, to know the compression method.

I have fixed this issue, so now we will be able to detect the
compression method of the externalized compressed data as well. I
have also added a test case for this. I have rebased other patches
also on top of this patch. I have fixed the doc compilation issue in
patch 0004 raised by Justin. I still could not figure out what is the
right place for inheriting the compression method related change in
the "ddl.sgml" so that is still there, any suggestions on that?

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

On Wed, Feb 10, 2021 at 9:52 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> [ new patches ]

I think that in both varattrib_4b and toast_internals.h it would be
better to pick a less generic field name. In toast_internals.h it's
just info; in postgres.h it's va_info. But:

[rhaas pgsql]$ git grep info | wc -l
24552

There are no references in the current source tree to va_info, so at
least that one is greppable, but it's still not very descriptive. I
suggest info -> tcinfo and va_info -> va_tcinfo, where "tc" stands for
"TOAST compression". Looking through 24552 references to info to find
the ones that pertain to this feature might take longer than searching
the somewhat shorter list of references to tcinfo, which prepatch is
just:

[rhaas pgsql]$ git grep tcinfo | wc -l
0

I don't see why we should allow for datum_decompress to be optional,
as toast_decompress_datum_slice does. Likely every serious compression
method will support that anyway. If not, the compression AM can deal
with the problem, rather than having the core code do it. That will
save some tiny amount of performance, too.

src/backend/access/compression/Makefile is missing a copyright header.

It's really sad that lz4_cmdecompress_slice allocates
VARRAWSIZE_4B_C(value) + VARHDRSZ rather than slicelength + VARHDRSZ
as pglz_cmdecompress_slice() does. Is that a mistake, or is that
necessary for some reason? If it's a mistake, let's fix it. If it's
necessary, let's add a comment about why, probably starting with
"Unfortunately, ....".

I think you have a fairly big problem with row types. Consider this example:

create table t1 (a int, b text compression pglz);
create table t2 (a int, b text compression lz4);
create table t3 (x t1);
insert into t1 values (1, repeat('foo', 1000));
insert into t2 values (1, repeat('foo', 1000));
insert into t3 select t1 from t1;
insert into t3 select row(a, b)::t1 from t2;

rhaas=# select pg_column_compression((t3.x).b) from t3;
pg_column_compression
-----------------------
pglz
lz4
(2 rows)

That's not good, because now

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Feb 10, 2021 at 3:06 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I think you have a fairly big problem with row types. Consider this example:
>
> create table t1 (a int, b text compression pglz);
> create table t2 (a int, b text compression lz4);
> create table t3 (x t1);
> insert into t1 values (1, repeat('foo', 1000));
> insert into t2 values (1, repeat('foo', 1000));
> insert into t3 select t1 from t1;
> insert into t3 select row(a, b)::t1 from t2;
>
> rhaas=# select pg_column_compression((t3.x).b) from t3;
> pg_column_compression
> -----------------------
> pglz
> lz4
> (2 rows)
>
> That's not good, because now

...because now I hit send too soon. Also, because now column b has
implicit dependencies on both compression AMs and the rest of the
system has no idea. I think we probably should have a rule that
nothing except pglz is allowed inside of a record, just to keep it
simple. The record overall can be toasted so it's not clear why we
should also be toasting the original columns at all, but I think
precedent probably argues for continuing to allow PGLZ, as it can
already be like that on disk and pg_upgrade is a thing. The same kind
of issue probably exists for arrays and range types.

I poked around a bit trying to find ways of getting data compressed
with one compression method into a column that was marked with another
compression method, but wasn't able to break it.

In CompareCompressionMethodAndDecompress, I think this is still
playing a bit fast and loose with the rules around slots. I think we
can do better. Suppose that at the point where we discover that we
need to decompress at least one attribute, we create the new slot
right then, and also memcpy tts_values and tts_isnull. Then, for that
attribute and any future attributes that need decompression, we reset
tts_values in the *new* slot, leaving the old one untouched. Then,
after finishing all the attributes, the if (decompressed_any) block,
you just have a lot less stuff to do. The advantage of this is that
you haven't tainted the old slot; it's still got whatever contents it
had before, and is in a clean state, which seems better to me.

It's unclear to me whether this function actually needs to
ExecMaterializeSlot(newslot). It definitely does need to
ExecStoreVirtualTuple(newslot) and I think it's a very good idea, if
not absolutely mandatory, for it not to modify anything about the old
slot. But what's the argument that the new slot needs to be
materialized at this point? It may be needed, if the old slot would've
had to be materialized at this point. But it's something to think
about.

The CREATE TABLE documentation says that COMPRESSION is a kind of
column constraint, but that's wrong. For example, you can't write
CREATE TABLE a (b int4 CONSTRAINT thunk COMPRESSION lz4), for example,
contrary to what the syntax summary implies. When you fix this so that
the documentation matches the grammar change, you may also need to
move the longer description further up in create_table.sgml so the
order matches.

The use of VARHDRSZ_COMPRESS in toast_get_compression_oid() appears to
be incorrect. VARHDRSZ_COMPRESS is offsetof(varattrib_4b,
va_compressed.va_data). But what gets externalized in the case of a
compressed datum is just VARDATA(dval), which excludes the length
word, unlike VARHDRSZ_COMPRESS, which does not. This has no
consequences since we're only going to fetch 1 chunk either way, but I
think we should make it correct.

TOAST_COMPRESS_SET_SIZE_AND_METHOD() could Assert something about cm_method.

Small delta patch with a few other suggested changes attached.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Feb 11, 2021 at 3:26 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Wed, Feb 10, 2021 at 3:06 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > I think you have a fairly big problem with row types. Consider this example:
> >
> > create table t1 (a int, b text compression pglz);
> > create table t2 (a int, b text compression lz4);
> > create table t3 (x t1);
> > insert into t1 values (1, repeat('foo', 1000));
> > insert into t2 values (1, repeat('foo', 1000));
> > insert into t3 select t1 from t1;
> > insert into t3 select row(a, b)::t1 from t2;
> >
> > rhaas=# select pg_column_compression((t3.x).b) from t3;
> > pg_column_compression
> > -----------------------
> > pglz
> > lz4
> > (2 rows)
> >
> > That's not good, because now

Yeah, that's really bad.

> ...because now I hit send too soon. Also, because now column b has
> implicit dependencies on both compression AMs and the rest of the
> system has no idea. I think we probably should have a rule that
> nothing except pglz is allowed inside of a record, just to keep it
> simple. The record overall can be toasted so it's not clear why we
> should also be toasting the original columns at all, but I think
> precedent probably argues for continuing to allow PGLZ, as it can
> already be like that on disk and pg_upgrade is a thing. The same kind
> of issue probably exists for arrays and range types.

While constructing a row type from the tuple we flatten the external
data I think that would be the place to decompress the data if they
are not compressed with PGLZ. For array-type, we are already
detoasting/decompressing the source attribute see "construct_md_array"
so the array type doesn't have this problem. I haven't yet checked
the range type. Based on my analysis it appeared that the different
data types are getting constructed at different paths so maybe we
should find some centralized place or we need to make some function
call in all such places so that we can decompress the attribute if
required before forming the tuple for the composite type.

I have quickly hacked the code and after that, your test case is working fine.

postgres[55841]=# select pg_column_compression((t3.x).b) from t3;
pg_column_compression
-----------------------
pglz

(2 rows)

-> now the attribute 'b' inside the second tuple is decompressed
(because it was not compressed with PGLZ) so the compression method of
b is NULL

postgres[55841]=# select pg_column_compression((t3.x)) from t3;
pg_column_compression
-----------------------

pglz
(2 rows)

--> but the second row itself is compressed back using the local
compression method of t3

W.R.T the attached patch, In HeapTupleHeaderGetDatum, we don't even
attempt to detoast if there is no external field in the tuple, in POC
I have got rid of that check, but I think we might need to do better.
Maybe we can add a flag in infomask to detect whether the tuple has
any compressed data or not as we have for detecting the external data
(HEAP_HASEXTERNAL).

So I will do some more analysis in this area and try to come up with a
clean solution.

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

On Thu, Feb 11, 2021 at 7:36 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> W.R.T the attached patch, In HeapTupleHeaderGetDatum, we don't even
> attempt to detoast if there is no external field in the tuple, in POC
> I have got rid of that check, but I think we might need to do better.
> Maybe we can add a flag in infomask to detect whether the tuple has
> any compressed data or not as we have for detecting the external data
> (HEAP_HASEXTERNAL).

No. This feature isn't close to being important enough to justify
consuming an infomask bit.

I don't really see why we need it anyway. If array construction
already categorically detoasts, why can't we do the same thing here?
Would it really cost that much? In what case? Having compressed values
in a record we're going to store on disk actually seems like a pretty
dumb idea. We might end up trying to recompress something parts of
which have already been compressed.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Feb 11, 2021 at 8:17 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Thu, Feb 11, 2021 at 7:36 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > W.R.T the attached patch, In HeapTupleHeaderGetDatum, we don't even
> > attempt to detoast if there is no external field in the tuple, in POC
> > I have got rid of that check, but I think we might need to do better.
> > Maybe we can add a flag in infomask to detect whether the tuple has
> > any compressed data or not as we have for detecting the external data
> > (HEAP_HASEXTERNAL).
>
> No. This feature isn't close to being important enough to justify
> consuming an infomask bit.

Okay,

> I don't really see why we need it anyway. If array construction
> already categorically detoasts, why can't we do the same thing here?
> Would it really cost that much? In what case? Having compressed values
> in a record we're going to store on disk actually seems like a pretty
> dumb idea. We might end up trying to recompress something parts of
> which have already been compressed.
>

If we refer the comments atop function "toast_flatten_tuple_to_datum"

---------------
* We have a general rule that Datums of container types (rows, arrays,
* ranges, etc) must not contain any external TOAST pointers. Without
* this rule, we'd have to look inside each Datum when preparing a tuple
* for storage, which would be expensive and would fail to extend cleanly
* to new sorts of container types.
*
* However, we don't want to say that tuples represented as HeapTuples
* can't contain toasted fields, so instead this routine should be called
* when such a HeapTuple is being converted into a Datum.
*
* While we're at it, we decompress any compressed fields too. This is not
* necessary for correctness, but reflects an expectation that compression
* will be more effective if applied to the whole tuple not individual
* fields. We are not so concerned about that that we want to deconstruct
* and reconstruct tuples just to get rid of compressed fields, however.
* So callers typically won't call this unless they see that the tuple has
* at least one external field.
----------------

It appears that the general rule we want to follow is that while
creating the composite type we want to flatten any external pointer,
but while doing that we also decompress any compressed field with the
assumption that compressing the whole row/array will be a better idea
instead of keeping them compressed individually. However, if there
are no external toast pointers then we don't want to make an effort to
just decompress the compressed data.

Having said that I don't think this rule is followed throughout the
code for example

1. "ExecEvalRow" is calling HeapTupleHeaderGetDatum only if there is
any external field and which is calling "toast_flatten_tuple_to_datum"
so this is following the rule.
2. "ExecEvalWholeRowVar" is calling "toast_build_flattened_tuple", but
this is just flattening the external toast pointer but not doing
anything to the compressed data.
3. "ExecEvalArrayExpr" is calling "construct_md_array", which will
detoast the attribute if attlen is -1, so this will decompress any
compressed data even though there is no external toast pointer.

So in 1 we are following the rule but in 2 and 3 we are not.

IMHO, for the composite data types we should make common a rule and we
should follow that everywhere. As you said it will be good if we can
always detoast any external/compressed data, that will help in getting
better compression as well as fetching the data will be faster because
we can avoid multi level detoasting/decompression. I will analyse
this further and post a patch for the same.

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

On Wed, Feb 10, 2021 at 04:56:17PM -0500, Robert Haas wrote:
> Small delta patch with a few other suggested changes attached.

Robert's fixup patch caused the CI to fail, since it 1) was called *.patch;
and, 2) didn't include the previous patches.

This includes a couple proposals of mine as separate patches.

. psql: Add HIDE_COMPRESSAM for regress testing; I'm proposing this as part of
0001 (I excluded the reression test changes from 0001 which would've been
reverted by this patch).
. Add default_toast_compression GUC with ideas from Robert; I'm proposing
something like this as part of 0001. Actually, the full complexity with syscache
lookups and invalidation aren't needed in 0001, but are needed in the
"Custom compression" patch.
. default --enable-lz4 (this not meant to be merged so you could leave this as
a separate patch)

I'm not including the whole patch series, since this patch is failing
pg_restore tests:
|Create custom compression methods

--
Justin

On Sun, Feb 14, 2021 at 12:49:40PM -0600, Justin Pryzby wrote:
> On Wed, Feb 10, 2021 at 04:56:17PM -0500, Robert Haas wrote:
> > Small delta patch with a few other suggested changes attached.
>
> Robert's fixup patch caused the CI to fail, since it 1) was called *.patch;
> and, 2) didn't include the previous patches.
>
> This includes a couple proposals of mine as separate patches.

CIs failed on BSD and linux due to a test in contrib/, but others passed.
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.127551
https://cirrus-ci.com/task/6087701947482112
https://cirrus-ci.com/task/6650651900903424
https://cirrus-ci.com/task/5524751994060800

Resending with fixes to configure.ac and missed autoconf run. I think this is
expected to fail on mac, due to missing LZ4.

BTW, compressamapi.h doesn't need to be included in any of these, at least in
the 0001 patch:

src/backend/access/common/indextuple.c | 2 +-
src/backend/catalog/heap.c | 2 +-
src/backend/catalog/index.c | 2 +-
src/backend/parser/parse_utilcmd.c | 2 +-

It's pretty unfriendly that this requires quoting the integer to be
syntactically valid:

|postgres=# create table j(q text compression pglz with (level 1) );
|2021-01-30 01:26:33.554 CST [31814] ERROR: syntax error at or near "1" at character 52
|2021-01-30 01:26:33.554 CST [31814] STATEMENT: create table j(q text compression pglz with (level 1) );
|ERROR: syntax error at or near "1"
|LINE 1: create table j(q text compression pglz with (level 1) );

On Mon, Feb 15, 2021 at 1:58 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> On Sun, Feb 14, 2021 at 12:49:40PM -0600, Justin Pryzby wrote:
> > On Wed, Feb 10, 2021 at 04:56:17PM -0500, Robert Haas wrote:
> > > Small delta patch with a few other suggested changes attached.
> >
> > Robert's fixup patch caused the CI to fail, since it 1) was called *.patch;
> > and, 2) didn't include the previous patches.
> >
> > This includes a couple proposals of mine as separate patches.
>
> CIs failed on BSD and linux due to a test in contrib/, but others passed.
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.127551
> https://cirrus-ci.com/task/6087701947482112
> https://cirrus-ci.com/task/6650651900903424
> https://cirrus-ci.com/task/5524751994060800
>
> Resending with fixes to configure.ac and missed autoconf run. I think this is
> expected to fail on mac, due to missing LZ4.
>
> BTW, compressamapi.h doesn't need to be included in any of these, at least in
> the 0001 patch:
>
> src/backend/access/common/indextuple.c | 2 +-
> src/backend/catalog/heap.c | 2 +-
> src/backend/catalog/index.c | 2 +-
> src/backend/parser/parse_utilcmd.c | 2 +-
>
> It's pretty unfriendly that this requires quoting the integer to be
> syntactically valid:
>
> |postgres=# create table j(q text compression pglz with (level 1) );
> |2021-01-30 01:26:33.554 CST [31814] ERROR: syntax error at or near "1" at character 52
> |2021-01-30 01:26:33.554 CST [31814] STATEMENT: create table j(q text compression pglz with (level 1) );
> |ERROR: syntax error at or near "1"
> |LINE 1: create table j(q text compression pglz with (level 1) );

Thanks for the review and patch for HIDE_COMPRESSAM, I will merge
this into the main patch. And work on other comments after fixing the
issue related to compressed data in composite types.

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

On Sat, Feb 13, 2021 at 8:14 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Thu, Feb 11, 2021 at 8:17 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >
> > On Thu, Feb 11, 2021 at 7:36 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > W.R.T the attached patch, In HeapTupleHeaderGetDatum, we don't even
> > > attempt to detoast if there is no external field in the tuple, in POC
> > > I have got rid of that check, but I think we might need to do better.
> > > Maybe we can add a flag in infomask to detect whether the tuple has
> > > any compressed data or not as we have for detecting the external data
> > > (HEAP_HASEXTERNAL).
> >
> > No. This feature isn't close to being important enough to justify
> > consuming an infomask bit.
>
> Okay,
>
> > I don't really see why we need it anyway. If array construction
> > already categorically detoasts, why can't we do the same thing here?
> > Would it really cost that much? In what case? Having compressed values
> > in a record we're going to store on disk actually seems like a pretty
> > dumb idea. We might end up trying to recompress something parts of
> > which have already been compressed.
> >
>
> If we refer the comments atop function "toast_flatten_tuple_to_datum"
>
> ---------------
> * We have a general rule that Datums of container types (rows, arrays,
> * ranges, etc) must not contain any external TOAST pointers. Without
> * this rule, we'd have to look inside each Datum when preparing a tuple
> * for storage, which would be expensive and would fail to extend cleanly
> * to new sorts of container types.
> *
> * However, we don't want to say that tuples represented as HeapTuples
> * can't contain toasted fields, so instead this routine should be called
> * when such a HeapTuple is being converted into a Datum.
> *
> * While we're at it, we decompress any compressed fields too. This is not
> * necessary for correctness, but reflects an expectation that compression
> * will be more effective if applied to the whole tuple not individual
> * fields. We are not so concerned about that that we want to deconstruct
> * and reconstruct tuples just to get rid of compressed fields, however.
> * So callers typically won't call this unless they see that the tuple has
> * at least one external field.
> ----------------
>
> It appears that the general rule we want to follow is that while
> creating the composite type we want to flatten any external pointer,
> but while doing that we also decompress any compressed field with the
> assumption that compressing the whole row/array will be a better idea
> instead of keeping them compressed individually. However, if there
> are no external toast pointers then we don't want to make an effort to
> just decompress the compressed data.
>
> Having said that I don't think this rule is followed throughout the
> code for example
>
> 1. "ExecEvalRow" is calling HeapTupleHeaderGetDatum only if there is
> any external field and which is calling "toast_flatten_tuple_to_datum"
> so this is following the rule.
> 2. "ExecEvalWholeRowVar" is calling "toast_build_flattened_tuple", but
> this is just flattening the external toast pointer but not doing
> anything to the compressed data.
> 3. "ExecEvalArrayExpr" is calling "construct_md_array", which will
> detoast the attribute if attlen is -1, so this will decompress any
> compressed data even though there is no external toast pointer.
>
> So in 1 we are following the rule but in 2 and 3 we are not.
>
> IMHO, for the composite data types we should make common a rule and we
> should follow that everywhere. As you said it will be good if we can
> always detoast any external/compressed data, that will help in getting
> better compression as well as fetching the data will be faster because
> we can avoid multi level detoasting/decompression. I will analyse
> this further and post a patch for the same.

I have done further analysis of this issue and came up with the
attached patch. So with this patch, like external toast posiners we
will not allow any compressed data also in the composite types. The
problem is that now we will be processing all the tuple while forming
the composite type irrespective of the source of the tuple, I mean if
user is directly inserting into the array type and not selecting from
another table then there will not be any compressed data so now
checking each field of tuple for compressed data is unnecessary but I
am not sure how to distinguish between those cases.

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

@cfbot: Resending with fixes to regression tests.

I'm hoping to see check-world pass --with-lz4 on an environment other than my
own PC.

On Fri, Feb 19, 2021 at 2:43 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:

I had an off list discussion with Robert and based on his suggestion
and a poc patch, I have come up with an updated version for handling
the composite type. Basically, the problem was that ExecEvalRow we
are first forming the tuple and then we are calling
HeapTupleHeaderGetDatum and then we again need to deform to find any
compressed data so that can cause huge performance penalty in all
unrelated paths which don't even contain any compressed data. So
Robert's idea was to check for the compressed/external data even
before forming the tuple. I have implemented that and I can see we
are not seeing any performance penalty.

Test setup:
----------------
create table t1 (f1 int, f2 text, f3 text, f4 text, f5 text, f6
text,f7 text, f8 text, f9 text);
create table t2 (f1 int, f2 text, f3 text, f4 text, f5 text, f6
text,f7 text, f8 text, f9 text);
create table t3(x t1);

pgbench custom script for all test:
------------------------------------------------
\set x random(1, 10000)
select row(f1,f2,f3,f4,f5,f6,f7,f8,f9)::t1 from t2 where f1=:x;

test1:
Objective: Just select on data and form row, data contain no
compressed/external (should not create regression on unrelated paths)
data: insert into t2 select i, repeat('f1',
10),repeat('f2',10),repeat('f3', 10),repeat('f4', 10),repeat('f5',
10),repeat('f6',10),repeat('f7', 10),repeat('f8', 10) from
generate_series(1,10000) as i;
Result(TPS): Head: 1509.79 Patch: 1509.67

test2: data contains 1 compressed filed no external data
data: insert into t2 select i, repeat('f2',
10),repeat('f3',10000),repeat('f3', 10),repeat('f5', 10),repeat('f6',
4000),repeat('f7',10),repeat('f8', 10),repeat('f9', 10) from
generate_series(1,10000) as i;
Result(TPS): Head: 1088.08 Patch: 1071.48

test4: data contains 1 compressed/1 external field
(alter table t2 alter COLUMN f2 set storage external;)
data: (insert into t2 select i, repeat('f2',
10000),repeat('f3',10000),repeat('f3', 10),repeat('f5',
10),repeat('f6', 4000),repeat('f7',10),repeat('f8', 10),repeat('f9',
10) from generate_series(1,10000) as i;)
Result(TPS): Head: 1459.28 Patch: 1459.37

test5: where head need not decompress but patch needs to:
data: insert into t2 select i, repeat('f2',
10),repeat('f3',6000),repeat('f34', 5000),repeat('f5',
10),repeat('f6', 4000),repeat('f7',10),repeat('f8', 10),repeat('f9',
10) from generate_series(1,10000) as I;
--pgbench script
\set x random(1, 10000)
insert into t3 select row(f1,f2,f3,f4,f5,f6,f7,f8,f9)::t1 from t2 where f1=:x;
Result(TPS): Head: 562.36 Patch: 469.91

Summary: It seems like in most of the unrelated cases we are not
creating any regression with the attached patch. There is only some
performance loss when there is only the compressed data in such cases
with the patch we have to decompress whereas in head we don't. But, I
think it is not a overall loss because eventually if we have to fetch
the data multiple time then with patch we just have to decompress once
as whole row is compressed whereas on head we have to decompress field
by field, so I don't think this can be considered as a regression.

I also had to put the handling in the extended record so that it can
decompress any compressed data in the extended record. I think I need
to put some more effort into cleaning up this code. I have put a very
localized fix in ER_get_flat_size, basically this will ignore the
ER_FLAG_HAVE_EXTERNAL flag and it will always process the record. I
think the handling might not be perfect but I posted it to get the
feedback on the idea.

Other changes:
- I have fixed other pending comments from Robert. I will reply to
individual comments in a separate mail.
- Merge HIDE_COMPRESSAM with 0001.

Pending work:
- Cleanup 0001, especially for extended records.
- Rebased other patches.
- Review default compression method guc from Justin

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

On Thu, Feb 11, 2021 at 1:37 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Wed, Feb 10, 2021 at 9:52 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > [ new patches ]
>
> I think that in both varattrib_4b and toast_internals.h it would be
> better to pick a less generic field name. In toast_internals.h it's
> just info; in postgres.h it's va_info. But:
>
> [rhaas pgsql]$ git grep info | wc -l
> 24552
>
> There are no references in the current source tree to va_info, so at
> least that one is greppable, but it's still not very descriptive. I
> suggest info -> tcinfo and va_info -> va_tcinfo, where "tc" stands for
> "TOAST compression". Looking through 24552 references to info to find
> the ones that pertain to this feature might take longer than searching
> the somewhat shorter list of references to tcinfo, which prepatch is
> just:
>
> [rhaas pgsql]$ git grep tcinfo | wc -l
> 0

Done as suggested

>
> I don't see why we should allow for datum_decompress to be optional,
> as toast_decompress_datum_slice does. Likely every serious compression
> method will support that anyway. If not, the compression AM can deal
> with the problem, rather than having the core code do it. That will
> save some tiny amount of performance, too.

Done

> src/backend/access/compression/Makefile is missing a copyright header.

Fixed

> It's really sad that lz4_cmdecompress_slice allocates
> VARRAWSIZE_4B_C(value) + VARHDRSZ rather than slicelength + VARHDRSZ
> as pglz_cmdecompress_slice() does. Is that a mistake, or is that
> necessary for some reason? If it's a mistake, let's fix it. If it's
> necessary, let's add a comment about why, probably starting with
> "Unfortunately, ....".

In older versions of the lz4 there was a problem that the decompressed
data size could be bigger than the slicelength which is resolved now
so we can allocate slicelength + VARHDRSZ, I have fixed it.

Please refer the latest patch at
https://www.postgresql.org/message-id/CAFiTN-u2pyXDDDwZXJ-fVUwbLhJSe9TbrVR6rfW_rhdyL1A5bg%40mail.gmail.com

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

On Thu, Feb 11, 2021 at 3:26 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> In CompareCompressionMethodAndDecompress, I think this is still
> playing a bit fast and loose with the rules around slots. I think we
> can do better. Suppose that at the point where we discover that we
> need to decompress at least one attribute, we create the new slot
> right then, and also memcpy tts_values and tts_isnull. Then, for that
> attribute and any future attributes that need decompression, we reset
> tts_values in the *new* slot, leaving the old one untouched. Then,
> after finishing all the attributes, the if (decompressed_any) block,
> you just have a lot less stuff to do. The advantage of this is that
> you haven't tainted the old slot; it's still got whatever contents it
> had before, and is in a clean state, which seems better to me.

Fixed

>
> It's unclear to me whether this function actually needs to
> ExecMaterializeSlot(newslot). It definitely does need to
> ExecStoreVirtualTuple(newslot) and I think it's a very good idea, if
> not absolutely mandatory, for it not to modify anything about the old
> slot. But what's the argument that the new slot needs to be
> materialized at this point? It may be needed, if the old slot would've
> had to be materialized at this point. But it's something to think
> about.

I think if the original slot was materialized then materialing the new
slot make more sense to me so done that way.

>
> The CREATE TABLE documentation says that COMPRESSION is a kind of
> column constraint, but that's wrong. For example, you can't write
> CREATE TABLE a (b int4 CONSTRAINT thunk COMPRESSION lz4), for example,
> contrary to what the syntax summary implies. When you fix this so that
> the documentation matches the grammar change, you may also need to
> move the longer description further up in create_table.sgml so the
> order matches.

Fixed

> The use of VARHDRSZ_COMPRESS in toast_get_compression_oid() appears to
> be incorrect. VARHDRSZ_COMPRESS is offsetof(varattrib_4b,
> va_compressed.va_data). But what gets externalized in the case of a
> compressed datum is just VARDATA(dval), which excludes the length
> word, unlike VARHDRSZ_COMPRESS, which does not. This has no
> consequences since we're only going to fetch 1 chunk either way, but I
> think we should make it correct.

Fixed

> TOAST_COMPRESS_SET_SIZE_AND_METHOD() could Assert something about cm_method.

While replying to the comments, I realised that I have missed it. I
will fix it in the next version.

> Small delta patch with a few other suggested changes attached.

Merged

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

On Fri, Feb 19, 2021 at 11:12 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> I had an off list discussion with Robert and based on his suggestion
> and a poc patch, I have come up with an updated version for handling
> the composite type. Basically, the problem was that ExecEvalRow we
> are first forming the tuple and then we are calling
> HeapTupleHeaderGetDatum and then we again need to deform to find any
> compressed data so that can cause huge performance penalty in all
> unrelated paths which don't even contain any compressed data. So
> Robert's idea was to check for the compressed/external data even
> before forming the tuple. I have implemented that and I can see we
> are not seeing any performance penalty.

I think that these performance tests aren't really exercising the
expanded-record stuff, just the ExecEvalRow changes. We need to test
that test case, and I tend to suspect there's going to be a measurable
regression.

I spent some time looking at how datums get into the expanded record
system. There seem to be four possible paths:
expanded_record_set_tuple(), make_expanded_record_from_datum(),
expanded_record_set_field_internal(), and
expanded_record_set_fields(). The first two of these inject an entire
tuple, while the latter two work on a field-by-field basis. For that
reason, the latter two are not really problematic. I'm not quite sure
what the right thing to do is here, but if we wanted to check whether
a Datum that we're absorbing is non-pglz-compressed in those places,
it would be easy to do. Also, as far as I can see,
make_expanded_record_from_datum() is completely unused. So the problem
case is where expanded_record_set_tuple() is getting called, and
specifically where it's being called with expand_external = true. Any
place that it's being called with expand_external = false, there's
apparently no problem with the result tuple containing external
datums, so probably non-pglz compressed data is OK there too.

All of the places that can potentially pass expand_external = true are
actually passing !estate->atomic, where estate is a PLpgSQL_execstate.
In other words, I think the case where this happens is when we're in a
context where the computed value could need to survive across a COMMIT
or ROLLBACK, like there may be a procedure running (maybe more than
one, each invoking the next via CALL) but there are no queries in
progress. We have to expand TOAST references because committing a
transaction that deleted data means you might not be able to resolve
the old TOAST pointer any more: even if you use a snapshot that can
see everything, VACUUM could nuke the deleted rows - or some of them -
at any time. To avoid trouble we have to un-externalize before any
COMMIT or ROLLBACK occurs. That can suck for performance because we
might be fetching a big value that we don't end up using for anything
- say if the variable isn't used again - but it beats failing.

The argument that we need to force decompression in such cases is
considerably more tenuous. It revolves around the possibility that the
compression AM itself has been dropped. As long as we have only
built-in compression methods, which are undroppable, it seems like we
could potentially just decide to do nothing at all about this issue.
If the only reason for expanding TOAST pointers inside the
expanded-record stuff is to avoid the possibility of one being
invalidated by a transaction commit, and if compression methods can't
be invalidated by a transaction commit, well then we don't really have
a problem. That's not a great solution in terms of our chances of
getting this whole patch series committed, but it might at least be
enough to unblock the first few patches, and we could document the
rest of the issue for later research.

What makes me a bit uncomfortable about that approach is that it
presupposes that everything that uses expanded records has some other
defense against those tuples getting written to disk without first
expanding any external datums. And it isn't obvious that this is the
case, or at least not to me. For example, PLpgsql's
coerce_function_result_tuple()'s code for
VARATT_IS_EXTERNAL_EXPANDED() has three cases. The first case passes
the tuple through SPI_returntuple() which calls
heap_copy_tuple_as_datum() which calls toast_flatten_tuple_to_datum()
if required, but the second case calls EOH_flatten_into() and does NOT
pass the result through SPI_returntuple(). And ER_flatten_info() has
no defense against this case that I can see: sure, it skips the fast
path if ER_FLAG_HAVE_EXTERNAL is set, but that doesn't actually do
anything to resolve TOAST pointers. Maybe there's no bug there for
some reason, but I don't know what that reason might be. We seem to
have no test cases either in the main test suite or in the plpgsql
test suite where ER_flatten_info gets called with
ER_FLAG_HAVE_EXTERNAL is set, which seems a little unfortunate. If
there is such a bug here it's independent of this patch, I suppose,
but it would still be nice to understand what's going on here better
than I do.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, Feb 19, 2021 at 4:21 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> What makes me a bit uncomfortable about that approach is that it
> presupposes that everything that uses expanded records has some other
> defense against those tuples getting written to disk without first
> expanding any external datums. And it isn't obvious that this is the
> case, or at least not to me. For example, PLpgsql's
> coerce_function_result_tuple()'s code for
> VARATT_IS_EXTERNAL_EXPANDED() has three cases. The first case passes
> the tuple through SPI_returntuple() which calls
> heap_copy_tuple_as_datum() which calls toast_flatten_tuple_to_datum()
> if required, but the second case calls EOH_flatten_into() and does NOT
> pass the result through SPI_returntuple(). And ER_flatten_info() has
> no defense against this case that I can see: sure, it skips the fast
> path if ER_FLAG_HAVE_EXTERNAL is set, but that doesn't actually do
> anything to resolve TOAST pointers. Maybe there's no bug there for
> some reason, but I don't know what that reason might be. We seem to
> have no test cases either in the main test suite or in the plpgsql
> test suite where ER_flatten_info gets called with
> ER_FLAG_HAVE_EXTERNAL is set, which seems a little unfortunate. If
> there is such a bug here it's independent of this patch, I suppose,
> but it would still be nice to understand what's going on here better
> than I do.

Andres just pointed out to me the error of my thinking here:
ER_flatten_into can *never* encounter a case with both
ER_FLAG_FVALUE_VALID and ER_FLAG_HAVE_EXTERNAL, because
ER_get_flat_size has to get called first, and will de-toast external
values as it goes. So there actually is justification for
coerce_function_result_tuple() to skip the call to SPI_returntuple().

Given that, one might wonder why the test in ER_flatten_into() even
cares about ER_FLAG_HAVE_EXTERNAL in the first place... I suppose it's
just a harmless oversight.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Sat, Feb 20, 2021 at 2:51 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Fri, Feb 19, 2021 at 11:12 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> I think that these performance tests aren't really exercising the
> expanded-record stuff, just the ExecEvalRow changes. We need to test
> that test case, and I tend to suspect there's going to be a measurable
> regression.

I will do testing around this area.

> I spent some time looking at how datums get into the expanded record
> system. There seem to be four possible paths:
> expanded_record_set_tuple(), make_expanded_record_from_datum(),
> expanded_record_set_field_internal(), and
> expanded_record_set_fields(). The first two of these inject an entire
> tuple, while the latter two work on a field-by-field basis. For that
> reason, the latter two are not really problematic. I'm not quite sure
> what the right thing to do is here, but if we wanted to check whether
> a Datum that we're absorbing is non-pglz-compressed in those places,
> it would be easy to do. Also, as far as I can see,
> make_expanded_record_from_datum() is completely unused. So the problem
> case is where expanded_record_set_tuple() is getting called, and
> specifically where it's being called with expand_external = true. Any
> place that it's being called with expand_external = false, there's
> apparently no problem with the result tuple containing external
> datums, so probably non-pglz compressed data is OK there too.
> All of the places that can potentially pass expand_external = true are
> actually passing !estate->atomic, where estate is a PLpgSQL_execstate.
> In other words, I think the case where this happens is when we're in a
> context where the computed value could need to survive across a COMMIT
> or ROLLBACK, like there may be a procedure running (maybe more than
> one, each invoking the next via CALL) but there are no queries in
> progress. We have to expand TOAST references because committing a
> transaction that deleted data means you might not be able to resolve
> the old TOAST pointer any more: even if you use a snapshot that can
> see everything, VACUUM could nuke the deleted rows - or some of them -
> at any time. To avoid trouble we have to un-externalize before any
> COMMIT or ROLLBACK occurs. That can suck for performance because we
> might be fetching a big value that we don't end up using for anything
> - say if the variable isn't used again - but it beats failing.

I agree with most of this, but I don't think the only reason to
un-externalize is just for COMMIT or ROLLBACK. I mean using the
trigger function we might insert a RECORD type to another table which
has the ROWTYPE as the table on which we are doing the operation. See
below example

CREATE TABLE t1(a int, b varchar compression lz4);
INSERT INTO t1 select 1, repeat('a', 3000);
CREATE TABLE t2 (x t1 compression pglz);

CREATE OR REPLACE FUNCTION log_last_name_changes()
RETURNS TRIGGER
LANGUAGE PLPGSQL
AS
$$
BEGIN
INSERT INTO t2 select OLD;
RETURN NEW;
END;
$$;

CREATE TRIGGER last_name_changes
BEFORE UPDATE
ON t1
FOR EACH ROW
EXECUTE PROCEDURE log_last_name_changes();

UPDATE t1 SET a=2;

SELECT pg_column_compression((t2.x).b) FROM t2;
pg_column_compression
-----------------------
lz4
(1 row)

So basically, in this case, we are not un-externalizing because of
ROLLBACK or COMMIT, instead, we are doing that because we want to
insert it into the new table. So this is without my patch and without
my patch (v25_0001_Disallow_compressed_data_inside_container_types,
basically without the changes in expandedrecord.c). Here is the call
stack when exactly this tuple gets flattened.

#0 expanded_record_set_field_internal (erh=0x2bbcfb0, fnumber=2,
newValue=45863112, isnull=false, expand_external=true,
check_constraints=false) at expandedrecord.c:1225
#1 0x00000000009a1899 in ER_get_flat_size (eohptr=0x2bbcfb0) at
expandedrecord.c:713
#2 0x00000000009a0954 in EOH_get_flat_size (eohptr=0x2bbcfb0) at
expandeddatum.c:77
#3 0x000000000048f61c in heap_compute_data_size (tupleDesc=0x2bd0168,
values=0x2bd02c8, isnull=0x2bd02d0) at heaptuple.c:155
#4 0x00000000004916b3 in heap_form_tuple (tupleDescriptor=0x2bd0168,
values=0x2bd02c8, isnull=0x2bd02d0) at heaptuple.c:1045
#5 0x00000000007296eb in tts_virtual_copy_heap_tuple (slot=0x2bd0280)
at execTuples.c:272
#6 0x0000000000728d0b in ExecCopySlotHeapTuple (slot=0x2bd0280) at
../../../src/include/executor/tuptable.h:456
#7 0x000000000072a5d8 in tts_buffer_heap_copyslot (dstslot=0x2bd0820,
srcslot=0x2bd0280) at execTuples.c:767
#8 0x0000000000758840 in ExecCopySlot (dstslot=0x2bd0820,
srcslot=0x2bd0280) at ../../../src/include/executor/tuptable.h:480
#9 0x000000000075c263 in ExecModifyTable (pstate=0x2bcfa08) at
nodeModifyTable.c:2264

> The argument that we need to force decompression in such cases is
> considerably more tenuous. It revolves around the possibility that the
> compression AM itself has been dropped. As long as we have only
> built-in compression methods, which are undroppable, it seems like we
> could potentially just decide to do nothing at all about this issue.
> If the only reason for expanding TOAST pointers inside the
> expanded-record stuff is to avoid the possibility of one being
> invalidated by a transaction commit, and if compression methods can't
> be invalidated by a transaction commit, well then we don't really have
> a problem.

Even after the above case, we might say it is still not a problem for
this patch because even though t2 doesn't have a direct relationship
with lz4 but it has an indirect relationship with lz4 via t1. So I
think this particular case which I showed might not be a problem even
for the custom compression method. However, I agree that the
decompressing to survive COMMIT/ROLLBACK might be a problem for custom
compression methods but not for the built-in method. So I agree with
the conclusion that even if we don't make any changes to the
"expandedrecord.c", it won't be a problem for the built-in methods.

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

On Sat, Feb 20, 2021 at 11:04 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

> Even after the above case, we might say it is still not a problem for
> this patch because even though t2 doesn't have a direct relationship
> with lz4 but it has an indirect relationship with lz4 via t1. So I
> think this particular case which I showed might not be a problem even
> for the custom compression method. However, I agree that the
> decompressing to survive COMMIT/ROLLBACK might be a problem for custom
> compression methods but not for the built-in method. So I agree with
> the conclusion that even if we don't make any changes to the
> "expandedrecord.c", it won't be a problem for the built-in methods.

I think I was wrong here, consider below scenario (Robert had sent
this test to me(offlist) for showing some other example, which I have
modified a bit to prove another point)

create table foo (a int, b text);
create table foo1 (a int, b text compression lz4);
insert into foo1 select 1, repeat('a', 3000);

create or replace function make_foo() returns foo as $$declare x foo;
begin
x.a = 1;
select b into x.b from foo1;
return x;
end$$ language plpgsql;

create table bar (f foo);
insert into bar select make_foo();
SELECT pg_column_compression((bar.f).b) FROM bar;
pg_column_compression
-----------------------
lz4
(1 row)

So basically, now table bar doesn't have any relation with foo1 and it
is still the row with compression method lz4. This issue is resolved
with my changes in
(v25_0001_Disallow_compressed_data_inside_container_types, in
expandedrecord.c). So the point is that for the built-in method also
we need changes related to expandedrecord at least the changes I made
where tuple are actually formed for inserting into the target.

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

On Fri, Feb 19, 2021 at 9:42 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
Justin reported an issue offlist, basically, compression_1.out was not
updated so cfbot was failing on windows while running without-lz4, so
I have fixed that. Along with that I have also fixed 2 minor pending
issues 1) removing of compressamapi.h from unwanted places, reported
by Justin and 2) Assert in TOAST_COMPRESS_SET_SIZE_AND_METHOD macro,
suggested by Robert.

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

On Sat, Feb 20, 2021 at 11:04 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Sat, Feb 20, 2021 at 2:51 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >
> > On Fri, Feb 19, 2021 at 11:12 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > I think that these performance tests aren't really exercising the
> > expanded-record stuff, just the ExecEvalRow changes. We need to test
> > that test case, and I tend to suspect there's going to be a measurable
> > regression.
>
> I will do testing around this area.

I have done testing for the expanded-record. Basically I have noticed
there is no performance regression when there are no
compressed/external fields. But there is a huge regression when there
are compressed data.

Test setup:
----------------
create table foo (a int, b text, c text);
create table bar (f foo);

create or replace function make_foo() returns foo as $$declare x foo;
begin
x.a = 1;
select b,c into x.b, x.c from foo;
return x;
end$$ language plpgsql;

create or replace function test() returns void AS
$$
begin
for i in 1..100000 loop
insert into bar select make_foo();
end loop;
end;
$$ language 'plpgsql';

Testcase: select test(); (every time truncate bar before executing this query)

Case1: No compress/no external
insert into foo select 1, repeat('1234567890', 10), repeat('1234567890', 10);

execution time for "select test()"
Head: 2536.420 ms
patch: 2688.565 ms

Case2: Only compress
insert into foo select 1, repeat('1234567890', 500), repeat('1234567890', 10);

execution time for "select test()"
head: 2545.944 ms
Patch: 9375.524 ms

Case2: compress + external
Alter table foo alter column c set storage external;
insert into foo select 1, repeat('1234567890', 500), repeat('1234567890', 500);

execution time for "select test()"
Head: 10265.052 ms
Patch: 15469.902 ms

Summary:
In this particular path we are only processing the already deformed
tuple that is the reason we are not seeing regression with the
non-compressed data. But with compressed data we have to give extra
cost for the decompression and that is why there is regression. But
IMHO with this we are getting one benefit is that now we will not have
individual compressed data inside composite type so next time when we
will have to select from those composite type then we will have to do
less decompression with patch compared to the head so we might gain
there. I will try to come up with the complete test case where we
will do selection after inserting into the composite type and compare
the performance.

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

On Sun, Feb 21, 2021 at 5:33 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>

Based on offlist discussion with Robert, I have done further analysis
of the composite type data. So the Idea is that I have analyzed all
the callers of
HeapTupleGetDatum and HeapTupleHeaderGetDatum and divide them into two
category 1) Callers which are forming the tuple from values that can
not have compressed/external data.
2) Callers which can have external/compressed data

So for type 1) instead of calling HeapTupleGetDatum or
HeapTupleHeaderGetDatum we can call some function HeapTupleGetRawDatum
which is similar to PoniterGetDatum. And for type 2) we will detoast
any varlena even before forming the tuple so that we don't have to pay
the penalty for checking the compressed attributes after forming the
tuple. After this change now we have no caller for HeapTupleGetDatum
but I have kept it because it is an exposed routine.

Here is the analysis for the callers for HeapTupleGetDatum and
HeapTupleHeaderGetDatum
1. functions which can build tuple from compressed/external filed
(Detoasted before forming the tuple)
ExecEvalRow(), ExecEvalConvertRowtype(), ExecEvalConvertRowtype(),
populate_record()
exec_eval_datum()->make_tuple_from_row() before forming tuple
populate_record()

2. functions (no compressed/external filed possible analysis given
function wise):
dblink_get_pkey() : INT and Name tuple built from fixed length data types
hstore_populate_record(),hstore_each() : Getting values from hstore no
ondisk varlena possible
pg_old_snapshot_time_mapping(): Building tuple from in memory old snapshot data
pg_buffercache_pages(): Buffer cache info
pg_stat_statements_info(): No disk value, just stat_statement info
pgp_armor_headers(): Building in memory string in
pgp_extract_armor_headers and operating on those values
pgstattuple_approx_internal(): no varlena
ssl_extension_info(): ssl extension info, no any ondisk data
pg_last_committed_xact(): fixed length field
pg_xact_commit_timestamp_origin(): No varlena field
pg_get_multixact_members(): multixact info no actual tuple data
pg_prepared_xact(): prepared xact info
pg_walfile_name_offset(): walfile name/offset
pg_get_object_address(), pg_identify_object(),
pg_identify_object_as_address(): Only object information, form from in
memory strings.
pg_sequence_parameters(): No varlena field
pg_stat_get_wal_receiver(): walreceiver statistics
pg_stats_ext_mcvlist_items(): In memory array or fixed length fields.
tt_process_call(),prs_process_call: parser tokens
aclexplode(): fixed type and cstring from in memory string.
pg_timezone_abbrevs(): no varlena
pg_stat_file(): no varlena
pg_lock_status(): lock stats
pg_get_keywords(), pg_get_catalog_foreign_keys(): in memory strings.
pg_partition_tree(): no varlena
pg_stat_get_wal(): no varlena
tsvector_unnest(): in memory array
show_all_settings(): guc values from in memory struct
plperl_hash_to_datum(): value fetched from perl hash
pltcl_func_handler(): tuple from cstrings
test_predtest(): no varlena
pg_visibility*(): Only data from visibility map so no varlena
pg_stat_get_wal(), pg_stat_get_archiver(): Building tuple from in-memory data
pgstatindex_impl(), pgstatginindex_internal(), pgstathashindex():
fixed/in-memory data
replication slot func in slotfunc.c: replication slot info
controls file info functions in pg_conttroldata.c : Control file data
page inspect function in contrib/pageinspect(brinfunc.c, btreefunc.c,
ginfunc.c, gistfunc.c, hashfunc.c, heapfunc.c): Only meta page or
header info.
record_in and record_recv, are forming tuple from input cstring

Next I will be working on reviewing GUC for default compression method
by Justin and post the next patch series.

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

On Fri, Feb 26, 2021 at 8:10 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Sun, Feb 21, 2021 at 5:33 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
>
> Based on offlist discussion with Robert, I have done further analysis
> of the composite type data. So the Idea is that I have analyzed all
> the callers of
> HeapTupleGetDatum and HeapTupleHeaderGetDatum and divide them into two
> category 1) Callers which are forming the tuple from values that can
> not have compressed/external data.
> 2) Callers which can have external/compressed data

I just realized that there is one more function
"heap_copy_tuple_as_datum" which is flattening the tuple based on the
HeapTupleHasExternal check, so I think I will have to analyze the
caller of this function as well and need to do a similar analysis,
although there are just a few callers for this. And, I think the fix
in ExecEvalConvertRowtype is wrong, we will have to do something for
the compressed type here as well. I am not sure what is the best way
to fix it because we are directly getting the input tuple so we can
not put an optimization of dettoasting before forming the tuple. We
might detoast in execute_attr_map_tuple, when the source and target
row types are different because we are anyway deforming and processing
each filed in that function but the problem is execute_attr_map_tuple
is used at multiple places but for that, we can make another version
of this function which actually detoast along with conversion and use
that in ExecEvalConvertRowtype. But if there is no tuple conversion
needed then we directly use heap_copy_tuple_as_datum and in that case,
there is no deforming at all so maybe, in this case, we can not do
anything but I think ExecEvalConvertRowtype should not be the very
common path.

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

On Sat, Feb 27, 2021 at 7:44 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

> I just realized that there is one more function
> "heap_copy_tuple_as_datum" which is flattening the tuple based on the
> HeapTupleHasExternal check, so I think I will have to analyze the
> caller of this function as well and need to do a similar analysis,
> although there are just a few callers for this. And, I think the fix
> in ExecEvalConvertRowtype is wrong, we will have to do something for
> the compressed type here as well. I am not sure what is the best way
> to fix it because we are directly getting the input tuple so we can
> not put an optimization of dettoasting before forming the tuple. We
> might detoast in execute_attr_map_tuple, when the source and target
> row types are different because we are anyway deforming and processing
> each filed in that function but the problem is execute_attr_map_tuple
> is used at multiple places but for that, we can make another version
> of this function which actually detoast along with conversion and use
> that in ExecEvalConvertRowtype. But if there is no tuple conversion
> needed then we directly use heap_copy_tuple_as_datum and in that case,
> there is no deforming at all so maybe, in this case, we can not do
> anything but I think ExecEvalConvertRowtype should not be the very
> common path.

I have done further analysis for this, basically,
ExecEvalConvertRowtype can never have the compressed/external data
because it is converting from one composite type to another composite
type and while forming the composite type only we ensure that there
can not be any compressed/external data. Refer below comments in
ExecEvalConvertRowtype

/*
* The tuple is physically compatible as-is, but we need to insert the
* destination rowtype OID in its composite-datum header field, so we
* have to copy it anyway. heap_copy_tuple_as_datum() is convenient
* for this since it will both make the physical copy and insert the
* correct composite header fields. Note that we aren't expecting to
* have to flatten any toasted fields: the input was a composite
* datum, so it shouldn't contain any. So heap_copy_tuple_as_datum()
* is overkill here, but its check for external fields is cheap.
*/
*op->resvalue = heap_copy_tuple_as_datum(&tmptup, outdesc);

For heap_copy_tuple_as_datum, I have removed the external tuple check
and instead I have passed a parameter whether we need to flatten or
not. So the callers who are sure that they can not have any
compressed/external field should only pass false so that it will
completely skip the flattening path for those callers. But after
doing that in some of the callers especially
ExecFetchSlotHeapTupleDatum and
SPI_returntuple we will have to process the complete tuple when the
function's return type is tuple. I am not sure how to optimize this
because this is directly getting the tuple from the function. I am
not too much worried about the other callers like
PLyMapping_ToComposite, PLySequence_ToComposite and
PLyGenericObject_ToComposite because in these function we are forming
tuple from value before calling heap_copy_tuple_as_datum so if we
think these are performance critical paths then we have a way to
detoast even before forming the tuple.

Another function which I think can be problematic is
"expanded_record_set_tuple", because if we don't handle the compressed
types in this function then we can not go with the assumption that the
composite will never have compressed data. I am not completely sure
how much of a problem that can be? Maybe if we don't do anything here
then we might need to do something in ExecEvalConvertRowtype because
therein we assume that composite type can not contain compressed data
as well.

I have also reviewed the patch for the default compression method GUC
and made some changes.

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

> Subject: [PATCH v28 3/4] Add default_toast_compression GUC

This part isn't working. My first patch worked somewhat better: due to doing
strcmp() with the default GUC, it avoided using the cached AM OID. (But it
would've failed with more than 2 AMs, since the cache wasn't invalidated, since
I couldn't tell when it was needed).

Your patch does this:

|postgres=# SET default_toast_compression=lz4 ;
|postgres=# CREATE TABLE t(a text);
|postgres=# \d+ t
| a | text | | | | extended | pglz | |

assign_default_toast_compression() should set
default_toast_compression_oid=InvalidOid, rather than
default_toast_compression=NULL.

In my original patch, that was commented, since I was confused, not realizing
that the GUC machinery itself assigns to the string value. We should assign to
the cached Oid, instead.

Reading my own patch, I see that in AccessMethodCallback() should also say
InvalidOid.
| default_toast_compression_oid = false;
The false assignment was copied from namespace.c: baseSearchPathValid.

--
Justin

On my PC, this new change is causing a test failure:

SELECT SUBSTR(f1, 2000, 50) FROM cmdata1;
- substr
-----------------------------------------------------
- 01234567890123456789012345678901234567890123456789
-(1 row)
-
+ERROR: compressed lz4 data is corrupt

@@ -119,15 +119,15 @@ lz4_cmdecompress_slice(const struct varlena *value, int32 slicelength)
int32 rawsize;
struct varlena *result;

- /* allocate memory for holding the uncompressed data */
- result = (struct varlena *) palloc(VARRAWSIZE_4B_C(value) + VARHDRSZ);
+ /* allocate memory for the uncompressed data */
+ result = (struct varlena *) palloc(slicelength + VARHDRSZ);

- /* decompress partial data using lz4 routine */
+ /* decompress the data */
rawsize = LZ4_decompress_safe_partial((char *) value + VARHDRSZ_COMPRESS,
VARDATA(result),
VARSIZE(value) - VARHDRSZ_COMPRESS,
slicelength,
- VARRAWSIZE_4B_C(value));
+ slicelength);

Also, in the tests, you have this at both the top and bottom of the file:

src/test/regress/sql/compression.sql:\set HIDE_COMPRESSAM false
src/test/regress/sql/compression.sql:\set HIDE_COMPRESSAM false

Whereas the patch I sent had at the end:

+\set HIDE_COMPRESSAM on

("on" is the default when run under pg_regress)

On Sun, Feb 28, 2021 at 9:48 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> On my PC, this new change is causing a test failure:
>
> SELECT SUBSTR(f1, 2000, 50) FROM cmdata1;
> - substr
> -----------------------------------------------------
> - 01234567890123456789012345678901234567890123456789
> -(1 row)
> -
> +ERROR: compressed lz4 data is corrupt

The older version of lz4 had this problem that while decompressing
partial if we don't give the buffer size up to full data length it was
failing[1] but it is solved in version 1.9.

> @@ -119,15 +119,15 @@ lz4_cmdecompress_slice(const struct varlena *value, int32 slicelength)
> int32 rawsize;
> struct varlena *result;
>
> - /* allocate memory for holding the uncompressed data */
> - result = (struct varlena *) palloc(VARRAWSIZE_4B_C(value) + VARHDRSZ);
> + /* allocate memory for the uncompressed data */
> + result = (struct varlena *) palloc(slicelength + VARHDRSZ);
>
> - /* decompress partial data using lz4 routine */
> + /* decompress the data */
> rawsize = LZ4_decompress_safe_partial((char *) value + VARHDRSZ_COMPRESS,
> VARDATA(result),
> VARSIZE(value) - VARHDRSZ_COMPRESS,
> slicelength,
> - VARRAWSIZE_4B_C(value));
> + slicelength);

This is done for the latest version as now we don't need to allocate
the buffer of full size, it is enough the allocate just equal to the
slicelength.

> Also, in the tests, you have this at both the top and bottom of the file:
>
> src/test/regress/sql/compression.sql:\set HIDE_COMPRESSAM false
> src/test/regress/sql/compression.sql:\set HIDE_COMPRESSAM false
>
> Whereas the patch I sent had at the end:
>
> +\set HIDE_COMPRESSAM on
>
> ("on" is the default when run under pg_regress)

I will fix this.

[1] https://docs.unrealengine.com/en-US/API/Runtime/Core/Compression/LZ4_decompress_safe_partial/index.html

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

On Mon, Mar 01, 2021 at 10:32:23AM +0530, Dilip Kumar wrote:
> On Sun, Feb 28, 2021 at 9:48 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> >
> > On my PC, this new change is causing a test failure:
> >
> > SELECT SUBSTR(f1, 2000, 50) FROM cmdata1;
> > - substr
> > -----------------------------------------------------
> > - 01234567890123456789012345678901234567890123456789
> > -(1 row)
> > -
> > +ERROR: compressed lz4 data is corrupt
>
> The older version of lz4 had this problem that while decompressing
> partial if we don't give the buffer size up to full data length it was
> failing[1] but it is solved in version 1.9.

Thanks. It seems like that explains it.
I think if that's a problem with recent versions, then you'll have to
conditionally disable slicing.
https://packages.debian.org/liblz4-dev

Slicing isn't generally usable if it sometimes makes people's data inaccessible
and gives errors about corruption.

I guess you could make it a compile time test on these constants (I don't know
the necessary version, though)

#define LZ4_VERSION_MAJOR 1 /* for breaking interface changes */
#define LZ4_VERSION_MINOR 7 /* for new (non-breaking) interface capabilities */
#define LZ4_VERSION_RELEASE 1 /* for tweaks, bug-fixes, or development */
#define LZ4_VERSION_NUMBER (LZ4_VERSION_MAJOR *100*100 + LZ4_VERSION_MINOR *100 + LZ4_VERSION_RELEASE)

If the version is too low, either make it #error, or disable slicing.
The OS usual library version infrastructure will make sure the runtime version
is at least the MAJOR+MINOR of the compile time version.

--
Justin

On Sat, Feb 27, 2021 at 9:35 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> > Subject: [PATCH v28 3/4] Add default_toast_compression GUC
>
> This part isn't working. My first patch worked somewhat better: due to doing
> strcmp() with the default GUC, it avoided using the cached AM OID. (But it
> would've failed with more than 2 AMs, since the cache wasn't invalidated, since
> I couldn't tell when it was needed).
>
> Your patch does this:
>
> |postgres=# SET default_toast_compression=lz4 ;
> |postgres=# CREATE TABLE t(a text);
> |postgres=# \d+ t
> | a | text | | | | extended | pglz | |
>
> assign_default_toast_compression() should set
> default_toast_compression_oid=InvalidOid, rather than
> default_toast_compression=NULL.

I will fix this.

> In my original patch, that was commented, since I was confused, not realizing
> that the GUC machinery itself assigns to the string value. We should assign to
> the cached Oid, instead.

> Reading my own patch, I see that in AccessMethodCallback() should also say
> InvalidOid.
> | default_toast_compression_oid = false;
> The false assignment was copied from namespace.c: baseSearchPathValid.

I will fix this.

So as of now, we can make this patch such that it is enough to work
with the built-in method and later we can add another enhancement
patch that can work with the custom compression methods.

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

On Mon, Mar 1, 2021 at 11:06 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:

> Thanks. It seems like that explains it.
> I think if that's a problem with recent versions, then you'll have to
> conditionally disable slicing.
> https://packages.debian.org/liblz4-dev
>
> Slicing isn't generally usable if it sometimes makes people's data inaccessible
> and gives errors about corruption.
>
> I guess you could make it a compile time test on these constants (I don't know
> the necessary version, though)
>
> #define LZ4_VERSION_MAJOR 1 /* for breaking interface changes */
> #define LZ4_VERSION_MINOR 7 /* for new (non-breaking) interface capabilities */
> #define LZ4_VERSION_RELEASE 1 /* for tweaks, bug-fixes, or development */
> #define LZ4_VERSION_NUMBER (LZ4_VERSION_MAJOR *100*100 + LZ4_VERSION_MINOR *100 + LZ4_VERSION_RELEASE)
>
> If the version is too low, either make it #error, or disable slicing.
> The OS usual library version infrastructure will make sure the runtime version
> is at least the MAJOR+MINOR of the compile time version.

I think we can check the version and if it too low i.e. below1.8.3 (
in this release the slicing issue was fixed) then we can call the full
decompression routine from the slicing function.

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

On Mon, Mar 1, 2021 at 5:36 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Mon, Mar 1, 2021 at 11:06 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> > Thanks. It seems like that explains it.
> > I think if that's a problem with recent versions, then you'll have to
> > conditionally disable slicing.
> > https://packages.debian.org/liblz4-dev
> >
> > Slicing isn't generally usable if it sometimes makes people's data inaccessible
> > and gives errors about corruption.
> >
> > I guess you could make it a compile time test on these constants (I don't know
> > the necessary version, though)
> >
> > #define LZ4_VERSION_MAJOR 1 /* for breaking interface changes */
> > #define LZ4_VERSION_MINOR 7 /* for new (non-breaking) interface capabilities */
> > #define LZ4_VERSION_RELEASE 1 /* for tweaks, bug-fixes, or development */
> > #define LZ4_VERSION_NUMBER (LZ4_VERSION_MAJOR *100*100 + LZ4_VERSION_MINOR *100 + LZ4_VERSION_RELEASE)
> >
> > If the version is too low, either make it #error, or disable slicing.
> > The OS usual library version infrastructure will make sure the runtime version
> > is at least the MAJOR+MINOR of the compile time version.
>
> I think we can check the version and if it too low i.e. below1.8.3 (
> in this release the slicing issue was fixed) then we can call the full
> decompression routine from the slicing function.

I have done that in the attached patch. Along with that, I have also
fixed the other issues raised by Justin related to the compression
method GUC patch and also removed the stuff from the GUC patch which
is not required for the built-in methods.

Now, I think the only pending thing is related to the expandedrecord,
basically, currently, we have detoasted the compressed filed only in
expanded_record_set_field_internal function. I am still not
completely sure that for the built-in types do we need to do something
for expanded_record_set_tuple and expanded_record_set_field or not, I
mean in these functions do we only expand the external to survive the
COMMIT/ROLLBACK or do we also expand it send it to some target table
like we do in expanded_record_set_field_internal.

As Robert mentioned upthread fixing in expanded_record_set_field might
not be very problematic as the tuple is already deformed but we have a
problem in expanded_record_set_tuple as we might need to deform the
tuple even though there are no compressed/external data.

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

On Mon, Mar 1, 2021 at 8:53 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> Now, I think the only pending thing is related to the expandedrecord,
> basically, currently, we have detoasted the compressed filed only in
> expanded_record_set_field_internal function. I am still not
> completely sure that for the built-in types do we need to do something
> for expanded_record_set_tuple and expanded_record_set_field or not, I
> mean in these functions do we only expand the external to survive the
> COMMIT/ROLLBACK or do we also expand it send it to some target table
> like we do in expanded_record_set_field_internal.

I have done further analysis of the compressed field in the
expandedrecord. My observation is that only in
expanded_record_set_field_internal we unconditionally pass true and
only when it is called from ER_get_flat_size. In all the other
functions (expanded_record_set_tuple and expanded_record_set_fields)
we only pass expand_external to true if estate->atomic is not set.
And, the estate->atomic is set to false only if we are executing the
anonymous block from a transaction block (there might be another way
to have estate->atomic as false). But the point is that the
flattening in these two functions are conditional which means we can
not use these expanded records to form some kind of row, otherwise, we
can not have the conditional flattening based on the way how the PL
block is being executed, so I think this proves Robert's point that we
are expanding this only for surviving the commit/rollback inside the
PL block. That means for the built-in types, decompression in
expanded_record_set_field_internal should be sufficient and that was
already done in my latest version of patch v29-0001.

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

Hi,

Does this patch need to do something about ExtractReplicaIdentity()?
If there are compressed fields in the tuple being built, can we rely
on the decompression engine being available at the time we need to do
something with the tuple?

More generally, I think it would be good to divide up 0001 into at
least 3 parts:

- The first part would invent HeapTupleGetRawDatum() and
HeapTupleHeaderGetRawDatum() and use them in place of the existing
functions everywhere that it's safe to do so. The current patch just
switches to using PointerGetDatum() but I think we should instead add
something like static inline Datum
HeapTupleHeaderGetRawDatum(HeapTupleHeader tuple) {
Assert(!HeapTupleHeaderHasExternal(tup)); return
PointerGetDatum(tuple); } This provides some type safety while being
just as fast as a direct use of PointerGetDatum() in optimized code. I
think that the Assert will have to be ripped out if we proceed with
the other patches, but if we can have it at this stage, so much the
better. I think this patch should also invent
PG_RETURN_HEAPTUPLEHEADER_RAW and likewise use that where appropriate
- including, I think, in place of cases that are now using
PG_RETURN_DATUM(HeapTupleGetDatum(...)). All of these changes make
sense from an efficiency standpoint apart from any possible
definitional changes.

- The second part would optimize code that the first part cannot
safely convert to use the "raw" versions. For example, the changes to
ExecEvalRow() can go in this part. You can view this part as getting
rid of calls to HeapTupleGetDatum(), HeapTupleHeaderGetDatum(), and/or
PG_RETURN_HEAPTUPLE() that couldn't be changed categorically, but can
be changed if we make some other code changes. These changes, too, can
potentially be justified on performance grounds independently of
anything else.

- Then we could maybe have some more patches that make other kinds of
preparatory changes. I'm not too sure exactly what should go in here,
or whether it should be 1 patch or several or maybe 0. But if there's
preparatory stuff that's potentially separately committable and not
the same as the stuff above, then it should go into patches here.

- The last patch would actually change the rule for composite datums.

One advantage of this is that if we have to revert the last patch for
some reason we are not ripping the entire thing, churning the code
base and widely-used APIs for everyone. Another advantage is that
getting those first two patches committed or even just applied locally
on a branch would, at least IMHO, make it a lot simpler to see what
potential problem spots remain - and by "problem" I mean mostly from a
performance point of view.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Mar 4, 2021 at 2:49 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> Hi,
>
> Does this patch need to do something about ExtractReplicaIdentity()?
> If there are compressed fields in the tuple being built, can we rely
> on the decompression engine being available at the time we need to do
> something with the tuple?

We log the replica identity tuple in the WAL so that later walsender
can stream this to the subscriber, and before sending to the
subscriber anyway we have detoast all the data. Said that I think the
problem you are worried about is not only with 'replica identity
tuple' but it is with any tuple. I mean we copy the compressed field
as it is in WAL and suppose we copy some fields which are compressed
with lz4 and then we restart the server with another binary that is
compiled without lz4. Now, the problem is the walsender can not
decompress those data.

> More generally, I think it would be good to divide up 0001 into at
> least 3 parts:
>
> - The first part would invent HeapTupleGetRawDatum() and
> HeapTupleHeaderGetRawDatum() and use them in place of the existing
> functions everywhere that it's safe to do so. The current patch just
> switches to using PointerGetDatum() but I think we should instead add
> something like static inline Datum
> HeapTupleHeaderGetRawDatum(HeapTupleHeader tuple) {
> Assert(!HeapTupleHeaderHasExternal(tup)); return
> PointerGetDatum(tuple); } This provides some type safety while being
> just as fast as a direct use of PointerGetDatum() in optimized code. I
> think that the Assert will have to be ripped out if we proceed with
> the other patches, but if we can have it at this stage, so much the
> better. I think this patch should also invent
> PG_RETURN_HEAPTUPLEHEADER_RAW and likewise use that where appropriate
> - including, I think, in place of cases that are now using
> PG_RETURN_DATUM(HeapTupleGetDatum(...)). All of these changes make
> sense from an efficiency standpoint apart from any possible
> definitional changes.
>
> - The second part would optimize code that the first part cannot
> safely convert to use the "raw" versions. For example, the changes to
> ExecEvalRow() can go in this part. You can view this part as getting
> rid of calls to HeapTupleGetDatum(), HeapTupleHeaderGetDatum(), and/or
> PG_RETURN_HEAPTUPLE() that couldn't be changed categorically, but can
> be changed if we make some other code changes. These changes, too, can
> potentially be justified on performance grounds independently of
> anything else.
>
> - Then we could maybe have some more patches that make other kinds of
> preparatory changes. I'm not too sure exactly what should go in here,
> or whether it should be 1 patch or several or maybe 0. But if there's
> preparatory stuff that's potentially separately committable and not
> the same as the stuff above, then it should go into patches here.
>
> - The last patch would actually change the rule for composite datums.
>
> One advantage of this is that if we have to revert the last patch for
> some reason we are not ripping the entire thing, churning the code
> base and widely-used APIs for everyone. Another advantage is that
> getting those first two patches committed or even just applied locally
> on a branch would, at least IMHO, make it a lot simpler to see what
> potential problem spots remain - and by "problem" I mean mostly from a
> performance point of view.

Okay, I will work on this.

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

On Thu, Mar 4, 2021 at 4:03 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Thu, Mar 4, 2021 at 2:49 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> > More generally, I think it would be good to divide up 0001 into at
> > least 3 parts:
>
> Okay, I will work on this.

As per the suggestion I have divided 0001 in 3 patches.

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

On Thu, Mar 4, 2021 at 4:03 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Thu, Mar 4, 2021 at 2:49 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >
> > Hi,
> >
> > Does this patch need to do something about ExtractReplicaIdentity()?
> > If there are compressed fields in the tuple being built, can we rely
> > on the decompression engine being available at the time we need to do
> > something with the tuple?
>
> We log the replica identity tuple in the WAL so that later walsender
> can stream this to the subscriber, and before sending to the
> subscriber anyway we have detoast all the data. Said that I think the
> problem you are worried about is not only with 'replica identity
> tuple' but it is with any tuple. I mean we copy the compressed field
> as it is in WAL and suppose we copy some fields which are compressed
> with lz4 and then we restart the server with another binary that is
> compiled without lz4. Now, the problem is the walsender can not
> decompress those

Based on the off list discussion with Robert, there are a couple of
problems which might be very difficult to handle when we support the
custom compression method using the access methods, the major problems
are 1) compressed data inside composite type 2) Access method might
get dropped before walsender decode the compressed data. I think the
first problem we still have is some solution although it may impact
performance in some cases i.e. extended record. But the problem of
the compressed data inside the WAL is a bigger problem. So as of now
we are planning to go ahead only with the built-in methods and if we
are only continuing with the built-in method so it doesn't make sense
to continue the access method infrastructure. I have rewrote the
patches without using the access method for compression. Changes in
the patches

- Removed complete dependency on the access method for compression
- While moving the tuple from one table to another table with
different compression method, no need to compare the compression
method and decompress.
- Alter table set compression, will not rewrite the old data, so only
the new tuple will be compressed with the new compression method.
- No preserve.

I feel the built-in method patch now looks cleaner and smaller than it
was before.

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

On Sat, Mar 06, 2021 at 08:59:16PM +0530, Dilip Kumar wrote:
> - Alter table set compression, will not rewrite the old data, so only
> the new tuple will be compressed with the new compression method.
> - No preserve.

+1, this simplifies things. If someone *wants* to rewrite the table, they can
VACUUM FULL, CLUSTER, or dump+restore.

I checked that it's possible to do simple column manipulations on columns
written with-lz4 with binaries built without-lz4:
- is null
- texteq if length differs
- explain analyze

If I pg_upgrade from an binary with-lz4 to one without-lz4, it fails
while restoring the schema, after running check, which is bad:
| pg_restore: error: could not execute query: ERROR: not built with lz4 support
|CREATE TABLE "public"."a" (
| "t" "text" COMPRESSION lz4,

For comparison, upgrading from binaries with-libxml to binaries without-libxml
actualy passes pg_upgrade.

It's arguable which behavior is desirable:
- allow CREATE TABLE(..COMPRESSION lz4) during pg_upgrade;
- allow CREATE TABLE(..COMPRESSION lz4) always. This has the advantage that
GetAttributeCompression() doesn't have conditional compilation. This seems
to be parallel to the libxml case - apparently, it's possible to create an
XML column, but not insert into it.
- abort pg_upgrade --check if the old cluster has lz4 and the new one doesn't,
if there are any lz4 compressed columns. This avoids the possibilty of
running an upgrade to binaries without lz4, starting a new cluster (which
leaves the old cluster unsafe to start if --link was used), and then the new
cluster may even appear to work, until an LZ4 column is accessed in a
nontrivial way. It has the disadvantage that there's no obvious parallel in
pg_upgrade (checksums and xml are the closest?). And the disadvantage that
some people might *want* the upgrade to succeed in that case to then recompile
with lz4 afterwards.

In this patch, SET default_toast_compression=lz4 "works" even if without-lz4,
but then CREATE TABLE fails. You should either allow table creation (as
above), or check in check_default_toast_compression() if lz4 is enabled.
Its comment about "catalog access" is incorrect now.

Now, I wonder if default_toast_compression should be a GUC, or a reloption.
An obvious advantage of being a GUC is that regression tests are trivial with
make installcheck.

Some minor fixes:

+ if (strcmp(def->compression, newdef->compression))
!= 0

+ * NULL for non varlena type or the uncompressed data.
remove "the"

+ * InvalidOid for the plain/external storage otherwise default
remove "the"

+ behavior is to exclude compression methods, resulting in the columns
remove "the"

+ attcompression and attstorage for the respective index attribute if ... the respective input values are
say "and/or attstorage"

+ If this variable is set to <literal>true</literal>, column's [...]
I wrote this, but I guess it should say: columns'

I think you could also say either of these:
.. column compression method details are not displayed.
.. details of column compression are not displayed.

--
Justin

On Sun, Mar 7, 2021 at 1:27 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> On Sat, Mar 06, 2021 at 08:59:16PM +0530, Dilip Kumar wrote:
> > - Alter table set compression, will not rewrite the old data, so only
> > the new tuple will be compressed with the new compression method.
> > - No preserve.
>
> +1, this simplifies things. If someone *wants* to rewrite the table, they can
> VACUUM FULL, CLUSTER, or dump+restore.

Thanks

> I checked that it's possible to do simple column manipulations on columns
> written with-lz4 with binaries built without-lz4:
> - is null
> - texteq if length differs
> - explain analyze

That's because unless you really need to compress/decompress the data
it won't error out. IMHO this behavior is fine as this is the
behavior with other libraries also e.g libxml

> If I pg_upgrade from an binary with-lz4 to one without-lz4, it fails
> while restoring the schema, after running check, which is bad:
> | pg_restore: error: could not execute query: ERROR: not built with lz4 support
> |CREATE TABLE "public"."a" (
> | "t" "text" COMPRESSION lz4,
>
> For comparison, upgrading from binaries with-libxml to binaries without-libxml
> actualy passes pg_upgrade.
>
> It's arguable which behavior is desirable:
> - allow CREATE TABLE(..COMPRESSION lz4) during pg_upgrade;
> - allow CREATE TABLE(..COMPRESSION lz4) always. This has the advantage that
> GetAttributeCompression() doesn't have conditional compilation. This seems
> to be parallel to the libxml case - apparently, it's possible to create an
> XML column, but not insert into it.

IMHO we can always allow creating the table with lz4 and only error
out when we really need to compress/decompress the data. I like this
behavior because it is the same as libxml. But I am fine with
allowing it only in binary upgrade also. Another option could be to
fall back to default "pglz" in binary upgrade mode if it is built
without-lz4 but the problem is this will change the table
specification after the upgrade. So maybe we can go with either of
the first two options. Any other thoughts on this?

> - abort pg_upgrade --check if the old cluster has lz4 and the new one doesn't,
> if there are any lz4 compressed columns. This avoids the possibilty of
> running an upgrade to binaries without lz4, starting a new cluster (which
> leaves the old cluster unsafe to start if --link was used), and then the new
> cluster may even appear to work, until an LZ4 column is accessed in a
> nontrivial way. It has the disadvantage that there's no obvious parallel in
> pg_upgrade (checksums and xml are the closest?). And the disadvantage that
> some people might *want* the upgrade to succeed in that case to then recompile
> with lz4 afterwards.

Yeah.

> In this patch, SET default_toast_compression=lz4 "works" even if without-lz4,
> but then CREATE TABLE fails. You should either allow table creation (as
> above), or check in check_default_toast_compression() if lz4 is enabled.
> Its comment about "catalog access" is incorrect now.

I will fix the comment, I agree that only if we allow to always
create a table then only it makes sense to set the default as lz4 if
it is compiled without lz4.

> Now, I wonder if default_toast_compression should be a GUC, or a reloption.
> An obvious advantage of being a GUC is that regression tests are trivial with
> make installcheck.

I don't think it makes much sense to give a table-wise option, we
anyways have the option to give a compression method per attribute and
I think selecting the compression method more depends upon the
attribute data and type. So I think providing the GUC makes more
sense when the user wants to select some default compression method
for most of its attributes.

> Some minor fixes:
>
> + if (strcmp(def->compression, newdef->compression))
> != 0
>
> + * NULL for non varlena type or the uncompressed data.
> remove "the"
>
> + * InvalidOid for the plain/external storage otherwise default
> remove "the"
>
> + behavior is to exclude compression methods, resulting in the columns
> remove "the"
>
>
> + attcompression and attstorage for the respective index attribute if ... the respective input values are
> say "and/or attstorage"
>
> + If this variable is set to <literal>true</literal>, column's [...]
> I wrote this, but I guess it should say: columns'
>
> I think you could also say either of these:
> .. column compression method details are not displayed.
> .. details of column compression are not displayed.
>

Thanks, I will fix these in the next version.

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

On Sun, Mar 07, 2021 at 12:16:41PM +0530, Dilip Kumar wrote:
> > If I pg_upgrade from an binary with-lz4 to one without-lz4, it fails
> > while restoring the schema, after running check, which is bad:
> > | pg_restore: error: could not execute query: ERROR: not built with lz4 support
> > |CREATE TABLE "public"."a" (
> > | "t" "text" COMPRESSION lz4,

Actually, it looks like pg_upgrading an xml column works, but calling xml
functions fails.

I think that's a deficiency in pg_upgrade - it should be caught early during
the --check phase and not after dumping and in the middle of restoring the
schema (which can sometimes take significant time).

> > For comparison, upgrading from binaries with-libxml to binaries without-libxml
> > actualy passes pg_upgrade.
> >
> > It's arguable which behavior is desirable:
> > - allow CREATE TABLE(..COMPRESSION lz4) during pg_upgrade;
> > - allow CREATE TABLE(..COMPRESSION lz4) always. This has the advantage that
> > GetAttributeCompression() doesn't have conditional compilation. This seems
> > to be parallel to the libxml case - apparently, it's possible to create an
> > XML column, but not insert into it.
>
> IMHO we can always allow creating the table with lz4 and only error
> out when we really need to compress/decompress the data. I like this
> behavior because it is the same as libxml. But I am fine with
> allowing it only in binary upgrade also. Another option could be to
> fall back to default "pglz" in binary upgrade mode if it is built
> without-lz4 but the problem is this will change the table
> specification after the upgrade.

No, you certainly can't do that.
You'd have a table defined as pglz but with lz4 in the data files.
In the best case, it would give errors about corrupt lz4 data.

--
Justin

On Sun, Mar 7, 2021 at 12:47 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> > IMHO we can always allow creating the table with lz4 and only error
> > out when we really need to compress/decompress the data. I like this
> > behavior because it is the same as libxml. But I am fine with
> > allowing it only in binary upgrade also. Another option could be to
> > fall back to default "pglz" in binary upgrade mode if it is built
> > without-lz4 but the problem is this will change the table
> > specification after the upgrade.
>
> No, you certainly can't do that.
> You'd have a table defined as pglz but with lz4 in the data files.
> In the best case, it would give errors about corrupt lz4 data.

Yeah, we can not do that. Just missed that part :)

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

On Sun, Mar 07, 2021 at 01:36:50PM +0530, Dilip Kumar wrote:
> On Sun, Mar 7, 2021 at 12:47 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> > > IMHO we can always allow creating the table with lz4 and only error
> > > out when we really need to compress/decompress the data. I like this
> > > behavior because it is the same as libxml. But I am fine with
> > > allowing it only in binary upgrade also. Another option could be to
> > > fall back to default "pglz" in binary upgrade mode if it is built
> > > without-lz4 but the problem is this will change the table
> > > specification after the upgrade.
> >
> > No, you certainly can't do that.
> > You'd have a table defined as pglz but with lz4 in the data files.
> > In the best case, it would give errors about corrupt lz4 data.
>
> Yeah, we can not do that. Just missed that part :)

But I believe what you're thinking is that it ought to be possible to restore a
backup, even if the binaries in the target cluster don't support the
compression used by the source tables.

Earlier in this thread, I suggested to implement an option to pg_restore to
avoid outputting compression, in order to allow restoring with a different
compression (by using the default_toast_compression GUC). Now, it seems like
that's even more important, to allow restoring into binaries --without-lz4.
(the pg_dump isn't in LZ4 format, it just needs to not say "COMPRESSION LZ4").

I think you're planning to allow the CREATE TABLE to succeed in any case, but
it's not helpful if the DBA has to restore the schema, and then alter all the
text columns to set PGLZ, and then restore the data and post-data.

Also, I suggest to rename the pg_dump option:
| --no-compression-methods do not dump compression methods

I have a patch to pg_dump to support alternate compression in the dump itself
(in addition to zlib), so the name will be confusing. I suggest
--no-toast-compression, like the GUC. And the same for pg_restore.

--
Justin

On Sun, Mar 7, 2021 at 2:19 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> Earlier in this thread, I suggested to implement an option to pg_restore to
> avoid outputting compression, in order to allow restoring with a different
> compression (by using the default_toast_compression GUC). Now, it seems like
> that's even more important, to allow restoring into binaries --without-lz4.
> (the pg_dump isn't in LZ4 format, it just needs to not say "COMPRESSION LZ4").

IMHO, we have an option with pg_dump that should be sufficient, no?
but I agree that having such an option with restore will give more
flexibility basically, by using the same dump we can restore to binary
--with-lz4 as well as without-lz4 if such option exists with restore
as well. But it seems in pg_restore we process token by token so if
we want to implement such an option then I think we will have to parse
the complete string of CREATE TABLE command and remove the compression
option if it exists for any attribute. I am not sure whether providing
this option is worth the complexity?

> I think you're planning to allow the CREATE TABLE to succeed in any case, but
> it's not helpful if the DBA has to restore the schema, and then alter all the
> text columns to set PGLZ, and then restore the data and post-data.
>
> Also, I suggest to rename the pg_dump option:
> | --no-compression-methods do not dump compression methods
>
> I have a patch to pg_dump to support alternate compression in the dump itself
> (in addition to zlib), so the name will be confusing. I suggest
> --no-toast-compression, like the GUC. And the same for pg_restore.

Ok.

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

On Sun, Mar 7, 2021 at 1:27 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> On Sat, Mar 06, 2021 at 08:59:16PM +0530, Dilip Kumar wrote:
> > - Alter table set compression, will not rewrite the old data, so only
> > the new tuple will be compressed with the new compression method.
> > - No preserve.
>
> In this patch, SET default_toast_compression=lz4 "works" even if without-lz4,
> but then CREATE TABLE fails. You should either allow table creation (as
> above), or check in check_default_toast_compression() if lz4 is enabled.
> Its comment about "catalog access" is incorrect now.

As of now I have made GUC behavior similar to the CREATE TABLE, in
both case it will throw an error if it is not compiled with lz4
method.

>
> + if (strcmp(def->compression, newdef->compression))
> != 0
>
> + * NULL for non varlena type or the uncompressed data.
> remove "the"
>
> + * InvalidOid for the plain/external storage otherwise default
> remove "the"
>
> + behavior is to exclude compression methods, resulting in the columns
> remove "the"
>
>
> + attcompression and attstorage for the respective index attribute if ... the respective input values are
> say "and/or attstorage"
>
> + If this variable is set to <literal>true</literal>, column's [...]
> I wrote this, but I guess it should say: columns'
>
> I think you could also say either of these:
> .. column compression method details are not displayed.
> .. details of column compression are not displayed.

I have fixed the above comments, and also some other minor fixup.

So now only pending point is, how do we handle the upgrade when you
are upgrading from --with-lz4 to --without-lz4 binary and a couple of
options discussed here are
a) Should we allow table creation with lz4 even if it is compiled
--without-lz4? In case of xml we always allow table creation even if
it is compiled --wthout-libxml
b) Instead of allowing this always, only allow during binary upgrade.

With this we will be able to make binary upgrades successful.
However, if users have to access the data compiled with the lz4 then
then need to enable the lz4 support. But I think that is true with
any case for example with a hot standby setup if the user configures
the standby without lz4 then also for accessing the lz4 compressed
data user needs to enable the lz4 support.

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

On Sun, Mar 07, 2021 at 06:04:41PM +0530, Dilip Kumar wrote:
> On Sun, Mar 7, 2021 at 2:19 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> >
> > Earlier in this thread, I suggested to implement an option to pg_restore to
> > avoid outputting compression, in order to allow restoring with a different
> > compression (by using the default_toast_compression GUC). Now, it seems like
> > that's even more important, to allow restoring into binaries --without-lz4.
> > (the pg_dump isn't in LZ4 format, it just needs to not say "COMPRESSION LZ4").
>
> IMHO, we have an option with pg_dump that should be sufficient, no?

I think it's insufficient since people may be unable to restore from backup, or
can only restore backup by resorting to this:
pg_restore -f- |sed 's/COMPRESSION lz4//' |psql -d mydb

I think there's a parallel with --no-tablespaces. But if a tablespace is
missing/renamed, the table is still restored (with errors during SET
default_tablespace), whereas if lz4 is missing, the table is not restored.
Or actually the table would be created, but the COPY/INSERTs would fail.

There's an argument to be made that this is already an issue - for example,
I've numerous times done a partial restore of a single partition, where the
column types have changed in the parent, and I need to use sed to restore the
partition. However, that's improving - in v14: "attach table" is a separate
pg_dump object, so the table *is* restored, and only the ATTACH command fails.
(See 9a4c0e36f).

I wonder if COMPRESSION should be dumped as ALTER statements, not in the
CREATE. In fact, the CREATE syntax is optional and could be removed. Similar
to ALTER TABLE t ALTER c SET STATISTICS 99 - there's no CREATE grammar for
that.

Note that I think that using ALTER doesn't resolves this issue, since the
createStmt is sent using the simple query protocol (PQexec), which means that
all its commands are executed as a single transaction, and if the ALTER to LZ4
fails, so does the preceding CREATE. This is the same issue I see with
"CREATE..ATTACH PARTITION", above.

> but I agree that having such an option with restore will give more
> flexibility basically, by using the same dump we can restore to binary
> --with-lz4 as well as without-lz4 if such option exists with restore
> as well. But it seems in pg_restore we process token by token so if
> we want to implement such an option then I think we will have to parse
> the complete string of CREATE TABLE command and remove the compression
> option if it exists for any attribute. I am not sure whether providing
> this option is worth the complexity?

Oh...I realize now that this is different from the "tablespace" case in that
the column compression is not stored separately in the dump. And because it
exists for each column, not for the whole table. I suppose one answer to that
would be to make compression a per-table reloption, rather than a per-attribute
option. (I can anticipate that you'll hate this idea.)

--
Justin

On Mon, Mar 8, 2021 at 5:02 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> So now only pending point is, how do we handle the upgrade when you
> are upgrading from --with-lz4 to --without-lz4 binary and a couple of
> options discussed here are
> a) Should we allow table creation with lz4 even if it is compiled
> --without-lz4? In case of xml we always allow table creation even if
> it is compiled --wthout-libxml
> b) Instead of allowing this always, only allow during binary upgrade.